Compare commits
14 Commits
phase-4
...
3f1a284acb
| Author | SHA1 | Date | |
|---|---|---|---|
| 3f1a284acb | |||
| 87f93f00b5 | |||
| d1e2902655 | |||
| 54dfa8d611 | |||
| 5d36d3456f | |||
| 0e9421dcf7 | |||
| baffeb3a44 | |||
| 29b7c90b29 | |||
| 64c9ca634a | |||
| 374a76c867 | |||
| b65e1e1098 | |||
| 996a16cfb5 | |||
| a06f90a164 | |||
| df977fc985 |
@@ -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
|
||||||
)
|
)
|
||||||
|
|||||||
@@ -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, "
|
||||||
|
|||||||
+29
-8
@@ -112,6 +112,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 +182,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 +254,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 +367,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] = []
|
||||||
|
|||||||
+133
-9
@@ -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
@@ -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())
|
||||||
|
|||||||
@@ -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 (T103–T108); 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 2–8 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 (T103–T117).
|
||||||
|
- 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 (T103–T108); 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."
|
||||||
|
}
|
||||||
@@ -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
|
||||||
|
|||||||
@@ -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 == []
|
||||||
|
|||||||
@@ -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
|
||||||
|
|||||||
@@ -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")
|
||||||
|
|||||||
@@ -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):
|
||||||
|
|||||||
Reference in New Issue
Block a user