8 Commits

Author	SHA1	Message	Date
Patrick Buckley	3252f3fd95	fix(memory): preserve replay identity and validation	2026-08-11 21:58:21 -07:00
Patrick Buckley	cc84f9d176	fix(memory): harden project scope authorization and consistency	2026-08-11 21:58:21 -07:00
Patrick Buckley	7f1329d3b0	fix(memory): atomic single-statement upsert for memory save/update (#735) ... * fix(memory): atomic single-statement upsert for memory save/update save_structured_memory used "try INSERT -> catch IntegrityError -> SELECT + UPDATE". On PostgreSQL a model saving the same key twice in a turn logged a uq_smem_name_scope violation on the failing INSERT, and the pattern threw + caught an exception on every update. Replace it with one statement: a new StorageBackend.upsert_structured_memory on both backends emitting INSERT ... ON CONFLICT (name, scope, scope_id) DO UPDATE ... RETURNING. It returns (row, was_update) -- the full saved row and whether an existing row was updated -- like Django's update_or_create; was_update is the supplied (fresh) memory_id differing from the returned id. save_structured_memory is a thin wrapper over it. description / mem_type of None mean "leave unset": the column default applies on insert and the stored value is kept on conflict; an explicit value (including "" / "general") overwrites -- so clearing a description or setting type back to "general" now persists, where the prior "if mem_type != 'general'" / "if description" semantics silently dropped it. The memory tool and the memories HTTP endpoint pass None for omitted fields and read effective type/scope from the returned row; the HTTP endpoint returns that row directly (one query, no follow-up SELECT). Removes the now-unused update_structured_memory primitive and its dead STRUCTURED_MEMORY_MUTABLE constant. Adds cross-backend storage tests and a session tool-path test (preserve-on-omit / overwrite-on-explicit), run on PostgreSQL via --storage-backend -- the save-over-existing path was previously SQLite-only. * docs(memory): clarify upsert was_update precondition Lead the upsert_structured_memory docstring with the behavioral contract (callers MUST supply a fresh unique memory_id) rather than the internal id-comparison mechanism, so a future caller can't reuse an existing id and silently get was_update=False on a real update.	2026-06-28 20:24:20 -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
Patrick Buckley	9a30530d41	feat(coord): surface child errors, isolate tool exceptions, add memory tool (#443) ... * feat(coord): surface child errors, isolate tool exceptions, add memory tool Closes four coordinator gaps identified during operator triage: 1. Child workstream errors now surface in inspect/wait. Worker-thread exception text is sanitized (URL userinfo masked, sk-/Bearer/ghp_/ github_pat_/AKIA tokens redacted, capped at 1024 chars) and persisted to workstream_config.last_error before _emit_state("error") fires, so coord polling never sees state=error with a missing cause. The row is cleared on recovery transitions (idle/running) so a once-leaked exception body doesn't outlive the failure. inspect_workstream and wait_for_workstream return last_error for state=error rows; the wait surface prefers it over the assistant-tail walk. 2. Tool exceptions now return as tool_results with sibling-aware guidance. ChatSession._safe_prepare_tool wraps every per-call _prepare_tool invocation; a buggy preparer becomes an error item for that call only — sibling parallel tool_calls keep going, never orphaning the assistant message's tool_calls block. run_one's runtime exception path includes the exception class and a short note that other tool calls in the batch completed independently so the model can recover. 3. Memory tool exposed to coordinator with a coord-only scope. memory.json gains coordinator: true + interactive: true + per-kind kind_variants. Coord sessions see scope enum ["coordinator"] and an orchestration-flavored description; IC sessions see ["global", "workstream", "user"] and the existing flavor. Coord-scope rows are private to the coordinator session (children cannot read or write them), closing the cross-session prompt-injection lane that an adversarially-steered child would otherwise have. Coord visibility is also restricted to coord-scope only — coords no longer see global / workstream / user memories that belong to the user's interactive sessions. 4. Per-call exception isolation in tool batches. _safe_prepare_tool was previously the implicit shield; now it's an explicit method with documented invariants. KeyboardInterrupt / GenerationCancelled re-raise so the cooperative cancel path still works. Other notable changes: - LAST_ERROR_CONFIG_KEY + persist_last_error / clear_last_error / load_last_error / sanitize_error_text moved to turnstone.core.memory (the storage facade hub) — readers in coordinator_client.py import the constant. - Memory scope tuples extracted to module constants _VALID_MEMORY_SCOPES and _IMPLICIT_SCOPE_WALK; seven inline duplicates collapsed. - tools.py grows _apply_kind_variant for the per-kind tool surface; tools without kind_variants pass through unchanged (no spurious deep-copies). - Session adds _coordinator_scope_id, _default_memory_scope, _implicit_scope_walk, and _record_fatal_error chokepoints so the worker-thread fatal path is one site rather than three. - Removed duplicate on_error / on_state_change emits from session_routes.py and coordinator_adapter.py — session.send()'s _record_fatal_error owns the sequence now. Tests: 4742 pass (no live), +30 net since the baseline. Ruff + mypy clean on every modified production file. * fix(coord): redact secrets in tool error paths via output_guard Copilot review flagged two paths where ``str(exc)`` flowed back into the model-facing tool_result without going through the credential- redaction the new fatal-error path applies: - ``ChatSession._safe_prepare_tool``: a preparer-side exception becomes an error item whose ``error`` field embedded the raw exception text. - ``ChatSession._execute_tools.run_one``: a runtime tool exception became an ``Error executing X: <e>`` tool_result, again with the raw exception text. Both now route through ``sanitize_error_text`` (sanitised log line + sanitised tool_result), and ``sanitize_error_text`` itself was refactored to delegate to ``output_guard.redact_credentials`` instead of carrying its own parallel regex catalog — the audit log + post-tool guard already use that pattern set, so the credential definition stays in one place. Also extended ``_RE_CONNECTION_STRING`` in ``output_guard`` to cover ``http(s)://user:pass@host`` so a misconfigured ``OPENAI_BASE_URL`` that lands in an httpx ``ConnectError.__str__`` is redacted by every caller of ``redact_credentials`` (audit details, close-reason persistence, last_error, the two tool error paths). The host (useful for triage) survives; only the password is replaced with the standard ``[REDACTED:password]`` marker. Tests: full suite (4745 pass), ruff + mypy clean. Two new tests pin the redaction behaviour in both tool error paths so a future refactor can't drift back to leaking ``str(exc)`` verbatim.	2026-04-28 12:19:34 -07:00
Patrick Buckley	405baf7cb2	fix: memory list/search cross-workstream scope leak (#253) ... * fix: scope-filter memory list/search to current workstream and user Unscoped memory(action='list') and memory(action='search') returned all memories across all workstreams. Now applies the same 3-query pattern (global + current workstream + current user) used by system prompt injection. * fix: validate user scope on memory search/list for unauthenticated sessions Adds _validate_scope guard to search and list prepare paths, matching save/get/delete. Prevents explicit scope='user' from returning all user-scoped memories when session is unauthenticated. * fix: update _get_visible_memories references to _list_visible_memories * fix: defense-in-depth guard for empty scope_id on search/list Copilot review: if scope is 'user' or 'workstream' with empty scope_id, the storage query returns all memories in that scope across all users/workstreams. The prepare step already validates via _validate_scope, but add exec-level guard to reject scoped queries with empty scope_id as defense-in-depth.	2026-03-30 04:43:49 -07:00
Patrick Buckley	8d88e6a7eb	feat: add memory get action, reduce search/list preview to 200 chars (#247) ... * feat: add memory get action, reduce search/list preview to 200 chars search and list truncated memory content to 500 chars with no way to read the full value. Two changes: - New 'get' action retrieves a single memory by name with complete untruncated content. Searches scopes narrowest-first (workstream → user → global). - search/list previews reduced from 500 to 200 chars now that get exists for full content. Both append a hint: "Use memory(action='get', name='...') for full content." Includes get_structured_memory_by_name wrapper in memory.py and 4 tests. * Update turnstone/tools/memory.json * fix: include 'get' in _prepare_memory docstring and invalid-action error	2026-03-29 20:53:48 -07:00
Patrick Buckley	723cad24bb	feat: structured memory system — typed/scoped memories with BM25 rele… (#53) ... * feat: structured memory system — typed/scoped memories with BM25 relevance and metacognitive prompting Replace flat key-value memories table with structured_memories (migration 014). Four memory types (user/project/feedback/reference), three scopes (global/workstream/user). Consolidate remember/recall/forget into two tools: memory (action-based: save/search/delete/list) and recall (conversation history only). BM25 relevance scoring (extracted to turnstone/core/bm25.py) selects top-5 memories for system message injection based on conversation context. Metacognitive prompting injects ephemeral nudges after corrections, tool denials, workstream resume, and completion signals. Scope isolation enforced: system message injection and nudge counts filtered to visible memories only (global + current workstream + authenticated user). User scope requires authentication. Content capped at 32KB. ILIKE/LIKE metacharacters escaped in both backends. 113 new tests (2053 total). * fix: CI failure + copilot review feedback - Fix time.monotonic() cooldown: use None sentinel instead of 0.0 default (monotonic clock starts at boot, not epoch — fresh CI runners have uptime < 300s so cooldown check always triggered) - Catch sa.exc.IntegrityError specifically in upsert instead of broad Exception (copilot review) - Preserve existing description/type on upsert when caller doesn't explicitly set them (copilot review) - Add last_accessed + access_count columns to schema/migration for future LRU/LFU eviction support	2026-03-13 21:21:09 -07:00