3 Commits

Author	SHA1	Message	Date
Patrick Buckley	39e0f930c1	fix(metacog): factor sanitiser regex tail + trim docstrings + drop tombstone ... Closes round-2 review findings q-3, q-4, q-5, q-7. * **q-4:** ``_NAME_CONTROL_CHARS`` and ``_PAYLOAD_CONTROL_CHARS`` shared 7 lines of Unicode-steering character classes (zero-width / bidi / separators / BOM / tag chars above BMP). Factored into a single ``_CONTROL_CHARS_TAIL`` constant; each regex now differs only in its leading ASCII range. Future bidi or zero-width additions edit one place. Side effect: this corrects a latent bug where ``_NAME_CONTROL_CHARS`` had two literal ASCII spaces in place of U+2028 / U+2029 (line and paragraph separators) — visible as ``r" "`` in source but rendered as the actual codepoints in ``_PAYLOAD_CONTROL_CHARS``. After the factoring both regexes correctly include U+2028 / U+2029, closing the gap that would have let a workstream name with embedded line separators forge a sibling bullet (the same vector ``\n`` was blocked for in the original bug-1 fix). Switched to ``\u`` escapes for readability (and to keep future Edit tool runs against this block reliable). * **q-3:** Tombstone clause "standing in for the deleted ``_watch_pending`` maxsize bound" survived in ``ChatSession.set_watch_runner``'s docstring after the apply-pass trim cleaned the inline soft-cap comment. Dropped. * **q-5:** ``test_newline_in_name_does_not_forge_extra_bullet`` carried five WHAT-narration comments restating what the immediately-following asserts already say. Dropped — the docstring carries the security invariant; the assertions speak for themselves. * **q-7:** ``patch_session_storage`` had a 14-line docstring including fallback-guidance and self-justification ("accumulated 7 near-duplicate sites"). Trimmed to a 3-line contract.	2026-05-06 16:16:57 -07:00
Patrick Buckley	20c4dfaca6	fix(metacog): tighten concurrency bound + lift storage-patch helper ... Closes review findings bug-4 and q-6. bug-4 — the watch dispatch concurrency test bounded depth at ``_WATCH_QUEUE_SOFT_CAP + 2 * per_thread`` (= 250) which is tautologically true: two threads × 100 fires can append at most 200 entries above the cap, so the bound asserted nothing more than what ``depth <= 2 * per_thread`` already says. Tighten to ``_WATCH_QUEUE_SOFT_CAP + N_THREADS`` (= 52): the count-then-drop window admits at most one slip per concurrent thread. q-6 — 7 near-duplicate ``monkeypatch.setattr(session_mod, "get_storage", lambda: _StubStorage())`` sites across ``test_watch_dispatch.py`` + ``test_watch_integration.py`` (4 different stub shapes, mostly trivial variations on the active flag). Lift a ``patch_session_storage`` helper into the existing ``tests/_helpers.py`` with kwargs for the common cases (``active``, ``raise_on_is_active``), returns the call list so call-shape assertions still work. Tests collapse from ~10-line inline-class blocks to one-line helper calls.	2026-05-06 16:16:57 -07:00
Patrick Buckley	89b6b299f7	fix(memory): query-aware candidate selection + OR-of-terms search (#468) ... * fix(memory): query-aware candidate selection + OR-of-terms search The system-message memory composition path used a recency-ordered candidate set (`_list_visible_memories(limit=fetch_limit)`). On deployments with more than `fetch_limit` (default 50) visible memories, BM25 only ever ranked the 50 most-recently-touched memories — a relevant memory written months ago was silently invisible regardless of how well it matched the recent context. Multi-word search at the SQL layer used AND-of-terms, killing recall on any multi-word query without an exact field overlap. ## Functional changes - `_init_system_messages` (`turnstone/core/session.py`): extract recent context first, then `_search_visible_memories(context)` to pull query-aware candidates. Search hits below `fetch_limit` union with the recency list (deduped by memory_id) so the BM25 candidate pool is always a SUPERSET of the prior recency-only pool — even on noisy queries where the cap fills with stopwords, the recency-50 the original bug surfaced still reaches BM25. Empty context skips search entirely. Candidate-selection logic extracted into `_select_memory_candidates`. - `search_structured_memories` (PostgreSQL + SQLite): per-term clauses join with OR instead of AND. A row matches if ANY term matches ANY of name/description/content. Downstream BM25 narrows back down by relevance. ## Perf hardening - Collapse the 1-3 fanned scope queries into a single SQL. New backend methods `list_visible_structured_memories` / `search_visible_structured_memories` union the visibility scopes into one WHERE OR-group, so a composition rebuild now hits the DB at most twice (search + recency) instead of up to six times. - Cap and normalize search terms. Composition can hand a multi-KB pasted message to ILIKE-based search; without a cap, every distinct token would emit one unindexable predicate per scope-fanned query. `normalize_search_terms` (`storage/_utils.py`) de-dupes case-insensitively, drops <2-char tokens, and hard-caps at 16. - Per-turn search cache. `_init_system_messages` fires from many call sites within one turn (state transitions, MCP refresh, tool results) and the recent-context query is identical across them. Session-instance cache keyed by (query, mem_type, limit) absorbs the duplicates; invalidated in `_append_user_turn` and after memory save/delete tool actions. - Stable secondary sort by `memory_id`. `updated` is second-precision and `touch_structured_memories` can land a batch on identical timestamps; without a tie-breaker SQL returns rows in implementation-defined order, BM25 input shuffles, and the LLM-side prompt cache misses across calls. All four backend ORDER BYs now break ties on `memory_id ASC`. ## Quality cleanups - Coalesce `memory.search.term_count` + `memory.search.zero_results` into a single `memory.search` log carrying both `term_count` and `result_count`. - New `memory.composition` log: source / candidates / injected. - Promote a shared `make_chat_session` factory to `tests/_helpers.py`. - Rename SQL builder local `extra` -> `scope_filters` for clarity. - Add docstrings on `search_structured_memories` so the AND->OR flip survives future readers. ## Tests Adds 20 tests across `tests/test_structured_memory.py`, `tests/test_structured_memory_storage.py`, and `tests/test_memory_relevance.py`: recency-ceiling regression, empty-query fallback, sparse-match union, recency-preserved-when- search-returns-noise (locks in the pool-superset invariant), OR-of-terms on both backends, scope filtering preserved, search-facade multi-word behavior, term-cap normalization, the new visible-scope helpers (list + search + empty-scopes guard), coord-scope composition isolation, end-to-end `memory(action='search')` tool execution, per-turn cache hit + invalidation, and stable ordering under tied `updated` timestamps. Memory test sweep: 102/102. Broader regression (session, storage, coordinator, load_skill): 411/411. * fix(memory): address Copilot review on PR #468 Three follow-ups from Copilot's inline review: 1. SUPERSET invariant violation (Copilot, session.py:5510). `(search_hits + extra)[:fetch_limit]` capped the union back down to fetch_limit, evicting the recency tail when search added distinct hits. Recency tail is exactly where ancient-but-recently-touched memories live — the recall this PR is supposed to improve — so tail eviction recreated the bug for the narrow case where a query term fell off the 16-cap and the matching memory sat in recency[40-49]. Drop the cap; both halves are already SQL-capped at fetch_limit, so the union is at most 2 × fetch_limit (~100 with defaults). BM25 over 100 candidates in pure Python is sub-ms; irrelevant recency fillers get score=0 and don't pollute ranking. Updates the docstring to actually be honest about the invariant. Adds `test_recency_tail_preserved_when_search_adds_distinct_hits` that locks the behavior in: 5 search hits + 10 recency = 15-item pool, every recency item present, source="union". 2. Unbounded `query.split()` in normalize_search_terms (Copilot, _utils.py:74). `str.split()` allocates the full token list before the cap-after-16 break, so a 100KB pasted query did MB of throwaway work even though only 16 tokens entered SQL. Switch to `re.finditer(r'\S+', query)` — streaming iterator, stops scanning at the first 16 normalized terms regardless of input size. 3. Misleading + unbounded log term_count (Copilot, session.py:8571). `len(item["query"].split())` had two problems: same unbounded split as #2, and the value reported the raw input token count rather than the normalized term count that actually hit the SQL WHERE clause — misleading metric for an operator trying to understand storage-side behavior. Switch to `len(normalize_search_terms(item["query"]))` — accurate count, and bounded for free via #2. Refuted: github-code-quality flagged `...` bodies in the new Protocol methods as "statement has no effect." False positive — `...` is the canonical Protocol body convention, used 213 other times in the same file. Memory test sweep: 103/103. Broader regression: 411/411.	2026-05-02 23:52:49 -07:00