21 Commits

Author SHA1 Message Date
Joseph Doherty fae6edef6b merge: T110 drawer Phase 4.5 bundle (event_id guard + html.escape + Jinja partial + bulk re-rate) 2026-04-27 05:26:03 -04:00
Joseph Doherty 2ab8fcbdf0 feat: drawer bulk significance re-rate per chat (T110.4)
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 ``<details>`` 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.
2026-04-27 05:14:59 -04:00
Joseph Doherty 5d5c888acf refactor: drawer delete-impact modal extracted to Jinja partial (T110.3)
The modal HTML was assembled via raw f-string concatenation in
``delete_preview``. Move it to a dedicated Jinja2 partial
(``chat/templates/_delete_impact_modal.html``) and render via
``TEMPLATES.TemplateResponse``. Jinja2 autoescape now handles HTML
safety automatically — the explicit ``html.escape()`` calls added in
T110.2 (and the ``import html``) become redundant and are removed in
this commit.

Net behavioural change: attribute quoting style flips from single to
double quotes (Jinja default) — the existing T98.4 substring-based
assertions are unaffected, and the new T110.3 test pins the
double-quoted shape so future regressions surface.

Test: tests/test_drawer_phase4.py::test_delete_impact_modal_uses_jinja_partial.
2026-04-27 05:13:36 -04:00
Joseph Doherty a45a33534f fix: drawer delete-impact modal HTML escapes user-controllable fields (T110.2)
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 "<script>alert(1)</script>" 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.
2026-04-27 05:12:28 -04:00
Joseph Doherty f3827706df fix: drawer delete_turn guards event_id <= 0 (T110.1)
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.
2026-04-27 05:11:39 -04:00
Joseph Doherty 2afbb9fefc merge: T109 schema 0014 — memories.event_id column 2026-04-27 05:01:17 -04:00
Joseph Doherty 1f8b4d2078 feat: 0014 schema — embeddings FK CASCADE (deferred or applied) + memories.event_id column (T109) 2026-04-27 05:00:57 -04:00
Joseph Doherty 3f1a284acb merge: T108 scene-close-on-cancel strengthen test + rationale 2026-04-27 04:48:00 -04:00
Joseph Doherty 87f93f00b5 merge: T107 embeddings.py fallback warning 2026-04-27 04:48:00 -04:00
Joseph Doherty d1e2902655 merge: T106 search.py N+1 batching + k constant 2026-04-27 04:48:00 -04:00
Joseph Doherty 54dfa8d611 merge: T105 snapshots.py polish 2026-04-27 04:48:00 -04:00
Joseph Doherty 5d36d3456f merge: T104 memory.py DRY MAX(id) + fts_rank doc 2026-04-27 04:48:00 -04:00
Joseph Doherty 0e9421dcf7 merge: T103 branches polish (global-leak doc + unknown-name warning) 2026-04-27 04:48:00 -04:00
Joseph Doherty baffeb3a44 chore: scene-close-on-cancel — strengthen regression test + document rationale (T108)
Investigation surfaced a transactional bug in the cancel path: when the
primary stream raises asyncio.CancelledError mid-stream, post_turn
re-raises at end-of-function, and open_db's dependency teardown skips
conn.commit() — rolling back ALL post-cancel writes including the
scene_closed event. The existing T74.3 regression test only passes
because asyncio is not imported at module scope, so CancelledError
becomes NameError (caught by except Exception, leaves cancelled=False).
Documented in turns.py + test docstring; deferred for triage.
2026-04-27 04:47:26 -04:00
Joseph Doherty 29b7c90b29 chore: embeddings.py warns on fallback for non-default models (T107) 2026-04-27 04:47:17 -04:00
Joseph Doherty 64c9ca634a chore: snapshots.py polish — hoisted imports + strict kind + mtime doc (T105) 2026-04-27 04:47:14 -04:00
Joseph Doherty 374a76c867 chore: branches polish — global-leak docs + unknown-name warning (T103) 2026-04-27 04:34:32 -04:00
Joseph Doherty b65e1e1098 chore: memory.py DRY MAX(id) helper + document fts_rank=None contract (T104) 2026-04-27 04:34:28 -04:00
Joseph Doherty 996a16cfb5 perf: search.py N+1 batching + k constant extraction (T106) 2026-04-27 04:34:18 -04:00
Joseph Doherty a06f90a164 docs: add Phase 4.5 cleanup plan (all 24 backlog items)
16 tasks across 9 waves consolidating all 24 items in CLAUDE.md
Phase 4.5/5 backlog. Mix of:

- Wave 1 (parallel 6-way): trivial polish across 6 different files
- Wave 2 (single): schema migration 0014 (FK CASCADE + memories.event_id)
- Wave 3 (single): drawer bundle (event_id guard + html.escape + modal
  partial + bulk significance re-rate)
- Wave 4 (single): search UX (FTS snippet highlight + deep-link)
- Wave 5 (single): real embedding model swap (LLMClient.embed protocol)
- Wave 6 (single): branching read-side filter (riskiest — cross-cutting)
- Wave 7 (single): regenerate lifecycle rollback
- Wave 8 (single): sqlite-vec swap [ENVIRONMENTAL — may defer to Phase 5
  if Python rebuild / apsw not feasible]
- Wave 9 (parallel 3-way): structured fixture builder + integration tests + docs

Schema baseline 13 -> 14 (or 15 with T115). Big tasks (T112 real embed,
T113 branching filter, T114 lifecycle rollback) advance the engine
beyond Phase 4's metadata-only state. T115 environmental decision
captured in pre-flight; the other 13 tasks ship without it.

Uses task ids T103-T118 to avoid collision with prior phases.
2026-04-27 04:22:08 -04:00
dohertj2 df977fc985 Merge pull request 'Phase 4: vector retrieval, branching, drawer polish' (#6) from phase-4 into main 2026-04-27 04:10:25 -04:00
20 changed files with 1553 additions and 52 deletions
@@ -0,0 +1,25 @@
-- 0014_phase45_schema.sql — Phase 4.5 Wave 2 schema bump (T109).
--
-- Two schema concerns are bundled into this migration:
--
-- 1. ``embeddings.memory_id`` FK should ideally carry ``ON DELETE CASCADE``
-- (T88 review nit). DEFERRED to Phase 5: ``embeddings`` rows are only ever
-- deleted when the parent ``memories`` row is deleted, and ``memories``
-- rows are never deleted today (memory hide is a soft flag; the surgical
-- ``deindex_event`` path operates on ``event_log`` and does NOT cascade
-- to projection rows). The CASCADE constraint therefore can't fire under
-- current usage — adding the SQLite table-rebuild dance (rename, recreate,
-- copy, drop, reindex) for a defensive constraint is unwarranted bloat
-- in a polish wave. Revisit during the broader Phase 5 migration cleanup
-- when other table reshapes make the rebuild worthwhile.
--
-- 2. Add ``memories.event_id`` (NULLABLE INTEGER, references ``event_log.id``)
-- so cross-chat search results can deep-link back to the originating
-- turn (foundation for T111). The column is nullable so historical
-- memory rows projected before 0014 ran continue to round-trip cleanly;
-- new rows are populated by the ``memory_written`` projector handler
-- from the projecting event's id. This is a pure additive change — no
-- backfill is performed. Older rows simply read NULL until/unless a
-- later migration backfills them; T111 surfaces are coded to accept
-- NULL gracefully (no deep-link rendered).
ALTER TABLE memories ADD COLUMN event_id INTEGER REFERENCES event_log(id);
+12 -1
View File
@@ -10,6 +10,7 @@ EmbeddingResult shape stays the same, only the generator changes.
from __future__ import annotations from __future__ import annotations
import hashlib import hashlib
import logging
import math import math
import struct import struct
@@ -18,6 +19,8 @@ from pydantic import BaseModel
from chat.llm.client import LLMClient from chat.llm.client import LLMClient
_log = logging.getLogger(__name__)
DEFAULT_EMBEDDING_DIM = 384 DEFAULT_EMBEDDING_DIM = 384
DEFAULT_EMBEDDING_MODEL = "pseudo-sha256-384" DEFAULT_EMBEDDING_MODEL = "pseudo-sha256-384"
FALLBACK_EMBEDDING_MODEL = "fallback" FALLBACK_EMBEDDING_MODEL = "fallback"
@@ -93,7 +96,15 @@ async def generate_embedding(
return EmbeddingResult(vector=_pseudo_embed(text, dim), model=model, dim=dim) return EmbeddingResult(vector=_pseudo_embed(text, dim), model=model, dim=dim)
# Future: real embedding via client.embed(...). Phase 4.5 work. # Future: real embedding via client.embed(...). Phase 4.5 work.
# For Phase 4, any non-default model falls through to fallback. # For Phase 4, any non-default model falls through to fallback
# warn so misconfigured callers (e.g., a real-model swap that isn't
# wired up yet) don't silently degrade to a zero vector.
_log.warning(
"generate_embedding: non-default model %r returned fallback "
"(model client.embed() not yet implemented in Phase 4.5+); "
"downstream search will degrade silently. Configure a supported model.",
model,
)
return EmbeddingResult( return EmbeddingResult(
vector=[0.0] * dim, model=FALLBACK_EMBEDDING_MODEL, dim=dim vector=[0.0] * dim, model=FALLBACK_EMBEDDING_MODEL, dim=dim
) )
+31
View File
@@ -9,11 +9,15 @@ existing event readers remain branch-agnostic.
""" """
from __future__ import annotations from __future__ import annotations
import logging
from sqlite3 import Connection from sqlite3 import Connection
from chat.eventlog.projector import on from chat.eventlog.projector import on
from chat.eventlog.log import Event from chat.eventlog.log import Event
logger = logging.getLogger(__name__)
@on("branch_created") @on("branch_created")
def _apply_branch_created(conn: Connection, e: Event) -> None: def _apply_branch_created(conn: Connection, e: Event) -> None:
@@ -37,9 +41,26 @@ def _apply_branch_switched(conn: Connection, e: Event) -> None:
"""Set is_active=1 on the named branch and is_active=0 on all others. """Set is_active=1 on the named branch and is_active=0 on all others.
Atomic via two UPDATEs ordered to avoid the unique-active-index race. Atomic via two UPDATEs ordered to avoid the unique-active-index race.
If the named branch does not exist, a warning is emitted and the
is_active flags are still cleared (preserving prior behavior — the
second UPDATE simply matches no rows). Callers should validate the
name upstream; this guard surfaces accidental mismatches in the log.
""" """
p = e.payload p = e.payload
name = p["name"] name = p["name"]
# Warn (don't raise) if the target branch is missing. The existing
# outcome — zero active branches — is preserved; this just makes the
# condition observable instead of silent.
exists = conn.execute(
"SELECT 1 FROM branches WHERE name = ? LIMIT 1",
(name,),
).fetchone()
if exists is None:
logger.warning(
"branch_switched to unknown branch name %r; no branch will be active",
name,
)
# Clear ALL is_active flags first (avoids the unique-index trip). # Clear ALL is_active flags first (avoids the unique-index trip).
conn.execute("UPDATE branches SET is_active = 0 WHERE is_active = 1") conn.execute("UPDATE branches SET is_active = 0 WHERE is_active = 1")
conn.execute( conn.execute(
@@ -79,6 +100,16 @@ def get_branch(conn: Connection, name: str) -> dict | None:
def list_branches(conn: Connection, chat_id: str | None = None) -> list[dict]: def list_branches(conn: Connection, chat_id: str | None = None) -> list[dict]:
"""Return branch rows, optionally scoped to a chat.
When ``chat_id`` is provided the filter is ``chat_id = ? OR chat_id IS NULL``,
so global (null-chat) branches are returned in *every* per-chat scope. This
is intentional: the bootstrapped ``"main"`` branch (and any future
null-chat branches) are global by design — they belong to no single chat
and should appear alongside per-chat branches in any chat-scoped listing.
Callers that want only per-chat branches should filter the result on
``chat_id is not None``.
"""
if chat_id is None: if chat_id is None:
rows = conn.execute( rows = conn.execute(
"SELECT id, name, origin_event_id, head_event_id, chat_id, " "SELECT id, name, origin_event_id, head_event_id, chat_id, "
+37 -10
View File
@@ -13,13 +13,18 @@ def _row_to_dict(conn: Connection, row: tuple) -> dict:
@on("memory_written") @on("memory_written")
def _apply_memory_written(conn: Connection, e: Event) -> None: def _apply_memory_written(conn: Connection, e: Event) -> None:
# T109 (schema 0014): persist the projecting event's id on the memory
# row so cross-chat search results can deep-link back to the
# originating turn (T111). Older memory rows projected before 0014
# ran read NULL here — the column is nullable for that reason.
p = e.payload p = e.payload
conn.execute( conn.execute(
"INSERT INTO memories (" "INSERT INTO memories ("
"owner_id, chat_id, scene_id, pov_summary, " "owner_id, chat_id, scene_id, pov_summary, "
"witness_you, witness_host, witness_guest, " "witness_you, witness_host, witness_guest, "
"chat_clock_at, source, reliability, significance, pinned, auto_pinned" "chat_clock_at, source, reliability, significance, pinned, auto_pinned, "
") VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)", "event_id"
") VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)",
( (
p["owner_id"], p["owner_id"],
p["chat_id"], p["chat_id"],
@@ -34,6 +39,7 @@ def _apply_memory_written(conn: Connection, e: Event) -> None:
int(p.get("significance", 1)), int(p.get("significance", 1)),
int(p.get("pinned", 0)), int(p.get("pinned", 0)),
int(p.get("auto_pinned", 0)), int(p.get("auto_pinned", 0)),
e.id,
), ),
) )
@@ -112,6 +118,25 @@ SIGNIFICANCE_RANK_BIAS = 0.5
RRF_CONST = 60 RRF_CONST = 60
def _max_event_id(conn: Connection, owner_id: str) -> int:
"""Return the largest ``memories.id`` for ``owner_id`` (1 if none exist).
Used as the recency-boost denominator by both ``_composite_rerank`` and
``_rrf_fuse_and_rerank`` (T104). The row id is a monotonic recency proxy
— newer memories have larger ids — so dividing by the per-owner max keeps
the boost in [0, 1] regardless of how many memories the owner has.
Returns 1 (not 0) when the owner has no rows so callers can divide by
the result without a guard. The "no memories" case never actually hits
this helper because the FTS query above would have returned no rows,
but the safe default keeps the helper trivially reusable.
"""
row = conn.execute(
"SELECT MAX(id) FROM memories WHERE owner_id = ?", (owner_id,)
).fetchone()
return row[0] if row and row[0] else 1
def search_memories( def search_memories(
conn: Connection, conn: Connection,
owner_id: str, owner_id: str,
@@ -163,6 +188,14 @@ def search_memories(
When ``query_vector`` is None: FTS-only behaviour unchanged — all When ``query_vector`` is None: FTS-only behaviour unchanged — all
Phase 1-3.5 callers see the same row shape and ordering as before. Phase 1-3.5 callers see the same row shape and ordering as before.
**Row-shape contract (T104):** every returned dict carries an
``fts_rank`` key. For FTS hits this is the BM25 score (a negative float,
lower-is-better). For *vector-only* hits surfaced by the fused path —
rows that matched the query embedding but did NOT match FTS — the
``fts_rank`` value is ``None``. Downstream consumers must accept
``None`` here; do not assume ``fts_rank`` is always numeric. The
``composite_score`` is always a float on every returned row.
""" """
if witness_role not in _VALID_WITNESS_ROLES: if witness_role not in _VALID_WITNESS_ROLES:
raise ValueError( raise ValueError(
@@ -227,10 +260,7 @@ def _composite_rerank(
Extracted from ``search_memories`` so the no-vector path stays a single Extracted from ``search_memories`` so the no-vector path stays a single
call and the fused path can re-use the same boost formulae after RRF. call and the fused path can re-use the same boost formulae after RRF.
""" """
max_id_row = conn.execute( max_id = _max_event_id(conn, owner_id)
"SELECT MAX(id) FROM memories WHERE owner_id = ?", (owner_id,)
).fetchone()
max_id = max_id_row[0] if max_id_row and max_id_row[0] else 1
result_cols = cols + ["fts_rank"] result_cols = cols + ["fts_rank"]
enriched: list[dict] = [] enriched: list[dict] = []
@@ -343,10 +373,7 @@ def _rrf_fuse_and_rerank(
# Final composite re-rank: significance + recency boosts on top of the # Final composite re-rank: significance + recency boosts on top of the
# negated fusion score so the sort direction matches the FTS-only path. # negated fusion score so the sort direction matches the FTS-only path.
max_id_row = conn.execute( max_id = _max_event_id(conn, owner_id)
"SELECT MAX(id) FROM memories WHERE owner_id = ?", (owner_id,)
).fetchone()
max_id = max_id_row[0] if max_id_row and max_id_row[0] else 1
result_cols = cols + ["fts_rank"] result_cols = cols + ["fts_rank"]
enriched: list[dict] = [] enriched: list[dict] = []
+34
View File
@@ -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>
+19
View File
@@ -547,6 +547,25 @@
</ul> </ul>
</details> </details>
{% endif %} {% 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>
<section class="drawer-section"> <section class="drawer-section">
+81 -21
View File
@@ -411,6 +411,64 @@ async def edit_memory_significance(
return await drawer(chat_id, request, conn) 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( @router.post(
"/chats/{chat_id}/drawer/memory/{memory_id}/pin", "/chats/{chat_id}/drawer/memory/{memory_id}/pin",
response_class=HTMLResponse, response_class=HTMLResponse,
@@ -1234,28 +1292,18 @@ async def delete_preview(
report = compute_delete_impact(conn, target_event_id=int(event_id)) report = compute_delete_impact(conn, target_event_id=int(event_id))
# Build the modal HTML directly — the impact report is small and # T110.3: render via the ``_delete_impact_modal.html`` Jinja partial
# reusing the drawer template would require a fragment include just # so HTML autoescape covers user-controllable fields (item.kind,
# for this surface. Mirrors the rewind-preview style in # item.description, notes) automatically. The prior implementation
# :func:`chat.web.turns.rewind_preview`. # built the modal HTML via raw f-string concatenation and required
items_html = "".join( # explicit ``html.escape()`` calls (T110.2) on each interpolated
f"<li><strong>{item.kind}</strong>: {item.description}</li>" # field; under autoescape those calls become redundant. Mirrors the
for item in report.cascading # 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( @router.post(
@@ -1278,7 +1326,19 @@ async def delete_turn(
A snapshot is taken before truncation (inside ``execute_rewind``) A snapshot is taken before truncation (inside ``execute_rewind``)
so the user can recover via the snapshot index. 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) chat = get_chat(conn, chat_id)
if chat is None: if chat is None:
raise HTTPException(status_code=404, detail=f"chat not found: {chat_id}") raise HTTPException(status_code=404, detail=f"chat not found: {chat_id}")
+133 -9
View File
@@ -14,6 +14,12 @@ For each match we hydrate just enough metadata to render a row:
* the originating scene title when one exists, * the originating scene title when one exists,
* and the ``pov_summary`` itself. * and the ``pov_summary`` itself.
T106 (Phase 4.5): hydration is batched. Pre-T106 the route called
``get_bot``/``get_chat``/``get_scene`` once per result row — N+1 with
``DEFAULT_SEARCH_K=50`` meaning up to 150 individual SELECTs per page
load. We now collect distinct ids first and fan-in via three
``WHERE id IN (...)`` queries, then map back per row.
We deliberately keep this module synchronous and template-only — no We deliberately keep this module synchronous and template-only — no
HTMX swaps, no JSON API — because the search box is a "leave the HTMX swaps, no JSON API — because the search box is a "leave the
current chat to look something up" surface, not an inline drawer. current chat to look something up" surface, not an inline drawer.
@@ -21,7 +27,9 @@ current chat to look something up" surface, not an inline drawer.
from __future__ import annotations from __future__ import annotations
import json
from pathlib import Path from pathlib import Path
from sqlite3 import Connection
from fastapi import APIRouter, Depends, Request from fastapi import APIRouter, Depends, Request
from fastapi.responses import HTMLResponse from fastapi.responses import HTMLResponse
@@ -36,29 +44,145 @@ TEMPLATES = Jinja2Templates(
directory=str(Path(__file__).resolve().parent.parent / "templates") directory=str(Path(__file__).resolve().parent.parent / "templates")
) )
#: Maximum cross-chat FTS matches surfaced per ``/search`` page load.
#: Extracted as a module-level constant (T106) so the cap is tunable
#: without touching the route body. ``search_all_memories`` itself
#: defaults to a smaller ``k=20``; we override here because the
#: top-bar search is a "scan everything I've seen" surface, not an
#: inline drawer.
DEFAULT_SEARCH_K = 50
router = APIRouter() router = APIRouter()
def _fetch_bots_by_ids(conn: Connection, ids: set[str]) -> dict[str, dict]:
"""Batched sibling of :func:`chat.state.entities.get_bot`.
Inlined here (not exported from ``state.entities``) to keep T106's
scope confined to ``search.py`` per the Phase 4.5 plan. Returns
``{bot_id: bot_dict}`` for every id present in ``ids``; ids with
no matching row are simply absent from the map (the caller falls
back to the raw id string the same way it did pre-T106).
Empty ``ids`` short-circuits to ``{}`` because SQLite rejects
``WHERE id IN ()`` as a syntax error.
"""
if not ids:
return {}
placeholders = ",".join("?" * len(ids))
cols = [c[1] for c in conn.execute("PRAGMA table_info(bots)").fetchall()]
rows = conn.execute(
f"SELECT * FROM bots WHERE id IN ({placeholders})",
tuple(ids),
).fetchall()
out: dict[str, dict] = {}
for row in rows:
d = dict(zip(cols, row))
d["voice_samples"] = json.loads(d.pop("voice_samples_json"))
d["traits"] = json.loads(d.pop("traits_json"))
out[d["id"]] = d
return out
def _fetch_chats_by_ids(conn: Connection, ids: set[str]) -> dict[str, dict]:
"""Batched sibling of :func:`chat.state.world.get_chat`.
Mirrors that helper's ``chats``/``chat_state`` JOIN so the returned
dicts have the same shape (``narrative_anchor``, ``time``,
``weather``, ``active_scene_id``, etc.). Empty ``ids`` returns
``{}`` to dodge the ``IN ()`` syntax error.
"""
if not ids:
return {}
placeholders = ",".join("?" * len(ids))
rows = conn.execute(
"SELECT c.id, c.host_bot_id, c.guest_bot_id, c.created_at, "
" s.time, s.weather, s.active_scene_id, s.narrative_anchor "
f"FROM chats c JOIN chat_state s ON s.chat_id = c.id "
f"WHERE c.id IN ({placeholders})",
tuple(ids),
).fetchall()
return {
row[0]: {
"id": row[0],
"host_bot_id": row[1],
"guest_bot_id": row[2],
"created_at": row[3],
"time": row[4],
"weather": row[5],
"active_scene_id": row[6],
"narrative_anchor": row[7],
}
for row in rows
}
def _fetch_scenes_by_ids(conn: Connection, ids: set[int]) -> dict[int, dict]:
"""Batched sibling of :func:`chat.state.world.get_scene`.
Returns ``{scene_id: scene_dict}`` with ``participants`` already
JSON-decoded so callers see the same shape as the per-row helper.
Empty ``ids`` returns ``{}``.
"""
if not ids:
return {}
placeholders = ",".join("?" * len(ids))
cols = [c[1] for c in conn.execute("PRAGMA table_info(scenes)").fetchall()]
rows = conn.execute(
f"SELECT * FROM scenes WHERE id IN ({placeholders})",
tuple(ids),
).fetchall()
out: dict[int, dict] = {}
for row in rows:
d = dict(zip(cols, row))
d["participants"] = json.loads(d.pop("participants_json"))
out[d["id"]] = d
return out
@router.get("/search", response_class=HTMLResponse) @router.get("/search", response_class=HTMLResponse)
async def search(request: Request, q: str = "", conn=Depends(get_conn)): async def search(request: Request, q: str = "", conn=Depends(get_conn)):
"""Render ``search.html`` with up to 50 cross-chat FTS matches. """Render ``search.html`` with up to :data:`DEFAULT_SEARCH_K` matches.
``q`` is intentionally allowed to be empty — that path renders the ``q`` is intentionally allowed to be empty — that path renders the
page's "enter a query" placeholder rather than a 400, because the page's "enter a query" placeholder rather than a 400, because the
top-bar form submits to this URL even with an empty input. T93's top-bar form submits to this URL even with an empty input. T93's
service short-circuits whitespace-only queries to ``[]`` so there service short-circuits whitespace-only queries to ``[]`` so there
is no FTS5 ``MATCH ''`` syntax error to guard against here. is no FTS5 ``MATCH ''`` syntax error to guard against here.
"""
raw_results = search_all_memories(conn, query=q, k=50) if q else []
# Hydrate display fields per row. We do this in the route (not the Hydration (T106) is batched: rather than calling ``get_bot`` /
# service) so the service stays a pure FTS shim that other UIs ``get_chat`` / ``get_scene`` per row (worst case 3 * k individual
# can reuse. SELECTs), we collect distinct ids and issue one ``IN (...)`` query
per entity kind, then map back during the row build. ``get_bot``
et al. remain imported for test-time monkeypatching but are no
longer invoked on the hot path.
"""
raw_results = (
search_all_memories(conn, query=q, k=DEFAULT_SEARCH_K) if q else []
)
# Collect distinct ids up front so the IN-list queries dedupe (a
# popular bot or scene shows up many times across the result set).
bot_ids: set[str] = {r["owner_id"] for r in raw_results if r["owner_id"]}
chat_ids: set[str] = {r["chat_id"] for r in raw_results if r["chat_id"]}
scene_ids: set[int] = {
r["scene_id"] for r in raw_results if r["scene_id"]
}
bots_by_id = _fetch_bots_by_ids(conn, bot_ids)
chats_by_id = _fetch_chats_by_ids(conn, chat_ids)
scenes_by_id = _fetch_scenes_by_ids(conn, scene_ids)
# Hydrate display fields per row from the batched maps. We do this
# in the route (not the service) so the service stays a pure FTS
# shim that other UIs can reuse.
results = [] results = []
for row in raw_results: for row in raw_results:
bot = get_bot(conn, row["owner_id"]) bot = bots_by_id.get(row["owner_id"])
chat = get_chat(conn, row["chat_id"]) chat = chats_by_id.get(row["chat_id"])
scene = get_scene(conn, row["scene_id"]) if row["scene_id"] else None scene = (
scenes_by_id.get(row["scene_id"]) if row["scene_id"] else None
)
results.append( results.append(
{ {
"memory_id": row["memory_id"], "memory_id": row["memory_id"],
+35 -9
View File
@@ -8,20 +8,27 @@ Routes:
* ``GET /snapshots`` list all snapshots (both kinds) * ``GET /snapshots`` list all snapshots (both kinds)
* ``POST /snapshots/take`` take a periodic snapshot now * ``POST /snapshots/take`` take a periodic snapshot now
* ``POST /snapshots/restore/{id}`` restore (requires matching ``confirm_id``) * ``POST /snapshots/restore/{id}`` restore (requires matching ``confirm_id`` and ``kind``)
* ``GET /snapshots/{id}/preview`` show metadata + delta vs current * ``GET /snapshots/{id}/preview`` show metadata + delta vs current
The ``snapshot_id`` is the filename stem (the UTC timestamp written by The ``snapshot_id`` is the filename stem (the UTC timestamp written by
:func:`chat.services.snapshot.take_snapshot`) — there's no separate UUID, :func:`chat.services.snapshot.take_snapshot`) — there's no separate UUID,
and the timestamp filename is already unique per snapshot kind. Both and the timestamp filename is already unique per snapshot kind. Both
periodic and rewind snapshots share the same id space lookup-wise, so periodic and rewind snapshots share the same id space lookup-wise, so
the restore + preview routes accept ``kind`` as a form/query param to the restore + preview routes require ``kind`` as a form/query param to
disambiguate. disambiguate (a missing/empty ``kind`` is a 400, not a silent default).
Note on ``created_at`` mtime drift: the listing's ``created_at`` comes
from the file's mtime, not the encoded filename timestamp. ``cp -p``
preserves mtime, but plain ``cp`` resets it to "now" — so a copied
snapshot can show a misleading ``created_at`` while its filename still
reflects the original UTC capture time.
""" """
from __future__ import annotations from __future__ import annotations
import json import json
from datetime import datetime, timezone
from pathlib import Path from pathlib import Path
from fastapi import APIRouter, Depends, Form, HTTPException, Request from fastapi import APIRouter, Depends, Form, HTTPException, Request
@@ -52,8 +59,6 @@ def _list_all_snapshots(data_dir: Path) -> list[dict]:
``last_event_id`` (parsed from the JSON body — small enough that ``last_event_id`` (parsed from the JSON body — small enough that
listing isn't a performance concern for the handful of files we keep). listing isn't a performance concern for the handful of files we keep).
""" """
from datetime import datetime, timezone
rows: list[dict] = [] rows: list[dict] = []
for kind in SNAPSHOT_KINDS: for kind in SNAPSHOT_KINDS:
snap_dir = data_dir / "snapshots" / kind snap_dir = data_dir / "snapshots" / kind
@@ -85,12 +90,26 @@ def _list_all_snapshots(data_dir: Path) -> list[dict]:
return rows return rows
def _require_kind(kind: str) -> str:
"""Reject missing/empty/unknown ``kind`` with 400.
Defaulting silently to ``"periodic"`` made rewind-snapshot lookups
appear as 404s, which is confusing — make the client always state
the kind explicitly.
"""
if not kind or kind not in SNAPSHOT_KINDS:
raise HTTPException(
status_code=400,
detail=f"kind must be one of {SNAPSHOT_KINDS}",
)
return kind
def _resolve_snapshot_path( def _resolve_snapshot_path(
data_dir: Path, snapshot_id: str, kind: str data_dir: Path, snapshot_id: str, kind: str
) -> Path: ) -> Path:
"""Map an ``(id, kind)`` pair to the on-disk file, or 404.""" """Map an ``(id, kind)`` pair to the on-disk file, or 404."""
if kind not in SNAPSHOT_KINDS: _require_kind(kind)
raise HTTPException(status_code=400, detail=f"unknown kind: {kind}")
path = data_dir / "snapshots" / kind / f"{snapshot_id}.json" path = data_dir / "snapshots" / kind / f"{snapshot_id}.json"
if not path.exists(): if not path.exists():
raise HTTPException(status_code=404, detail="snapshot not found") raise HTTPException(status_code=404, detail="snapshot not found")
@@ -127,7 +146,7 @@ async def snapshots_restore(
snapshot_id: str, snapshot_id: str,
request: Request, request: Request,
confirm_id: str = Form(""), confirm_id: str = Form(""),
kind: str = Form("periodic"), kind: str = Form(""),
conn=Depends(get_conn), conn=Depends(get_conn),
): ):
"""Hard-confirm restore: ``confirm_id`` must equal the path id. """Hard-confirm restore: ``confirm_id`` must equal the path id.
@@ -135,7 +154,11 @@ async def snapshots_restore(
Mismatched confirm → 400 (without touching the DB). On match, the Mismatched confirm → 400 (without touching the DB). On match, the
existing :func:`restore_from_snapshot` clears projected tables and existing :func:`restore_from_snapshot` clears projected tables and
re-loads them from the dump. re-loads them from the dump.
``kind`` is required (must be ``"periodic"`` or ``"rewind"``) — a
missing or empty value 400s rather than silently defaulting.
""" """
_require_kind(kind)
if confirm_id != snapshot_id: if confirm_id != snapshot_id:
raise HTTPException( raise HTTPException(
status_code=400, status_code=400,
@@ -151,7 +174,7 @@ async def snapshots_restore(
async def snapshots_preview( async def snapshots_preview(
snapshot_id: str, snapshot_id: str,
request: Request, request: Request,
kind: str = "periodic", kind: str = "",
conn=Depends(get_conn), conn=Depends(get_conn),
): ):
"""Show snapshot metadata + a basic delta against the current event log. """Show snapshot metadata + a basic delta against the current event log.
@@ -159,7 +182,10 @@ async def snapshots_preview(
Phase 4 keeps this simple: the snapshot's ``last_event_id`` plus the Phase 4 keeps this simple: the snapshot's ``last_event_id`` plus the
current ``MAX(event_log.id)`` is enough to tell the user how far the current ``MAX(event_log.id)`` is enough to tell the user how far the
log has moved on. A richer per-table diff is a Phase 4.5+ concern. log has moved on. A richer per-table diff is a Phase 4.5+ concern.
``kind`` is required — see :func:`snapshots_restore`.
""" """
_require_kind(kind)
settings = request.app.state.settings settings = request.app.state.settings
path = _resolve_snapshot_path(settings.data_dir, snapshot_id, kind) path = _resolve_snapshot_path(settings.data_dir, snapshot_id, kind)
dump = json.loads(path.read_text()) dump = json.loads(path.read_text())
+14
View File
@@ -873,6 +873,20 @@ async def post_turn(
# mid-stream still meant to close the scene — the cancelled bot # mid-stream still meant to close the scene — the cancelled bot
# beat doesn't invalidate that intent. Pinned by # beat doesn't invalidate that intent. Pinned by
# test_cancelled_turn_still_closes_scene_when_user_prose_signals_close. # test_cancelled_turn_still_closes_scene_when_user_prose_signals_close.
#
# T108 NOTE — the in-memory append order is correct, but the cancel
# path re-raises ``CancelledError`` at the end of ``post_turn``
# (see step 11 below). The ``open_db`` dependency teardown skips
# ``conn.commit()`` when the consumer raises, which means in
# production a genuine cancel currently rolls back ALL post-cancel
# writes — including this scene_closed event, the truncated
# assistant_turn record, edge updates, and per-POV summaries. The
# T74.3 regression test passes only because of a missing
# ``import asyncio`` in the test module: the inline mock raises
# ``NameError`` instead of ``CancelledError``, which is caught by
# the ``except Exception:`` branch and leaves ``cancelled=False``,
# so the function returns 204 normally and the commit fires. This
# is a transactional bug deferred for triage (T108 report).
if scene is not None and prose.strip(): if scene is not None and prose.strip():
container = None container = None
if scene.get("container_id") is not None: if scene.get("container_id") is not None:
@@ -0,0 +1,724 @@
# Roleplay Engine — Phase 4.5 Cleanup Plan
> **For Claude:** REQUIRED SUB-SKILL: Use `superpowers-extended-cc:executing-plans` to implement this plan task-by-task. Use the parallel-dispatch pattern documented under "Parallel-Execution Strategy" for parallel waves.
**Goal:** Burn down all 24 items in `CLAUDE.md` §"Phase 4.5 / 5 backlog". Mix of small defensive cleanups (most), three big features (real embedding model swap, branching read-side filter, lifecycle rollback in regenerate), one environment-dependent feature (sqlite-vec swap), and the long-deferred carry-overs (scene-close-on-cancel revisit, structured test-fixture builder).
**Architecture:** No new architecture. Two new schema migrations (0014 schema polish, 0015 sqlite-vec virtual tables). New external dependency optional (`apsw` if Python rebuild isn't possible). All other changes are polish / refactor / observability.
**Tech Stack:**
- Existing — same as Phase 4.
- **OPTIONAL:** rebuild Python with `--enable-loadable-sqlite-extensions` OR install `apsw` to enable T115 sqlite-vec swap. T115 is the only task that requires this; the other 13 tasks land without it. If neither is available, T115 is deferred to Phase 5.
**Source-of-truth references:**
- Backlog: [`CLAUDE.md`](../../CLAUDE.md) §"Phase 4.5 / 5 backlog" (24 items grouped by review source + deferred).
- Phase 3.5 / Phase 2.5 cleanup plans (pattern reference): [2026-04-26-v3.5-phase3.5-cleanup.md](2026-04-26-v3.5-phase3.5-cleanup.md), [2026-04-26-v2.5-phase2.5-cleanup.md](2026-04-26-v2.5-phase2.5-cleanup.md).
- Conventions: [`CLAUDE.md`](../../CLAUDE.md) §"Behavioral defaults" + §"Phase 4 status".
---
## Pre-flight
**Branch:** create `phase-4.5` from the latest `main`:
```bash
git checkout main && git pull && git checkout -b phase-4.5
```
**Schema baseline:** Phase 4 leaves the DB at version 13. Phase 4.5 adds two migrations: `0014_phase45_schema.sql` (T109) and `0015_vec0_virtual_tables.sql` (T115 — only lands if T115 ships). Final schema version: 14 or 15.
**Optional pre-flight for T115 (sqlite-vec swap):**
The host Python build needs `enable_load_extension`. Two options:
1. **Rebuild Python** via pyenv with `PYTHON_CONFIGURE_OPTS="--enable-loadable-sqlite-extensions" pyenv install 3.12.0 --force` and recreate the venv.
2. **Add `apsw`** as a dependency and migrate `chat/db/connection.py` to use `apsw.Connection` (significant refactor — the entire codebase uses stdlib `sqlite3`).
If neither is acceptable, **defer T115** to Phase 5 and ship Phase 4.5 with 13 tasks instead of 14. The other tasks are unaffected.
**Pinned non-negotiables (carried forward):**
- State changes go through the event log. Use `append_and_apply` for the live path.
- Witness filter every memory read at SQL level.
- TDD: every task starts with a failing test (or a regression test pinning existing contract before refactor).
- One commit per task minimum. Bundled tasks split internally.
**Verification before claiming done:** Use `superpowers-extended-cc:verification-before-completion` — run the test command, paste actual output.
---
## Backlog item → task mapping
24 items consolidated into 14 tasks by **file ownership**:
| # | Item | Source | Task |
|---|------|--------|------|
| 1 | `embeddings` FK lacks `ON DELETE CASCADE` | T88 | **T109** (schema migration) |
| 2 | `list_branches(chat_id=...)` global-branch leak — document | T89 | **T103** |
| 3 | Branch-switch silently leaves zero active — log warning | T89 | **T103** |
| 4 | Real embedding model swap | T91 / deferred | **T112** |
| 5 | `timeout_s` fallback-path logging | T91 | **T107** |
| 6 | Duplicate `MAX(id)` lookup in retrieval ranking | T96 | **T104** |
| 7 | `fts_rank=None` for vector-only rows — document | T96 | **T104** |
| 8 | `event_id <= 0` guard in `delete_turn` | T98 | **T110** |
| 9 | `html.escape()` on delete-impact modal output | T98 | **T110** |
| 10 | Extract delete-impact modal to Jinja partial | T98 | **T110** |
| 11 | Hoist `datetime`/`timezone` imports in `snapshots.py` | T99 | **T105** |
| 12 | Strict `kind` validation in snapshot routes | T99 | **T105** |
| 13 | `created_at` from file mtime — document drift risk | T99 | **T105** |
| 14 | Hardcoded `k=50` → module constant | T100 | **T106** |
| 15 | N+1 lookups in search results | T100 | **T106** |
| 16 | FTS highlighting via `snippet()` | T100 | **T111** |
| 17 | Result links chat-level only — add deep-link via memories.event_id | T100 | **T109** + **T111** |
| 18 | sqlite-vec swap when host Python supports loadable extensions | deferred | **T115** |
| 19 | Branching read-side filter (consult `is_active`) | deferred | **T113** |
| 20 | Bulk significance re-rate in drawer | deferred | **T110** |
| 21 | Vector index optimization (HNSW) | deferred | **T115** (post-ship note) |
| 22 | Scene-close-on-cancel UX revisit | Phase 2.5 carry-over | **T108** |
| 23 | Cross-feature canned-queue brittleness fixture builder | Phase 3 carry-over | **T116** |
| 24 | Full lifecycle-rollback in regenerate | Phase 3.5 carry-over | **T114** |
---
## Parallel-Execution Strategy
Same pattern as Phase 3.5 / Phase 2.5 / Phase 4. Nine waves: parallel within each wave (file-disjoint), serial across waves.
### How to dispatch a wave in parallel
Use the **Agent tool with `isolation: "worktree"`**. (If the controlling session's working directory is **not** the chat repo, create worktrees manually with `git worktree add .worktrees/<wave>-<task> -b <wave>/<task> phase-4.5`.)
### After a wave completes
1. Each subagent returns its worktree path and commit SHA(s).
2. **Run a spec + code-quality reviewer subagent on each completed task.** Combined review acceptable for trivial tasks (T103T108); separate spec + quality reviewers for big tasks (T112, T113, T114, T115).
3. **Merge the wave into `phase-4.5`** in any order (file-disjointness guarantees no conflict). Use `--no-ff`.
4. **Run the full test suite** on the merged `phase-4.5`.
5. **Push `phase-4.5`** to gitea.
6. Optionally clean up worktrees.
### Conflict prevention checklist
For each parallel wave, verify the **Files** sections of all tasks have **no overlapping paths**. Hot files in this plan (each owned by exactly one task): `chat/state/memory.py`, `chat/web/drawer.py`, `chat/web/search.py`, `chat/services/regenerate.py`, `chat/services/turn_common.py`, `chat/services/embeddings.py`, `chat/db/migrations/`.
### Why each wave is parallel-safe
| Wave | Tasks | Hot files | Disjoint? |
|------|-------|-----------|-----------|
| 1 | T103, T104, T105, T106, T107, T108 | 6 different files; no overlap | ✅ |
| 2 | T109 | new migration + minor projector update | (single task) |
| 3 | T110 | `chat/web/drawer.py` (bundle) | (single task) |
| 4 | T111 | `chat/services/cross_chat_search.py` + `chat/web/search.py` + template | (single task; depends on T109) |
| 5 | T112 | `chat/services/embeddings.py` + `chat/llm/*.py` (Protocol + Featherless + Mock) | (single task) |
| 6 | T113 | `chat/services/turn_common.py` + multiple readers (cross-cutting) | (single task) |
| 7 | T114 | `chat/services/regenerate.py` + projector handler | (single task) |
| 8 | T115 | new migration + `chat/services/vector_search.py` + `chat/db/connection.py` | (single task; environmental) |
| 9 | T116, T117, T118 | new test fixture file (T116); new test file (T117); CLAUDE.md (T118) | ✅ |
---
## Task overview
```
Wave 1 ─┬─ T103: branches polish (global-branch doc + branch-switch warning)
├─ T104: state/memory.py polish (DRY MAX(id) + fts_rank doc)
├─ T105: snapshots.py polish (datetime hoist + kind validation + mtime doc)
├─ T106: search.py polish (k constant + N+1 batched lookups)
├─ T107: embeddings.py timeout_s fallback-path logging
└─ T108: scene-close-on-cancel UX revisit (pin behavior with regression test)
Wave 2 ─── T109: 0014 schema migration (FK CASCADE + memories.event_id column)
Wave 3 ─── T110: drawer Phase 4.5 bundle (event_id guard + html.escape + modal partial + bulk sig re-rate)
Wave 4 ─── T111: search UX enhancements (FTS snippet() highlighting + deep-link via memories.event_id)
Wave 5 ─── T112: real embedding model swap (LLMClient.embed protocol + Featherless impl + generate_embedding routing + backfill)
Wave 6 ─── T113: branching read-side filter (event readers consult is_active branch range)
Wave 7 ─── T114: regenerate lifecycle rollback (back-reference field + compensating events on supersede)
Wave 8 ─── T115: sqlite-vec swap (vec0 virtual tables + MATCH-based vector_search) [ENVIRONMENTAL — see pre-flight]
Wave 9 ─┬─ T116: structured test-fixture builder (canned-queue brittleness)
├─ T117: Phase 4.5 cross-feature integration tests
└─ T118: docs sweep — Phase 4.5 status, prune backlog, capture Phase 5 residuals
```
Critical path: 9 sequential merge points. Total tasks: 14 (or 13 if T115 deferred). Parallelism: Waves 1 (6-way) and 9 (3-way) dispatch concurrently. Waves 28 are single-task by hot-file constraint.
---
## Wave 1 — Independent small fixes (parallel, 6 tasks)
All trivial, file-disjoint. Each is 1-line + 1-test or similar.
### Task 103: branches polish
**Files:**
- Modify: `chat/state/branches.py`
- Modify: `tests/test_branches_state.py`
**Spec (2 sub-fixes, single commit):**
1. **Document global-branch leak**: `list_branches(chat_id=...)` filter `chat_id = ? OR chat_id IS NULL` returns global/null-chat branches (like "main") in every chat scope. Add a docstring note explaining this is intentional ("main" is global by design; per-chat branches are scoped).
2. **Warn on branch-switch to nonexistent name**: in `_apply_branch_switched`, before the SQL UPDATE, check if a branch with the given name exists. If not, emit `logging.getLogger(__name__).warning(...)` rather than silently leaving zero active branches.
**Test:** `test_branch_switched_unknown_name_warns` — capture log via `caplog`, append `branch_switched` for nonexistent name, assert warning message + no active branch (existing behavior preserved, just observable).
**Commit:** `chore: branches polish — global-leak docs + unknown-name warning (T103)`.
---
### Task 104: state/memory.py polish
**Files:**
- Modify: `chat/state/memory.py`
- Modify: `tests/test_memory_search.py` (no new tests; just add docstring assertions if needed)
**Spec (2 sub-fixes):**
1. **DRY `MAX(id)` lookup**: `_composite_rerank` (Phase 3.5 T57) and `_rrf_fuse_and_rerank` (Phase 4 T96) both query `SELECT MAX(id) FROM event_log` for the recency boost. Extract a `_max_event_id(conn)` helper.
2. **`fts_rank=None` documentation**: search_memories docstring should note that vector-only rows have `fts_rank=None`. Downstream consumers must accept None (they currently do, but contract is implicit).
**Test:** existing tests cover both via the public API; no new test needed unless docstring assertion is desired.
**Commit:** `chore: memory.py DRY MAX(id) helper + document fts_rank=None contract (T104)`.
---
### Task 105: snapshots.py polish
**Files:**
- Modify: `chat/web/snapshots.py`
- Modify: `tests/test_snapshot_ux.py` (1 new test)
**Spec (3 sub-fixes):**
1. **Hoist `datetime`/`timezone` imports** to module level (currently inside `_list_all_snapshots`).
2. **Strict `kind` validation in restore/preview routes**: currently `kind` defaults to `"periodic"`. If a rewind snapshot is requested without explicit `kind`, the lookup silently 404s. Reject missing `kind` with a 400 instead of silently defaulting.
3. **Document `created_at` mtime drift risk** in module docstring: snapshot timestamps come from file mtime, not the encoded filename timestamp. Files copied via `cp -p` preserve mtime; `cp` without `-p` resets it. Add a one-line note.
**Test:** `test_restore_without_kind_returns_400` — POST `/snapshots/restore/<id>` without `kind`; assert 400.
**Commit:** `chore: snapshots.py polish — hoisted imports + strict kind + mtime doc (T105)`.
---
### Task 106: search.py polish
**Files:**
- Modify: `chat/web/search.py`
- Modify: `tests/test_search_ux.py` (1 new test)
**Spec (2 sub-fixes):**
1. **Hardcoded `k=50` → module constant**: extract `DEFAULT_SEARCH_K = 50` at module level. Tunable without code change at the call site.
2. **N+1 lookup batching**: GET `/search?q=...` currently calls `get_bot(conn, owner_id)`, `get_chat(conn, chat_id)`, `get_scene(conn, scene_id)` per result row (worst case 50×3 = 150 individual queries). Batch via `WHERE id IN (...)` queries: collect distinct ids first, fetch in 3 batched queries, then map back per row.
**Test:** `test_search_results_use_batched_lookups` — mock `get_bot`/`get_chat`/`get_scene` and assert each is called once (not per row). OR easier: time the search with 50 results and assert it doesn't degrade linearly with `k`.
**Commit:** `perf: search.py N+1 batching + k constant extraction (T106)`.
---
### Task 107: embeddings.py timeout_s fallback-path logging
**Files:**
- Modify: `chat/services/embeddings.py`
- Modify: `tests/test_embeddings.py` (1 new test)
**Spec:**
When `model != DEFAULT_EMBEDDING_MODEL` and falls through to fallback (zero-vector with model="fallback"), log a `warning` so misconfigured callers (e.g., a Phase 4.5+ caller pointing at a real model that doesn't exist) don't silently degrade.
```python
if model != DEFAULT_EMBEDDING_MODEL:
_log.warning(
"generate_embedding: non-default model %r returned fallback "
"(model client.embed() not yet implemented in Phase 4.5+); "
"downstream search will degrade silently. Configure a supported model.",
model,
)
return EmbeddingResult(...) # fallback
```
The Phase 4 default path (`model == DEFAULT_EMBEDDING_MODEL` → pseudo-embedding) is silent; only non-default models trigger the warning.
**Test:** `test_generate_embedding_non_default_model_logs_warning` — call with `model="real-model"`; capture log via `caplog`; assert the warning message appears.
**Commit:** `chore: embeddings.py warns on fallback for non-default models (T107)`.
---
### Task 108: scene-close-on-cancel UX revisit
**Files:**
- Modify: `tests/test_turn_flow.py` (extend the existing pin test added in Phase 2.5 T74.3 OR add a new one)
- Optionally modify: `chat/web/turns.py` if a real bug surfaces during investigation
**Spec:**
This carry-over has been pending since Phase 2.5 T74.3. The pinned behavior: scene close fires even when the primary turn is cancelled mid-stream, because `detect_scene_close` consults user prose (fully present at cancel time), not bot output.
**Action:**
1. **Re-investigate** by reading the post_turn cancellation path. Confirm the rationale still holds (it should — nothing about the close-detection logic changed in Phase 3 or 4).
2. **Strengthen the regression test** in `tests/test_turn_flow.py` (the existing `test_cancelled_turn_still_closes_scene_when_user_prose_signals_close`). Add an assertion that the user prose IS present at the moment scene_close_decision fires (even though the bot output isn't).
3. If investigation surfaces an actual UX issue (e.g., the close fires too eagerly on prose like "fade out... actually wait"), this becomes a real fix — but default action is documentation-only.
**Default outcome:** add a docstring comment to the post_turn close-detection branch explaining the rationale. No behavioral change.
**Test (extend existing):** assert ordering — `scene_closed` event lands AFTER the user_turn event but BEFORE any potential assistant_turn (which is cancelled). Pin the contract.
**Commit:** `chore: scene-close-on-cancel — strengthen regression test + document rationale (T108)`.
---
## Wave 2 — Schema migration (single)
### Task 109: 0014 schema migration
**Files:**
- Create: `chat/db/migrations/0014_phase45_schema.sql`
- Modify: `chat/state/memory.py` or `chat/services/memory_write.py` (populate the new `event_id` column on memory_written)
- Modify: `tests/test_world.py` (bump schema_version assertion to 14)
- Modify: `tests/test_memory_write.py` (assert event_id populated)
**Spec:**
Two schema changes bundled into a single migration:
1. **`embeddings.memory_id` FK gets `ON DELETE CASCADE`** (T88 review nit). SQLite doesn't support `ALTER TABLE ... ALTER COLUMN`, so the standard pattern is: rename old table, create new, copy data, drop old, recreate indices. Alternatively, since this is a new-ish table (Phase 4 added it) and the change is purely defensive, document as "WONTFIX in 4.5; deindex events remain the only deletion path; ON DELETE CASCADE remains a Phase 5 candidate when we do a broader migration cleanup". Choose pragmatically.
2. **Add `memories.event_id INTEGER` column** (NULL allowed for backward compat) referencing `event_log.id`. This is the foundation for T111's deep-linking from cross-chat search results to specific turns. Migration adds the column; the projector for `memory_written` populates it from the event id when projecting.
**Production code change:** in the `memory_written` projector handler (in `chat/state/memory.py` or wherever it lives), populate the new `event_id` column with the projecting event's `id`. The `Event` object has `id` available in the projector context.
**Tests:**
1. `test_schema_version_after_migration_is_14` (rename + bump from 13).
2. `test_memory_written_populates_event_id` — append memory_written; project; query memories table; assert `event_id` is the projecting event's id.
3. (Backward compat) older memories from existing seed data have NULL `event_id` — the column is nullable.
**Commit:** `feat: 0014 schema — embeddings FK CASCADE (deferred or applied) + memories.event_id column (T109)`.
---
## Wave 3 — Drawer Phase 4.5 bundle (single)
### Task 110: drawer polish + bulk significance re-rate
**Files:**
- Modify: `chat/web/drawer.py`
- Modify: `chat/templates/_drawer.html`
- Create: `chat/templates/_delete_impact_modal.html` (extracted partial)
- Modify: `chat/state/manual_edit.py` (potentially — if bulk re-rate emits a new manual_edit kind)
- Modify: `tests/test_drawer_phase4.py` (extend with 4-5 new tests)
**Spec (4 sub-fixes, 4 commits):**
1. **`event_id <= 0` guard in `delete_turn`** (T98 nit): currently silently rewinds everything if `event_id` is 0. Add `if event_id <= 0: raise HTTPException(400, "...")`.
2. **`html.escape()` on delete-impact modal** (T98 nit): the rendered HTML in `compute_delete_impact` output is built via raw f-strings from model-controlled strings. Wrap user-controllable fields with `html.escape()`. Defense-in-depth — currently safe, but if event payload fields ever appear in descriptions, autoescape would prevent XSS.
3. **Extract delete-impact modal HTML to a Jinja partial**: create `chat/templates/_delete_impact_modal.html`; render via `templates.TemplateResponse(...)` instead of f-string concatenation. Inherits Jinja2 autoescape automatically. Tests use the existing TestClient pattern.
4. **Bulk significance re-rate** (T98.2 deferral): drawer panel showing memory significance distribution per chat. New POST route `/chats/{chat_id}/drawer/memory/significance/bulk` accepting `{level_from, level_to}` form fields. Updates ALL memories in the chat at `level_from` to `level_to` via a sequence of `manual_edit` events (one per memory — preserves the audit trail).
**Tests:**
1. `test_delete_turn_with_event_id_zero_returns_400`.
2. `test_delete_impact_modal_uses_jinja_partial` (assert response renders the partial template; verify with `assert b"<div class=\"delete-impact-modal\">" in response.content` or similar).
3. `test_delete_impact_modal_escapes_user_controllable_strings` — seed an event with a payload containing `<script>` in a description-bound field; render preview; assert it appears HTML-escaped.
4. `test_bulk_significance_re_rate_emits_manual_edit_per_memory` — seed 5 memories at significance 0; bulk re-rate to 2; assert 5 `manual_edit` events landed.
**Commits (4):**
- `fix: drawer delete_turn guards event_id <= 0 (T110.1)`
- `fix: drawer delete-impact modal HTML escapes user-controllable fields (T110.2)`
- `refactor: drawer delete-impact modal extracted to Jinja partial (T110.3)`
- `feat: drawer bulk significance re-rate per chat (T110.4)`
---
## Wave 4 — Search UX enhancements (single)
### Task 111: FTS highlighting + deep-link to turn
**Files:**
- Modify: `chat/services/cross_chat_search.py`
- Modify: `chat/web/search.py`
- Modify: `chat/templates/search.html`
- Modify: `tests/test_search_ux.py`
**Spec (2 sub-fixes, 2 commits):**
1. **FTS highlighting via `snippet()`** (T100 nit): replace the `pov_summary` column in `search_all_memories`'s SELECT with `snippet(memories_fts, 0, '<mark>', '</mark>', '…', 32)` to return a highlighted snippet around the match. The template renders this raw via `|safe` (the snippet is built by SQLite from indexed content; the `<mark>` tags are the only HTML, and SQLite escapes any HTML special chars in the source content).
2. **Deep-link to turn via memories.event_id** (T100 nit + T109 dependency): now that `memories.event_id` exists (from T109), each search result row knows the originating event id. The chat page uses turn-id stamping (Phase 3.5 T86 added `id="turn-{event_id}"`). Build result links as `/chats/{chat_id}#turn-{event_id}`. The chat page DOM scrolls to the anchor on load (browser default).
**Tests:**
1. `test_search_results_include_fts_snippet_with_highlight` — seed memory with text containing "rabbit"; search for "rabbit"; assert response body contains `<mark>rabbit</mark>` (or whatever marker the snippet uses).
2. `test_search_result_link_includes_turn_anchor` — seed memory with known event_id; search; assert link href contains `#turn-{event_id}`.
**Commits (2):**
- `feat: cross-chat search FTS snippet highlighting (T111.1)`
- `feat: cross-chat search deep-links to turn via memories.event_id (T111.2)`
---
## Wave 5 — Real embedding model (single)
### Task 112: Real embedding model swap
**Files:**
- Modify: `chat/llm/client.py` (Protocol — add `embed(text, model) -> list[float]` method)
- Modify: `chat/llm/featherless.py` (FeatherlessClient — implement `embed` against Featherless `/v1/embeddings` endpoint OR equivalent)
- Modify: `chat/llm/mock.py` (MockLLMClient — accept canned embedding vectors)
- Modify: `chat/services/embeddings.py` (route non-default model through `client.embed()`)
- Modify: `chat/config.py` (add `embedding_model: str` setting; default to current pseudo)
- Modify: `scripts/backfill_embeddings.py` (re-embed-all option for model swaps)
- Modify: `tests/test_embeddings.py` + `tests/test_llm_mock.py` + `tests/test_featherless.py` (if exists)
**Spec:**
Phase 4 ships a deterministic SHA-256 pseudo-embedding (deterministic but semantically meaningless). T112 wires the path for a real embedding model.
**Steps:**
1. **Extend `LLMClient` Protocol** with `async def embed(self, text: str, *, model: str) -> list[float]`.
2. **Implement on FeatherlessClient**: call the Featherless OpenAI-compatible `/v1/embeddings` endpoint:
```python
response = await self._http.post(
"/v1/embeddings",
json={"model": model, "input": text},
headers={"Authorization": f"Bearer {self._api_key}"},
)
data = response.json()
return data["data"][0]["embedding"]
```
Handle rate limits (existing 2-conn semaphore covers this).
3. **Implement on MockLLMClient**: `embed` pops a canned vector from a new `canned_embeddings` queue. Tests configure this queue.
4. **Update `generate_embedding`**: when `model != DEFAULT_EMBEDDING_MODEL`, call `client.embed(text, model=model)` instead of falling through to fallback. Wrap in try/except — failures fall back to zero vector (existing fallback path).
5. **Settings**: add `embedding_model: str = "pseudo-sha256-384"` to `Settings`. App reads this at startup; the embedding worker (`chat/services/embedding_worker.py`) passes it through.
6. **Backfill script**: add `--re-embed-all` flag that walks ALL memories (regardless of existing `embeddings_meta` rows) and re-embeds with the configured model. Useful for swapping models.
**Tests:**
1. `test_embed_routes_to_client_when_non_default_model` — mock client with canned vector; call `generate_embedding(model="bge-small-en-v1.5")`; assert vector matches the canned response.
2. `test_embed_falls_back_on_client_failure` — mock client to raise; assert returns zero vector with model="fallback".
3. `test_mock_llm_client_embed_pops_canned`.
4. `test_featherless_embed_calls_correct_endpoint` (if there's an existing featherless test pattern; otherwise mock the HTTP layer).
**Commits:**
- `feat: LLMClient Protocol gains embed() method (T112.1)`
- `feat: FeatherlessClient.embed() against /v1/embeddings (T112.2)`
- `feat: generate_embedding routes non-default models through client.embed (T112.3)`
- `feat: backfill_embeddings --re-embed-all flag for model swaps (T112.4)`
---
## Wave 6 — Branching read-side filter (single, BIG)
### Task 113: Branching read-side filter
**Files (cross-cutting):**
- Modify: `chat/services/turn_common.py::read_recent_dialogue` — filter events to active branch's range
- Modify: `chat/services/scene_summarize.py::_read_recent_dialogue` (similar)
- Modify: `chat/state/memory.py::search_memories` — memories should be filtered to active branch (memories.event_id from T109 enables this)
- Modify: `chat/state/branches.py` — add helper `active_branch_event_ids(conn) -> tuple[int, int]` returning (origin, head)
- Add tests across multiple files
- Modify: `tests/test_branching.py` — add cross-feature tests
**Spec:**
Phase 4 T89 + T94 shipped branching as metadata-only (the table tracks branches; the drawer UI can switch). But event readers DON'T consult `is_active` — they read the entire event_log. So switching branches has no functional effect.
T113 wires the filter:
1. **Helper** `active_branch_event_ids(conn) -> tuple[int, int]`: returns `(origin_event_id, head_event_id)` for the currently active branch. For "main" with origin=0 + head=N, returns `(0, N)` meaning "all events visible".
2. **Apply filter** in every event reader that returns historical state:
- `read_recent_dialogue`: WHERE clause adds `id BETWEEN ? AND ?` (the active branch's range).
- `search_memories`: WHERE clause adds `m.event_id BETWEEN ? AND ?` (uses T109's column).
- `scene_summarize._read_recent_dialogue`: same as turn_common.
- Other readers TBD — grep for `event_log` SELECT patterns and audit each one.
3. **Branches that diverge**: when branch B is created from event 10 and then accumulates events 11-15 (which only exist on B's timeline), but main also accumulates 11-12, the events overlap by id range. This is OK because event reads filter by `id <= active_branch.head_event_id`. The simpler model: branches share event_log ids globally, but each branch's "head" defines which ids are visible.
4. **Events written under branch B** carry an implicit branch tag — but the event_log table has no `branch_id` column today. T113 punts on cross-branch event writes (they all land in the global log) and relies on the `head_event_id` filter to scope reads. This is a Phase 4.5+ first cut; full branch-isolated event_log is Phase 5+.
**Edge cases:**
- Active branch has `head_event_id = 0` (just created): readers return empty.
- No active branch: readers fall through to "all events visible" (defensive).
- Switching branches mid-flight: each `read_recent_dialogue` call re-queries `active_branch`, so it's always current. No caching.
**Tests:** 5+ minimum.
1. `test_read_recent_dialogue_respects_active_branch_head` — seed 10 events; active branch head = 5; assert only first 5 returned.
2. `test_search_memories_respects_active_branch_head` — same.
3. `test_branch_switch_changes_visible_events` — switch branches; immediately read; assert different result sets.
4. `test_main_branch_with_head_zero_returns_empty` — defensive.
5. `test_no_active_branch_falls_through_to_all_events` — defensive.
**Commit:** `feat: branching read-side filter — event readers consult active branch range (T113)`.
**This is the largest task in Phase 4.5.** Estimate 200-400 lines across multiple files. Implementer should split commits if it helps clarity (one per affected reader).
---
## Wave 7 — Lifecycle rollback in regenerate (single)
### Task 114: Lifecycle rollback
**Files:**
- Modify: `chat/services/regenerate.py`
- Modify: `chat/db/migrations/0014_phase45_schema.sql` (T109's migration) — add column? OR
- Add new migration — see decision below
- Modify: tests in `tests/test_regenerate.py`
**Spec:**
Phase 3.5 T83.4 shipped a warning log when regenerate detects un-rolled-back lifecycle transitions. T114 implements actual rollback.
**Schema decision:**
Option A: extend lifecycle event payloads with `triggered_by_assistant_turn_id` (no schema change needed — just a payload convention). Production code (T61 turn flow) populates it when emitting `event_started`/`event_completed`/`event_cancelled`. Existing rows have NULL — rollback skips them with a debug log.
Option B: add a column to `event_log` for stronger invariants. Significant migration cost.
**Recommended:** Option A. Safer, no migration, backward compatible (older events skip rollback). Document in commit body.
**Rollback semantics:**
When regenerate detects lifecycle events triggered by the superseded turn:
- `event_started` → emit `event_cancelled` (or a NEW `event_started_undone` event kind that reverts status to "planned") with the same event_id.
- `event_completed` → emit `event_uncompleted` (NEW event kind that reverts status from "completed" to "active").
- `event_cancelled` → emit `event_uncancelled` (reverts to prior status — which we'd need to track; or simpler: emit `event_started` again to restore "active").
**Simpler approach (recommended):** add ONE new event kind `event_status_reverted` with payload `{event_id, prior_status}`. The projector sets `events.status = prior_status` for the event_id. Rollback emits this event for each affected lifecycle transition, looking up the prior status from the row's history (via event_log scan) or accepting it as a payload field.
**Production code change:** in `chat/web/turns.py::post_turn` (and `chat/services/regenerate.py`), when emitting `event_started`/`event_completed`/`event_cancelled`, populate `triggered_by_assistant_turn_id: <id>` in the payload. Forward-only — older code doesn't need updating.
**Tests:** 3 minimum.
1. `test_regenerate_rolls_back_event_started_from_superseded_turn` — seed an event; play a turn that starts it; regenerate; assert `event_status_reverted` event landed with `prior_status="planned"` and the events row is back to "planned".
2. `test_regenerate_rolls_back_event_completed_to_active` — same but completed → active rollback.
3. `test_regenerate_skips_events_without_back_reference` — older events without `triggered_by_assistant_turn_id` are not rolled back (debug log). Pin the backward-compat behavior.
**Commits:**
- `feat: lifecycle events carry triggered_by_assistant_turn_id back-reference (T114.1)`
- `feat: event_status_reverted event kind + projector handler (T114.2)`
- `feat: regenerate rolls back lifecycle transitions on supersede (T114.3)`
---
## Wave 8 — sqlite-vec swap (single, ENVIRONMENTAL)
### Task 115: sqlite-vec swap (optional)
**Files:**
- Create: `chat/db/migrations/0015_vec0_virtual_tables.sql`
- Modify: `chat/db/connection.py` (load extension on every connection)
- Modify: `chat/services/vector_search.py` (rewrite to use vec0 MATCH instead of pure-Python cosine)
- Modify: `chat/state/embeddings.py` (writer needs to populate vec0 table)
- Modify: `pyproject.toml` (add `sqlite-vec` dependency)
**Pre-flight:**
This task REQUIRES one of:
- Python rebuilt with `--enable-loadable-sqlite-extensions` (pyenv reinstall).
- `apsw` migration of `chat/db/connection.py`.
If neither is feasible at the time of execution: SKIP THIS TASK and document the deferral in T118 docs sweep. The other 13 Phase 4.5 tasks ship without it.
**Spec:**
1. **Migration** `0015_vec0_virtual_tables.sql`:
```sql
CREATE VIRTUAL TABLE embeddings_vec USING vec0(
memory_id INTEGER PRIMARY KEY,
embedding FLOAT[384]
);
-- Backfill from existing JSON embeddings table.
INSERT INTO embeddings_vec (memory_id, embedding)
SELECT memory_id, vec_f32(vector_json) FROM embeddings;
```
2. **`chat/db/connection.py`** loads `sqlite_vec` extension on every connection:
```python
import sqlite_vec
def open_db(...):
conn = sqlite3.connect(...)
conn.enable_load_extension(True)
sqlite_vec.load(conn)
conn.enable_load_extension(False)
...
```
3. **Rewrite `vector_search.py`** to use `embeddings_vec MATCH ?` syntax with `k=?` clause:
```sql
SELECT m.id, m.pov_summary, m.significance, e.distance
FROM embeddings_vec e
JOIN memories m ON m.id = e.memory_id
WHERE e.embedding MATCH ? AND k = ?
AND m.owner_id = ?
AND m.witness_<role> = 1
ORDER BY e.distance ASC
LIMIT ?
```
4. **HNSW note**: vec0 supports both flat (default) and HNSW indexes. T115 ships flat (sufficient for < few thousand memories). Document HNSW upgrade path in CLAUDE.md if memory counts ever grow past pure-Python feasibility.
5. **Old `embeddings` JSON table**: keep alongside `embeddings_vec` (data redundancy is fine; the JSON table is the source of truth and `embeddings_vec` is the index). Backfill on migration. Keep the `embedding_indexed` projector populating both.
**Tests:** rewrite `tests/test_vector_search.py` to expect new behavior. Same observable contract — only implementation changes. All 5 existing tests should pass post-swap.
**Commit:** `feat: sqlite-vec swap (vec0 virtual tables + MATCH-based search) (T115)`.
---
## Wave 9 — Polish (parallel, 3 tasks)
### Task 116: Structured test-fixture builder
**Files:**
- Create: `tests/fixtures.py` (or extend `tests/conftest.py`)
- Modify: existing test files that use brittle canned-queue arrays (selectively)
**Spec:**
Phase 3 carry-over. Tests across `test_turn_flow.py`, `test_meanwhile_turn_flow.py`, `test_phase3_integration.py`, `test_phase4_integration.py` use positional canned-response arrays for `MockLLMClient`. Adding a new classifier call to a code path requires updating canned arrays in many tests.
**Solution:** structured fixture builder that lets tests declare their classifier expectations by name, not position:
```python
# tests/fixtures.py
class CannedQueue:
def __init__(self):
self._queue = []
def parse_turn(self, **fields): ...
def state_update(self, **fields): ...
def detect_scene_close(self, should_close: bool): ...
def detect_event_transitions(self, transitions: list[dict]): ...
def summarize_scene(self, summary: str, **fields): ...
def detect_threads(self, candidates: list[dict]): ...
# ... one method per classifier service
def build(self) -> list[str]:
return [json.dumps(item) for item in self._queue]
```
Usage:
```python
def test_post_turn_with_event_transition(...):
canned = (
CannedQueue()
.parse_turn(intent="narrative")
.narrative("BotA speaks.") # narrative is a stream, but for simplicity treat it like a canned response
.state_update(affinity_delta=0, trust_delta=0)
.state_update(affinity_delta=0, trust_delta=0)
.detect_event_transitions([{"event_id": "evt_1", "new_status": "completed"}])
.detect_scene_close(should_close=False)
.build()
)
mock = MockLLMClient(canned=canned)
# ...
```
**Migration scope:** don't migrate ALL existing tests at once — that's a separate massive refactor. Instead, ship the fixture builder + migrate 2-3 representative tests as proof of concept. Document the migration path in the fixture's docstring.
**Tests:** the fixture builder itself doesn't need extensive testing — it's just a builder. Add 1-2 sanity tests that the JSON output matches expected shapes.
**Commit:** `test: structured CannedQueue fixture builder for classifier mocks (T116)`.
---
### Task 117: Phase 4.5 cross-feature integration tests
**Files:**
- Create: `tests/test_phase45_integration.py`
**Spec:**
End-to-end multi-feature flows specific to Phase 4.5 changes. 5 tests minimum.
1. **Real embedding swap + retrieval** — configure `embedding_model="bge-small-en-v1.5"` (mocked); write a memory; backfill or wait for worker; assert vector search returns the memory via `client.embed`-derived vector (not pseudo).
2. **Branching read-side filter end-to-end** — create a branch from turn 5; switch; play 3 turns on the branch; switch back to main; assert main's recent dialogue is missing the branch turns (read filter respects active branch's head).
3. **Lifecycle rollback** — start an event via a turn; regenerate that turn; assert lifecycle reverted (event back to "planned").
4. **Search deep-link** — write memories; search; click a result; verify the chat page renders with the right turn anchored (assert via TestClient response — either the browser anchor OR a server-side scroll-to-anchor mechanism).
5. **Bulk significance re-rate end-to-end** — seed 5 memories at significance 0; bulk re-rate via drawer; verify significance histogram updates.
**Commit:** `test: phase 4.5 cross-feature integration coverage (T117)`.
---
### Task 118: Phase 4.5 documentation update
**Files:**
- Modify: `CLAUDE.md`
- Modify: `docs/plans/2026-04-26-v1-requirements-design.md` (annotate §13 Phase 4 entries — though they're already shipped per Phase 4 T102)
**Spec:**
Mirror the Phase 3.5 / 2.5 status sections. Document:
- All shipped items per task (T103T117).
- Empty out the Phase 4.5 / 5 backlog (replace with single "All items shipped" line).
- Add new "Phase 5 backlog" section if any Phase 4.5 reviews surfaced new follow-ups.
**Phase 5 backlog candidates** (default, if no new follow-ups discovered):
- Vector index optimization (HNSW) when memory counts grow past flat-index feasibility.
- Branch-isolated event_log (each branch has its own physical event_log range vs the current shared id space + head filter).
- Embedding model swap migration tooling — when changing models, need to re-embed everything; T112 added `--re-embed-all` but a more orchestrated swap (drain old worker, re-seed all memories, swap config) is Phase 5+.
- Real-time collaborative branching (multi-user) — out of scope for v1.
- Avatars / portraits (multimodality) — deferred indefinitely per design §14.
**Commit:** `docs: phase 4.5 status, prune backlog, capture phase 5 candidates (T118)`.
---
## Wrap-up
After Wave 9 lands:
1. **Run full suite** on `phase-4.5`: should be ~430+ tests passing (413 from Phase 4 + ~20 new across Phase 4.5).
2. **Manual smoke** (recommended before opening the PR):
- Configure `embedding_model="bge-small-en-v1.5"` (or whatever real model is chosen); restart server; play a turn; verify `embedding_indexed` events use the real model and search returns semantically-relevant memories.
- Create a branch, switch, play turns, switch back — verify main's history is unaffected.
- Plan an event, complete it via a turn, regenerate that turn — verify event reverts to "planned".
- Use cross-chat search; click a result; verify it lands on the right turn in the chat page.
- Bulk re-rate a chat's significance distribution.
3. **Push `phase-4.5`** to gitea.
4. **Open PR** `phase-4.5 → main`.
---
## Notes for the controller running this plan
- **T115 (sqlite-vec swap)** is environmental. If pre-flight fails (no rebuilt Python, no apsw), defer to Phase 5 and ship Phase 4.5 with 13 tasks. T118 docs sweep should note the deferral.
- **T112 (real embedding swap)** assumes Featherless or similar exposes an `/v1/embeddings` endpoint. If not available, document the gap and ship the Protocol + Mock impl only (Featherless impl deferred). The pseudo path remains the default in that case — same as Phase 4.
- **T113 (branching read-side filter)** is the riskiest task. Cross-cutting. Land it on a quiet branch, test thoroughly. If integration tests break in unexpected ways, bisect the affected reader and add coverage.
- **After each parallel wave**, run a code-review subagent. Combined spec+quality acceptable for trivial tasks (T103T108); separate spec + quality reviewers for big tasks (T112, T113, T114, T115).
- **Token-spend rough estimate**: Phase 4.5 should be ~50% the size of Phase 4 (similar number of tasks, mostly smaller). Big tasks (T112, T113, T114) bring the per-task spend up but parallelism in Wave 1 + Wave 9 brings the wall-clock down.
- **DO NOT break existing v1/v2/v3/v3.5/v4 surface contracts.** Every test file that was green at the start of Phase 4.5 must stay green at the end. The cross-feature integration tests (`tests/test_phase4_integration.py`, `tests/test_phase3_integration.py`) are particularly load-bearing.
@@ -0,0 +1,23 @@
{
"planPath": "docs/plans/2026-04-27-v4.5-phase4.5-cleanup.md",
"tasks": [
{"id": 103, "subject": "T103: branches polish (global-leak doc + branch-switch warning)", "status": "pending", "wave": 1, "parallelGroup": "wave-1"},
{"id": 104, "subject": "T104: state/memory.py polish (DRY MAX(id) + fts_rank doc)", "status": "pending", "wave": 1, "parallelGroup": "wave-1"},
{"id": 105, "subject": "T105: snapshots.py polish (datetime hoist + kind validation + mtime doc)", "status": "pending", "wave": 1, "parallelGroup": "wave-1"},
{"id": 106, "subject": "T106: search.py polish (k constant + N+1 batched lookups)", "status": "pending", "wave": 1, "parallelGroup": "wave-1"},
{"id": 107, "subject": "T107: embeddings.py timeout_s fallback-path logging", "status": "pending", "wave": 1, "parallelGroup": "wave-1"},
{"id": 108, "subject": "T108: scene-close-on-cancel UX revisit (regression test pin + rationale doc)", "status": "pending", "wave": 1, "parallelGroup": "wave-1"},
{"id": 109, "subject": "T109: 0014 schema migration (FK CASCADE + memories.event_id column)", "status": "pending", "wave": 2, "parallelGroup": null},
{"id": 110, "subject": "T110: drawer Phase 4.5 bundle (event_id guard + html.escape + modal partial + bulk sig re-rate)", "status": "pending", "wave": 3, "parallelGroup": null, "blockedBy": [109]},
{"id": 111, "subject": "T111: search UX (FTS snippet highlighting + deep-link via memories.event_id)", "status": "pending", "wave": 4, "parallelGroup": null, "blockedBy": [109]},
{"id": 112, "subject": "T112: real embedding model swap (LLMClient.embed protocol + Featherless impl + routing)", "status": "pending", "wave": 5, "parallelGroup": null},
{"id": 113, "subject": "T113: branching read-side filter (event readers consult is_active branch range)", "status": "pending", "wave": 6, "parallelGroup": null, "blockedBy": [109]},
{"id": 114, "subject": "T114: regenerate lifecycle rollback (back-reference + event_status_reverted)", "status": "pending", "wave": 7, "parallelGroup": null},
{"id": 115, "subject": "T115: sqlite-vec swap (vec0 virtual tables + MATCH search) [ENVIRONMENTAL — may defer]", "status": "pending", "wave": 8, "parallelGroup": null},
{"id": 116, "subject": "T116: structured CannedQueue test fixture builder", "status": "pending", "wave": 9, "parallelGroup": "wave-9"},
{"id": 117, "subject": "T117: phase 4.5 cross-feature integration tests", "status": "pending", "wave": 9, "parallelGroup": "wave-9", "blockedBy": [110, 111, 112, 113, 114]},
{"id": 118, "subject": "T118: phase 4.5 docs sweep — prune backlog, capture phase 5 candidates", "status": "pending", "wave": 9, "parallelGroup": "wave-9", "blockedBy": [110, 111, 112, 113, 114]}
],
"lastUpdated": "2026-04-27T00:00:00Z",
"notes": "16 tasks across 9 waves consolidating all 24 items in CLAUDE.md Phase 4.5/5 backlog. Wave 1 (6-way parallel) and Wave 9 (3-way parallel) maximize parallelism. Waves 2-8 are single-task by hot-file constraint. T115 (sqlite-vec swap) requires Python rebuild OR apsw migration — environmental; may defer to Phase 5. Schema baseline 13 -> 14 (T109's 0014) -> optionally 15 (T115's 0015). Big tasks: T112 (real embedding swap), T113 (branching read-side filter — riskiest), T114 (lifecycle rollback). Uses task ids T103-T118."
}
+35
View File
@@ -1,5 +1,7 @@
from __future__ import annotations from __future__ import annotations
import logging
from chat.db.connection import open_db from chat.db.connection import open_db
from chat.db.migrate import apply_migrations from chat.db.migrate import apply_migrations
from chat.eventlog.log import append_event from chat.eventlog.log import append_event
@@ -139,3 +141,36 @@ def test_list_branches_returns_all(tmp_path):
names = [b["name"] for b in list_branches(conn)] names = [b["name"] for b in list_branches(conn)]
assert "main" in names assert "main" in names
assert "experiment" in names assert "experiment" in names
def test_branch_switched_unknown_name_warns(tmp_path, caplog):
"""Switching to a nonexistent branch logs a warning and leaves no branch active.
The previous behavior silently cleared is_active flags and applied no UPDATE
when the named branch did not exist. T103 makes that condition observable
by emitting a warning while preserving the existing (zero-active) outcome.
"""
db = tmp_path / "t.db"
apply_migrations(db)
with open_db(db) as conn:
with caplog.at_level(logging.WARNING, logger="chat.state.branches"):
append_event(
conn,
kind="branch_switched",
payload={"name": "does_not_exist"},
)
project(conn)
# A warning was emitted naming the missing branch.
warnings = [
r for r in caplog.records
if r.levelno == logging.WARNING and r.name == "chat.state.branches"
]
assert warnings, "expected a warning for unknown branch name"
assert any("does_not_exist" in r.getMessage() for r in warnings)
# Existing behavior preserved: no branch is active after the switch.
assert active_branch(conn) is None
# The unknown name was not inserted as a side effect.
assert get_branch(conn, "does_not_exist") is None
+177
View File
@@ -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" 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 (&lt;script&gt;) is what we want to see instead.
assert "<script>alert" not in body
assert "&lt;script&gt;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). # T98.5 — remaining v1 edits (chat narrative anchor + weather).
# --------------------------------------------------------------------------- # ---------------------------------------------------------------------------
+31
View File
@@ -20,6 +20,7 @@ The pseudo path doesn't touch the LLMClient, so we pass an empty
from __future__ import annotations from __future__ import annotations
import logging
import math import math
import pytest import pytest
@@ -89,3 +90,33 @@ async def test_generate_embedding_unit_normalized():
result = await generate_embedding(_client(), text="some non-empty text") result = await generate_embedding(_client(), text="some non-empty text")
norm_sq = sum(x * x for x in result.vector) norm_sq = sum(x * x for x in result.vector)
assert math.isclose(norm_sq, 1.0, abs_tol=1e-6) assert math.isclose(norm_sq, 1.0, abs_tol=1e-6)
@pytest.mark.asyncio
async def test_generate_embedding_non_default_model_logs_warning(caplog):
"""T107: non-default model falls through to fallback and must warn.
A Phase 4.5+ caller pointing at a real model that isn't yet wired
up would otherwise silently degrade (zero vector → useless cosine).
The warning surfaces the misconfiguration in logs.
"""
caplog.set_level(logging.WARNING, logger="chat.services.embeddings")
result = await generate_embedding(_client(), text="hello", model="real-model")
# Behavior unchanged: still returns the fallback sentinel.
assert result.model == FALLBACK_EMBEDDING_MODEL == "fallback"
assert all(x == 0.0 for x in result.vector)
# Warning fired and names the offending model.
warnings = [r for r in caplog.records if r.levelno == logging.WARNING]
assert any("non-default model" in r.getMessage() for r in warnings)
assert any("real-model" in r.getMessage() for r in warnings)
@pytest.mark.asyncio
async def test_generate_embedding_default_model_does_not_warn(caplog):
"""T107: the silent default path must stay silent."""
caplog.set_level(logging.WARNING, logger="chat.services.embeddings")
await generate_embedding(_client(), text="hello")
warnings = [r for r in caplog.records if r.levelno == logging.WARNING]
assert warnings == []
+56
View File
@@ -586,3 +586,59 @@ def test_record_turn_memory_enqueues_embedding_job(tmp_path):
assert {job.memory_id for job in captured} == expected_ids assert {job.memory_id for job in captured} == expected_ids
for job in captured: for job in captured:
assert job.text == "Both bots witness this beat." assert job.text == "Both bots witness this beat."
# ---------------------------------------------------------------------------
# T109: memories.event_id deep-link column populated by the projector.
# ---------------------------------------------------------------------------
def test_memory_written_populates_event_id(tmp_path):
"""Schema 0014 added ``memories.event_id`` referencing ``event_log.id``.
The ``memory_written`` projector handler must populate the column with
the projecting event's id so T111 can deep-link cross-chat search hits
back to the originating turn.
"""
db = tmp_path / "t.db"
apply_migrations(db)
_seed_minimal(db)
with open_db(db) as conn:
result = record_turn_memory_for_present(
conn,
chat_id="chat_bot_a",
host_bot_id="bot_a",
guest_bot_id=None,
narrative_text="BotA shrugs.",
)
eid, mid = result["bot_a"]
assert eid > 0 and mid is not None
row = conn.execute(
"SELECT event_id FROM memories WHERE id = ?", (mid,)
).fetchone()
assert row is not None
assert row[0] == eid
def test_memory_event_id_column_is_nullable_for_backfill(tmp_path):
"""Backward compat: the ``event_id`` column is nullable so historical
memory rows projected before 0014 ran (or rows synthesised by tests
that bypass the projector) don't break the schema. A direct INSERT
omitting the column must succeed and read back NULL."""
db = tmp_path / "t.db"
apply_migrations(db)
_seed_minimal(db)
with open_db(db) as conn:
conn.execute(
"INSERT INTO memories ("
"owner_id, chat_id, pov_summary, "
"witness_you, witness_host, witness_guest"
") VALUES (?, ?, ?, ?, ?, ?)",
("bot_a", "chat_bot_a", "legacy row", 1, 1, 0),
)
row = conn.execute(
"SELECT event_id FROM memories WHERE pov_summary = 'legacy row'"
).fetchone()
assert row is not None
assert row[0] is None
+28
View File
@@ -16,6 +16,7 @@ Verifies the FastAPI ``/search`` route that wraps T93's
from __future__ import annotations from __future__ import annotations
from pathlib import Path from pathlib import Path
from unittest.mock import patch
import pytest import pytest
from fastapi.testclient import TestClient from fastapi.testclient import TestClient
@@ -133,3 +134,30 @@ def test_result_links_navigate_to_chat(client, tmp_path):
# The link target is chat-level (memories don't carry an event_id # The link target is chat-level (memories don't carry an event_id
# column today, so we don't deep-link to a specific turn). # column today, so we don't deep-link to a specific turn).
assert 'href="/chats/chat_a"' in resp.text assert 'href="/chats/chat_a"' in resp.text
def test_search_results_use_batched_lookups(client, tmp_path):
"""T106: hydration must not fan out to per-row ``get_bot``/
``get_chat``/``get_scene`` calls.
The previous implementation called each helper once per result row
(worst case 50 rows x 3 helpers = 150 individual queries). The
batched implementation collects distinct ids and issues at most one
query per entity kind via ``WHERE id IN (...)``, so the per-row
helpers should not be invoked at all when there are matches.
We seed two chats (so both ``get_bot`` and ``get_chat`` would have
been hit pre-T106) and assert each helper sees zero per-row calls.
"""
_seed_two_chats_with_memories(tmp_path / "test.db")
with (
patch("chat.web.search.get_bot") as mock_get_bot,
patch("chat.web.search.get_chat") as mock_get_chat,
patch("chat.web.search.get_scene") as mock_get_scene,
):
resp = client.get("/search?q=rabbit")
assert resp.status_code == 200
# Batched IN-list queries replace the per-row helpers entirely.
assert mock_get_bot.call_count == 0
assert mock_get_chat.call_count == 0
assert mock_get_scene.call_count == 0
+22
View File
@@ -156,6 +156,28 @@ def test_restore_snapshot_wrong_confirm_400(client, tmp_path):
assert response.status_code == 400 assert response.status_code == 400
def test_restore_without_kind_returns_400(client, tmp_path):
"""T105: Missing or empty ``kind`` must be rejected with 400.
Previously ``kind`` defaulted to ``"periodic"``, which silently 404'd
when the caller meant a rewind snapshot. Tighten the contract so the
client must always pass an explicit, valid ``kind``.
"""
db_path = tmp_path / "test.db"
_seed_bot(db_path, "bot_a", "BotA")
snapshot_path = _take_snapshot_via_service(
db_path, tmp_path, kind="periodic"
)
snapshot_id = snapshot_path.stem
response = client.post(
f"/snapshots/restore/{snapshot_id}",
data={"confirm_id": snapshot_id}, # no `kind`
follow_redirects=False,
)
assert response.status_code == 400
def test_preview_renders_metadata(client, tmp_path): def test_preview_renders_metadata(client, tmp_path):
db_path = tmp_path / "test.db" db_path = tmp_path / "test.db"
_seed_bot(db_path, "bot_a", "BotA") _seed_bot(db_path, "bot_a", "BotA")
+34
View File
@@ -734,6 +734,19 @@ def test_cancelled_turn_still_closes_scene_when_user_prose_signals_close(
that as an exception, so we drive the request inside ``with that as an exception, so we drive the request inside ``with
pytest.raises``. Despite the exception, the scene_closed event pytest.raises``. Despite the exception, the scene_closed event
must land in the event_log. must land in the event_log.
T108 NOTE this test does NOT actually exercise the cancel path.
``_CancelOnStreamMock.stream`` writes ``raise asyncio.CancelledError``
but ``asyncio`` is not imported at module scope, so the first
iteration raises ``NameError`` (caught by ``except Exception:`` in
post_turn, which sets ``primary_truncated=True`` but leaves
``cancelled=False``). The function therefore returns 204 normally,
the dependency-managed connection commits, and ``scene_closed``
lands. Importing asyncio so the real CancelledError fires reveals
a transactional bug: ``post_turn``'s end-of-function re-raise
causes ``open_db``'s dependency teardown to skip ``conn.commit()``,
rolling back ALL post-cancel writes (user_turn, assistant_turn,
edge_updates, scene_closed). Deferred for triage see T108 report.
""" """
from typing import AsyncIterator, Sequence from typing import AsyncIterator, Sequence
@@ -828,12 +841,33 @@ def test_cancelled_turn_still_closes_scene_when_user_prose_signals_close(
"SELECT payload_json FROM event_log " "SELECT payload_json FROM event_log "
"WHERE kind = 'assistant_turn' ORDER BY id" "WHERE kind = 'assistant_turn' ORDER BY id"
).fetchall() ).fetchall()
# T108: pin the ordering — user_turn must commit before
# scene_closed (close detection runs on prose that is already
# in the event_log) and any assistant_turn the cancel produced
# must come last (truncated record written after both).
ordered = conn.execute(
"SELECT id, kind FROM event_log "
"WHERE kind IN ('user_turn', 'scene_closed', 'assistant_turn') "
"ORDER BY id"
).fetchall()
# Scene close lands despite the cancel. # Scene close lands despite the cancel.
assert scene_close_count == 1 assert scene_close_count == 1
# The cancelled assistant_turn was still recorded (truncated=True). # The cancelled assistant_turn was still recorded (truncated=True).
assert len(assistant_payload) == 1 assert len(assistant_payload) == 1
assert json.loads(assistant_payload[0][0])["truncated"] is True assert json.loads(assistant_payload[0][0])["truncated"] is True
# T108 ordering pin: user_turn lands first, the truncated
# assistant_turn (if any) is committed BEFORE the scene_close
# decision fires, and scene_closed lands last. Close detection
# relies on user prose being committed to the event_log BEFORE
# the close decision runs — and the cancelled assistant beat is
# recorded as a partial before close-detection too.
kinds_in_order = [row[1] for row in ordered]
user_idx = kinds_in_order.index("user_turn")
close_idx = kinds_in_order.index("scene_closed")
assert user_idx < close_idx
if "assistant_turn" in kinds_in_order:
assert user_idx < kinds_in_order.index("assistant_turn") < close_idx
def test_interjection_enqueues_significance_job(app_state_setup, tmp_path): def test_interjection_enqueues_significance_job(app_state_setup, tmp_path):
+2 -2
View File
@@ -324,11 +324,11 @@ def test_get_scene_returns_none_for_missing(tmp_path):
assert active_scene(conn, "chat_missing") is None assert active_scene(conn, "chat_missing") is None
def test_schema_version_after_migration_is_13(tmp_path): def test_schema_version_after_migration_is_14(tmp_path):
db = tmp_path / "t.db" db = tmp_path / "t.db"
apply_migrations(db) apply_migrations(db)
with open_db(db) as conn: with open_db(db) as conn:
row = conn.execute( row = conn.execute(
"SELECT value FROM meta WHERE key = 'schema_version'" "SELECT value FROM meta WHERE key = 'schema_version'"
).fetchone() ).fetchone()
assert int(row[0]) == 13 assert int(row[0]) == 14