diff --git a/docs/api-reference.md b/docs/api-reference.md index 79713375..bdc9cf53 100644 --- a/docs/api-reference.md +++ b/docs/api-reference.md @@ -1758,16 +1758,19 @@ Status code: `403` ### `GET /v1/api/memories` -List structured memories with optional filters. Requires `read` scope. +List structured memories with optional filters. Requires `read` scope. Without +`scope`, returns only `global` plus the authenticated caller's `user` +namespace. The public endpoint accepts `global`, `workstream`, and `user`; +explicit workstream access is owner-bound. **Query parameters:** | Parameter | Type | Required | Default | Description | |------------|--------|----------|---------|------------------------------| -| `type` | string | no | `""` | Filter by memory type (user, project, feedback, reference) | +| `type` | string | no | `""` | Filter by memory type (user, general, feedback, reference) | | `scope` | string | no | `""` | Filter by scope (global, workstream, user) | | `scope_id` | string | no | `""` | Scope qualifier. Auto-resolved for `scope=user` when auth is active. | -| `limit` | int | no | `100` | Max results (capped at 200) | +| `limit` | int | no | `100` | Max results (1-200) | **Response:** @@ -1778,7 +1781,7 @@ List structured memories with optional filters. Requires `read` scope. "memory_id": "a1b2c3d4-e5f6-...", "name": "project_architecture", "description": "Core architecture patterns", - "type": "project", + "type": "general", "scope": "global", "scope_id": "", "content": "The project uses a hexagonal architecture...", @@ -1795,7 +1798,8 @@ List structured memories with optional filters. Requires `read` scope. ### `POST /v1/api/memories` Save or upsert a structured memory. Requires `write` scope. Returns `201` on -create, `200` on update. +create, `200` on update. Every write must include a non-empty, non-whitespace +`description`; content-only updates are rejected. **Request body:** @@ -1804,7 +1808,7 @@ create, `200` on update. "name": "deployment_process", "content": "Deploy via GitHub Actions. Staging auto-deploys on push to main.", "description": "CI/CD deployment workflow", - "type": "project", + "type": "general", "scope": "global", "scope_id": "" } @@ -1814,8 +1818,8 @@ create, `200` on update. |--------------|--------|----------|-------------|--------------------------------------| | `name` | string | yes | -- | Memory name (max 256 chars) | | `content` | string | yes | -- | Memory content (max 65536 chars) | -| `description`| string | no | `""` | Short description for search ranking | -| `type` | string | no | `"project"` | One of: user, project, feedback, reference | +| `description`| string | yes | -- | Non-empty relevance summary, required on create and update | +| `type` | string | no | unset | user, general, feedback, or reference | | `scope` | string | no | `"global"` | One of: global, workstream, user | | `scope_id` | string | no | `""` | Scope qualifier (auto-resolved for user scope) | @@ -1826,7 +1830,7 @@ create, `200` on update. "memory_id": "a1b2c3d4-e5f6-...", "name": "deployment_process", "description": "CI/CD deployment workflow", - "type": "project", + "type": "general", "scope": "global", "scope_id": "", "content": "Deploy via GitHub Actions...", @@ -1839,21 +1843,25 @@ create, `200` on update. | Status | Condition | |--------|--------------------------------------------------------| -| 400 | Missing name, empty content, invalid type/scope, name too long, content too long | +| 400 | Invalid input, public scope, scope ID, or limit | +| 403 | Cross-user or non-owner workstream access | +| 404 | Explicit workstream does not exist | +| 500 | Storage mutation failed | --- ### `POST /v1/api/memories/search` Search memories by query. Uses POST for the request body but is non-mutating -(requires only `read` scope). +(requires only `read` scope). An omitted scope searches only `global` plus the +authenticated caller's `user` namespace. **Request body:** ```json { "query": "authentication", - "type": "project", + "type": "general", "scope": "", "limit": 20 } @@ -1865,7 +1873,7 @@ Search memories by query. Uses POST for the request body but is non-mutating | `type` | string | no | `""` | Filter by type | | `scope` | string | no | `""` | Filter by scope | | `scope_id` | string | no | `""` | Filter by scope ID | -| `limit` | int | no | `20` | Max results (capped at 50) | +| `limit` | int | no | `20` | Max results (1-50) | **Response:** @@ -1876,7 +1884,7 @@ Search memories by query. Uses POST for the request body but is non-mutating "memory_id": "a1b2c3d4-e5f6-...", "name": "auth_patterns", "description": "Authentication architecture", - "type": "project", + "type": "general", "scope": "global", "scope_id": "", "content": "JWT tokens with HS256...", @@ -1894,7 +1902,9 @@ Search memories by query. Uses POST for the request body but is non-mutating ### `DELETE /v1/api/memories/{name}` -Delete a memory by name and scope. Requires `write` scope. +Delete a memory by name and scope. Requires `write` scope. The delete returns +success only for the row atomically removed and records the authenticated +actor in the audit log. **Path parameters:** @@ -1978,7 +1988,7 @@ Get a single memory by ID. Requires `admin.memories` permission. "memory_id": "a1b2c3d4-e5f6-...", "name": "project_architecture", "description": "Core architecture patterns", - "type": "project", + "type": "general", "scope": "global", "scope_id": "", "content": "The project uses...", diff --git a/docs/coordinator-skills.md b/docs/coordinator-skills.md index 72c144d2..e2add1d9 100644 --- a/docs/coordinator-skills.md +++ b/docs/coordinator-skills.md @@ -77,7 +77,7 @@ or MCP config can do adds to it. Current members: | `delete_workstream` | wind-down | Hard-delete one child. Requires approval. | | `list_nodes` | discover | Enumerate live cluster nodes + capabilities. | | `skills` (action=find) | discover | Browse the skill catalog; opt-in `kind` filter narrows by audience. | -| `memory` | persist | Durable orchestration memory (`coordinator` scope, per-user — survives across coordinator sessions). | +| `memory` | persist | Durable acting-user orchestration memory (`coordinator`), plus shared memory when attached to a project. | | `notify` | broadcast | Post a status update to a human channel at a narrative beat. | | `tasks` | plan | Orchestrator-only scratchpad. Children don't see it. | diff --git a/docs/diagrams/23-memory-architecture.puml b/docs/diagrams/23-memory-architecture.puml index 7a58b6af..00e45f52 100644 --- a/docs/diagrams/23-memory-architecture.puml +++ b/docs/diagrams/23-memory-architecture.puml @@ -20,58 +20,61 @@ participant "SDK Client\n(sdk/)" as SDK <> == Phase 1: Tool Path (session.send) == -Session -> Session : _prepare_tool_calls()\nparse memory(action=...) +Session -> Session : pin acting principal\nparse memory(action=...) note right - Tool schema: 4 actions - save, search, delete, list + Tool schema: 5 actions + save, get, search, delete, list Auto-approved (no approval needed) end note +Session -> Session : resolve live project access\nselect exact/inherited scope + Session -> Session : _exec_memory(item) alt action = save - Session -> Facade : save_structured_memory(\nname, content, description,\nmem_type, scope, scope_id) + Session -> Session : require non-empty description + Session -> Facade : save_structured_memory_strict(\n..., require_active_project) Facade -> Facade : normalize_key(name) - Facade -> Storage : create_structured_memory() - alt unique constraint violation - Storage --> Facade : IntegrityError - Facade -> Storage : get_structured_memory_by_name() - Storage --> Facade : existing row - Facade -> Storage : update_structured_memory() - end - Storage --> Facade : memory_id - Facade --> Session : (memory_id, old_content) - Session -> Session : _init_system_messages()\nrefresh BM25 context + Facade -> Storage : guarded atomic upsert\nON CONFLICT ... RETURNING + Storage --> Facade : (saved row, was_update) + Facade --> Session : saved row + Session -> Session : invalidate prefix/cache\naudit acting principal +end + +alt action = get + Session -> Facade : get_structured_memory_by_name_strict() + Facade -> Storage : exact scoped-name lookup + Storage --> Session : full row / not found end alt action = search - Session -> Facade : search_structured_memories(\nquery, mem_type, scope,\nscope_id, limit) - Facade -> Storage : search_structured_memories() + Session -> Storage : search exact scope or\nactor-visible scope union Storage --> Session : matched rows end alt action = delete - Session -> Facade : delete_structured_memory(\nname, scope, scope_id) - Facade -> Storage : delete_structured_memory() - Storage --> Session : bool (existed) - Session -> Session : _init_system_messages()\nrefresh BM25 context + Session -> Facade : delete_structured_memory_returning_strict() + Facade -> Storage : DELETE ... RETURNING + Storage --> Session : deleted row / not found + Session -> Session : invalidate + audit\nmark prefix dirty end == Phase 2: BM25 Relevance Injection == Session -> Session : _init_system_messages()\nevery conversation turn +Session -> Session : resolve acting principal\nand live project ACL Session -> Session : _list_visible_memories(\nlimit=fetch_limit) note right **Scope resolution:** - 1. global scope (always) - 2. workstream scope (ws_id) - 3. user scope (user_id, if auth) - Combined and deduplicated. + Interactive: global + workstream + + acting user + readable project + Coordinator: acting user's coordinator + + readable project end note -Session -> Facade : list_structured_memories()\nper scope -Facade -> Storage : list_structured_memories() +Session -> Facade : list_visible_structured_memories() +Facade -> Storage : one visibility-union query Storage --> Session : up to fetch_limit rows Session -> Relevance : extract_recent_context(\nmessages, max_messages=3) @@ -103,28 +106,31 @@ Session -> Session : inject into\nsystem message == Phase 3: Server API Path == -SDK -> API : GET /v1/api/memories\n?type=project&limit=20 -API -> Facade : list_structured_memories() -Facade -> Storage : list_structured_memories() +SDK -> API : GET /v1/api/memories\n?type=general&limit=20 +API -> API : bind scope to caller\ndefault global + caller user +API -> Storage : list visible rows Storage --> API : rows API --> SDK : {"memories": [...], "total": N} -SDK -> API : POST /v1/api/memories\n{name, content, ...} -API -> API : validate type, scope,\nname length, content length -API -> Facade : save_structured_memory() -Facade -> Storage : create / update +SDK -> API : POST /v1/api/memories\n{name, content, description, ...} +API -> API : validate type, scope,\nname/content/description +API -> API : reject internal scopes\nowner-bind workstream scope +API -> Facade : save_structured_memory_strict() +Facade -> Storage : atomic upsert Storage --> API : memory row +API -> API : record_audit(actor) API --> SDK : 201 (created) / 200 (updated) SDK -> API : POST /v1/api/memories/search\n{query, type, ...} -API -> Facade : search_structured_memories() -Facade -> Storage : search_structured_memories() +API -> API : bind scope to caller +API -> Storage : search visible rows Storage --> API : matched rows API --> SDK : {"memories": [...], "total": N} SDK -> API : DELETE /v1/api/memories/{name}\n?scope=global -API -> Facade : delete_structured_memory() -Facade -> Storage : delete row +API -> Facade : delete_structured_memory_returning_strict() +Facade -> Storage : DELETE ... RETURNING +API -> API : record_audit(actor) API --> SDK : {"status": "ok"} == Phase 4: Console Admin Path == @@ -141,7 +147,7 @@ Storage --> Admin : memory row Admin --> SDK : memory JSON SDK -> Admin : DELETE /v1/api/admin/memories/{id} -Admin -> Storage : delete_structured_memory_by_id() +Admin -> Storage : delete_structured_memory_by_id_returning() Admin -> Admin : record_audit(\n"memory.delete") Admin --> SDK : {"status": "ok"} diff --git a/docs/diagrams/png/23-memory-architecture.png b/docs/diagrams/png/23-memory-architecture.png index 4a2e6d0b..6a15e19d 100644 --- a/docs/diagrams/png/23-memory-architecture.png +++ b/docs/diagrams/png/23-memory-architecture.png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:bd7fe8bf5c2b56b075453a316e54d61214b0ae912517d9cad6c1e88785aac722 -size 300010 +oid sha256:137d6c91a34695c820d8b0a33fd753e79165604aa92bf2ac8480d3744b2ef844 +size 305199 diff --git a/docs/memory.md b/docs/memory.md index 137e2fb4..11e909a1 100644 --- a/docs/memory.md +++ b/docs/memory.md @@ -20,7 +20,7 @@ Each memory has three dimensions: | Type | Purpose | |-------------|------------------------------------------------------------| | `user` | User preferences, conventions, working style | -| `project` | Project-specific knowledge, architecture, patterns | +| `general` | General knowledge, architecture, patterns | | `feedback` | Corrections, lessons learned, things to avoid | | `reference` | Reference material, documentation, specifications | @@ -31,25 +31,37 @@ Each memory has three dimensions: | `global` | Visible to all workstreams and users | | `workstream` | Visible only within the originating workstream | | `user` | Follows the authenticated user across workstreams | -| `coordinator` | Coordinator sessions only; follows the user across coordinators | +| `coordinator` | Coordinator sessions only; follows the acting user | +| `project` | Shared by workstreams attached to one active project | A memory's identity is the tuple `(name, scope, scope_id)`. Saving a memory with the same identity upserts -- updating content while preserving the ID. -### Coordinator scope +### Inherited target and coordinator scope -Coordinator sessions are isolated to a single scope: `coordinator`, keyed by -the coordinator's creator `user_id`. It is durable -- every coordinator -session the same user runs (including concurrent ones) shares one -orchestration namespace, so procedures and lessons survive close/reopen. +Name-based operations use one inherited target when `scope` is omitted: + +- An attached active project selects `project` for `save`, `get`, and + `delete`. +- Read-only project access permits `get`, but `save` and `delete` fail. They do + not fall back to a broader namespace. +- Without a project, interactive sessions select `global`; coordinator + sessions select `coordinator`. + +A valid explicit scope selects exactly that scope. `search` and `list` are the +only actions that span every visible scope when `scope` is omitted. + +Each coordinator's private `coordinator` namespace is keyed by the acting +user's `user_id`. It is durable -- every coordinator session that user runs +(including concurrent ones) shares one orchestration namespace, so procedures +and lessons survive close/reopen. Isolation is bidirectional and enforced by session kind, not by secrecy of the scope id: -- A coordinator session can read and write **only** `coordinator`-scope rows. - It never sees `global`/`workstream`/`user` memories, so content written by - interactive sessions (which routinely ingest untrusted MCP/attachment - output) cannot reach a coordinator's system message. +- A coordinator session sees its acting user's `coordinator` scope and, when + attached, the shared `project` scope. It never sees + `global`/`workstream`/`user` memories. - Interactive sessions -- including a coordinator's own children, which share its `user_id` -- are rejected from the `coordinator` scope on every memory action. Children cannot plant rows the parent coordinator would read. @@ -64,12 +76,13 @@ coordinator cannot be constructed, so the scope id is always a real user. On every conversation turn, the system: -1. Fetches up to `fetch_limit` memories visible in the current scope -2. Extracts context from the last 3 user messages -3. Scores memories against that context using a BM25 index -4. Injects the top `relevance_k` memories into the system message as +1. Resolves the acting principal and their live project access +2. Fetches up to `fetch_limit` memories across that visibility envelope +3. Extracts context from the last 3 user messages +4. Scores memories against that context using a BM25 index +5. Injects the top `relevance_k` memories into the system message as `` XML tags -5. Appends a hint telling the model how many memories are in scope +6. Appends a hint telling the model how many memories are in scope This means the model always has its most relevant memories available without explicit recall -- but can still use `memory(action='search')` for deeper @@ -107,19 +120,23 @@ All fields are optional. Defaults are shown above. ## Tool Usage -The `memory` tool supports four actions: +The `memory` tool supports five actions: ### save Store or update a memory. +Every save is a complete write for the relevance summary: `description` must +be supplied and contain non-whitespace text on both creation and update. +Content-only updates are rejected. + ```json { "action": "save", "name": "project_architecture", "content": "The project uses a hexagonal architecture with...", "description": "Core architecture patterns", - "type": "project", + "type": "general", "scope": "global" } ``` @@ -128,9 +145,26 @@ Store or update a memory. |---------------|----------|-------------|------------------------------------------| | `name` | yes | -- | Snake_case identifier (max 256 chars) | | `content` | yes | -- | Memory content (max `max_content` chars) | -| `description` | no | `""` | Short description for relevance matching | -| `type` | no | `"project"` | One of: user, project, feedback, reference | -| `scope` | no | `"global"` | One of: global, workstream, user | +| `description` | yes | -- | Non-empty relevance summary, required on create and update | +| `type` | no | `"general"` | One of: user, general, feedback, reference | +| `scope` | no | inherited | Kind-valid scope; see inherited target above | + +### get + +Retrieve the full content of one memory by name. + +```json +{ + "action": "get", + "name": "project_architecture", + "scope": "project" +} +``` + +| Parameter | Required | Default | Description | +|-----------|----------|-----------|----------------------------| +| `name` | yes | -- | Memory name to retrieve | +| `scope` | no | inherited | Exact scope to query | ### search @@ -140,7 +174,7 @@ Find memories by query (BM25 full-text search). { "action": "search", "query": "authentication patterns", - "type": "project", + "type": "general", "limit": 10 } ``` @@ -167,7 +201,7 @@ Remove a memory by name. | Parameter | Required | Default | Description | |------------|----------|------------|--------------------------| | `name` | yes | -- | Memory name to delete | -| `scope` | no | `"global"` | Scope of the memory | +| `scope` | no | inherited | Exact scope to delete | ### list @@ -197,6 +231,12 @@ Four endpoints on the server for programmatic memory access. List memories with optional filters. +Without `scope`, the response is restricted to `global` plus the authenticated +caller's `user` namespace. The public API accepts only `global`, `user`, and +`workstream`; internal `project` and `coordinator` namespaces remain available +through the session tool and admin API. Explicit `workstream` access requires +its persisted owner (or a service token). + **Query parameters:** | Parameter | Type | Required | Default | Description | @@ -204,10 +244,10 @@ List memories with optional filters. | `type` | string | no | `""` | Filter by memory type | | `scope` | string | no | `""` | Filter by scope | | `scope_id` | string | no | `""` | Filter by scope ID | -| `limit` | int | no | `100` | Max results (capped at 200) | +| `limit` | int | no | `100` | Max results (1-200) | -When `scope=user` and `scope_id` is omitted, the authenticated user's ID is -used automatically. +When `scope=user`, the authenticated user's ID is used automatically and a +different supplied ID is rejected. `scope=workstream` requires `scope_id`. **Response:** `200` @@ -218,7 +258,7 @@ used automatically. "memory_id": "a1b2c3d4-e5f6-...", "name": "project_architecture", "description": "Core architecture patterns", - "type": "project", + "type": "general", "scope": "global", "scope_id": "", "content": "The project uses a hexagonal architecture...", @@ -236,6 +276,9 @@ used automatically. Save or upsert a structured memory. +`description` is mandatory for both creates and updates and must contain +non-whitespace text. The API rejects content-only updates. + **Request body:** ```json @@ -243,7 +286,7 @@ Save or upsert a structured memory. "name": "deployment_process", "content": "Deploy via GitHub Actions. Staging auto-deploys on push to main.", "description": "CI/CD deployment workflow", - "type": "project", + "type": "general", "scope": "global", "scope_id": "" } @@ -253,8 +296,8 @@ Save or upsert a structured memory. |--------------|--------|----------|-------------|--------------------------------------| | `name` | string | yes | -- | Memory name (max 256 chars) | | `content` | string | yes | -- | Memory content (max 65536 chars) | -| `description`| string | no | `""` | Short description for search ranking | -| `type` | string | no | `"project"` | One of: user, project, feedback, reference | +| `description`| string | yes | -- | Non-empty relevance summary, required on create and update | +| `type` | string | no | unset | user, general, feedback, or reference | | `scope` | string | no | `"global"` | One of: global, workstream, user | | `scope_id` | string | no | `""` | Scope qualifier (auto-resolved for user scope) | @@ -265,7 +308,7 @@ Save or upsert a structured memory. "memory_id": "a1b2c3d4-e5f6-...", "name": "deployment_process", "description": "CI/CD deployment workflow", - "type": "project", + "type": "general", "scope": "global", "scope_id": "", "content": "Deploy via GitHub Actions...", @@ -281,7 +324,10 @@ same `(name, scope, scope_id)` already existed. | Status | Condition | |--------|------------------------------------| -| 400 | Missing name, empty content, invalid type/scope, content too long | +| 400 | Invalid input, scope, scope ID, or limit | +| 403 | Cross-user or non-owner workstream access | +| 404 | Explicit workstream does not exist | +| 500 | Storage mutation failed | --- @@ -290,12 +336,15 @@ same `(name, scope, scope_id)` already existed. Search memories by query. Uses POST for the request body but is non-mutating (requires only `read` scope). +An omitted scope searches the same caller-bound `global` + `user` envelope as +the list endpoint. It never means every row in the table. + **Request body:** ```json { "query": "authentication", - "type": "project", + "type": "general", "scope": "", "scope_id": "", "limit": 20 @@ -308,7 +357,7 @@ Search memories by query. Uses POST for the request body but is non-mutating | `type` | string | no | `""` | Filter by type | | `scope` | string | no | `""` | Filter by scope | | `scope_id` | string | no | `""` | Filter by scope ID | -| `limit` | int | no | `20` | Max results (capped at 50) | +| `limit` | int | no | `20` | Max results (1-50) | **Response:** `200` @@ -319,7 +368,7 @@ Search memories by query. Uses POST for the request body but is non-mutating "memory_id": "a1b2c3d4-e5f6-...", "name": "auth_patterns", "description": "Authentication architecture", - "type": "project", + "type": "general", "scope": "global", "scope_id": "", "content": "JWT tokens with HS256...", @@ -337,6 +386,9 @@ Search memories by query. Uses POST for the request body but is non-mutating Delete a memory by name and scope. +Deletes are atomic: the row used for the success result and audit event is the +row actually removed. A storage failure returns `500`, not a false `404`. + **Path parameters:** | Parameter | Type | Description | @@ -391,7 +443,7 @@ List memories across all scopes (no automatic scope resolution). "memory_id": "a1b2c3d4-e5f6-...", "name": "project_architecture", "description": "Core architecture patterns", - "type": "project", + "type": "general", "scope": "global", "scope_id": "", "content": "The project uses...", @@ -440,7 +492,7 @@ Get a single memory by ID. "memory_id": "a1b2c3d4-e5f6-...", "name": "project_architecture", "description": "Core architecture patterns", - "type": "project", + "type": "general", "scope": "global", "scope_id": "", "content": "The project uses...", @@ -497,13 +549,13 @@ with TurnstoneServer("http://localhost:8080", token="tok_xxx") as client: "api_conventions", "All endpoints use /v1/ prefix. JSON responses.", description="API design patterns", - mem_type="project", + mem_type="general", scope="global", ) print(mem.memory_id) # Search memories - results = client.search_memories("authentication", mem_type="project", limit=10) + results = client.search_memories("authentication", mem_type="general", limit=10) for m in results.memories: print(f"{m['name']}: {m['description']}") @@ -524,7 +576,7 @@ with TurnstoneConsole("http://localhost:9090", token="tok_xxx") as admin: result = admin.list_memories(scope="global", limit=100) # Search - result = admin.search_memories("architecture", mem_type="project") + result = admin.search_memories("architecture", mem_type="general") # Get by ID mem = admin.get_memory("a1b2c3d4-e5f6-...") @@ -548,14 +600,14 @@ const mem = await client.saveMemory({ name: "api_conventions", content: "All endpoints use /v1/ prefix. JSON responses.", description: "API design patterns", - type: "project", + type: "general", scope: "global", }); // Search memories const results = await client.searchMemories({ query: "authentication", - type: "project", + type: "general", limit: 10, }); diff --git a/docs/tools.md b/docs/tools.md index 1dc353f5..bf8c9b54 100644 --- a/docs/tools.md +++ b/docs/tools.md @@ -152,7 +152,7 @@ into its successor's trajectory. **Auto-approved** (no user confirmation needed at runtime): - `read_file` -- reads files, no side effects - `search` -- grep-style search, no side effects -- `memory` -- structured persistent memory (save/search/delete/list) +- `memory` -- structured persistent memory (save/get/search/delete/list) - `recall` -- searches conversation history - `notify` -- sends notifications to linked channels (time-sensitive, auto-approved for urgency) @@ -433,7 +433,7 @@ Delegate a general-purpose task to an autonomous sub-agent. |-----------|--------|----------|-------------| | `prompt` | string | yes | Complete task description for the sub-agent. | -- **What it does**: Spawns a sub-agent that inherits the `TASK_AGENT_TOOLS` set (read, write, edit, search, bash, web tools, memory tools). The sub-agent runs autonomously to completion. Use for work that requires file modifications or command execution. +- **What it does**: Spawns a sub-agent that inherits the `TASK_AGENT_TOOLS` set (read, write, edit, search, bash, and web tools). The sub-agent runs autonomously to completion. Use for work that requires file modifications or command execution. - **Auto-approve**: No -- requires user confirmation. - **Agent availability**: Top-level only. @@ -447,18 +447,26 @@ Structured persistent memory across sessions with typed, scoped entries. | Parameter | Type | Required | Description | |---------------|---------|----------|-------------| -| `action` | string | yes | `save`, `search`, `delete`, or `list`. | -| `name` | string | save/delete | Short snake_case identifier for the memory. | +| `action` | string | yes | `save`, `get`, `search`, `delete`, or `list`. | +| `name` | string | save/get/delete | Short snake_case identifier for the memory. | | `content` | string | save | Memory content to store. | -| `description` | string | no | Short description for relevance matching (recommended for `save`). | -| `type` | string | no | Memory type: `user`, `project`, `feedback`, or `reference`. Default: `project`. | -| `scope` | string | no | Memory scope: `global`, `workstream`, or `user`. Default: `global`. | +| `description` | string | save | Non-empty description for relevance matching; required on create and update. | +| `type` | string | no | Memory type: `user`, `general`, `feedback`, or `reference`. Default: `general`. | +| `scope` | string | no | Memory scope: `global`, `workstream`, `user`, `coordinator`, or `project`. See defaults below. | | `query` | string | search | Search query for finding memories. | | `limit` | integer | no | Max results for `search` or `list`. Default: 20. | -- **What it does**: Manages structured persistent memories in the database. Memories persist across sessions, have a type classification (user preferences, project knowledge, feedback, reference material) and a scope (global across all workstreams, private to a workstream, or following a user). Relevant memories are included in the system prompt on startup. +- **What it does**: Manages structured persistent memories in the database. + Memories persist across sessions, have a type classification, and live in a + role-specific visible scope. Unscoped `save`/`get`/`delete` resolve to one + target: the attached active project, otherwise `global` for an interactive + session or `coordinator` for a coordinator. Read-only project access permits + `get` but makes `save`/`delete` fail without falling back. A valid explicit + scope selects exactly that scope. Unscoped `search`/`list` cover all visible + scopes; use the displayed scope when following a result with `get` or + `delete`. - **Auto-approve**: Yes. -- **Agent availability**: Not available to sub-agents (top-level only). +- **Agent availability**: Not available to task agents. --- @@ -473,7 +481,7 @@ Search conversation history for past messages and tool results. - **What it does**: Searches conversation history across sessions using FTS5 full-text search. Returns matching messages, tool calls, and tool results with timestamps and workstream context. - **Auto-approve**: Yes. -- **Agent availability**: Not available to sub-agents (top-level only). +- **Agent availability**: Not available to task agents. --- diff --git a/sdk/typescript/openapi-console.json b/sdk/typescript/openapi-console.json index 8d9ac624..6ce4888e 100644 --- a/sdk/typescript/openapi-console.json +++ b/sdk/typescript/openapi-console.json @@ -2,7 +2,7 @@ "openapi": "3.1.0", "info": { "title": "turnstone Console API", - "version": "1.8.0a6", + "version": "1.8.0a7", "description": "Cluster-wide visibility and control across all turnstone nodes." }, "paths": { diff --git a/sdk/typescript/openapi-server.json b/sdk/typescript/openapi-server.json index 4a418ad8..d2aa0f42 100644 --- a/sdk/typescript/openapi-server.json +++ b/sdk/typescript/openapi-server.json @@ -2,7 +2,7 @@ "openapi": "3.1.0", "info": { "title": "turnstone Server API", - "version": "1.8.0a6", + "version": "1.8.0a7", "description": "Single-node workstream management, chat interaction, and real-time streaming." }, "paths": { @@ -1869,7 +1869,7 @@ }, "/v1/api/memories": { "get": { - "summary": "List structured memories", + "summary": "List structured memories. Without a scope, returns global plus the authenticated user's memories; workstream scope is owner-bound.", "operationId": "v1_api_memories_get", "tags": [ "Memories" @@ -1891,7 +1891,7 @@ "schema": { "type": "string" }, - "description": "Filter by scope" + "description": "Filter by public scope: global, workstream, or user" }, { "name": "scope_id", @@ -1923,6 +1923,46 @@ } } } + }, + "400": { + "description": "Error 400", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorResponse" + } + } + } + }, + "403": { + "description": "Error 403", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorResponse" + } + } + } + }, + "404": { + "description": "Error 404", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorResponse" + } + } + } + }, + "500": { + "description": "Error 500", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorResponse" + } + } + } } } }, @@ -1962,13 +2002,43 @@ } } } + }, + "403": { + "description": "Error 403", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorResponse" + } + } + } + }, + "404": { + "description": "Error 404", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorResponse" + } + } + } + }, + "500": { + "description": "Error 500", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorResponse" + } + } + } } } } }, "/v1/api/memories/search": { "post": { - "summary": "Search structured memories by query", + "summary": "Search structured memories by query. Without a scope, searches global plus the authenticated user's memories.", "operationId": "v1_api_memories_search_post", "tags": [ "Memories" @@ -1993,6 +2063,46 @@ } } } + }, + "400": { + "description": "Error 400", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorResponse" + } + } + } + }, + "403": { + "description": "Error 403", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorResponse" + } + } + } + }, + "404": { + "description": "Error 404", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorResponse" + } + } + } + }, + "500": { + "description": "Error 500", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorResponse" + } + } + } } } } @@ -2043,6 +2153,26 @@ } } }, + "400": { + "description": "Error 400", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorResponse" + } + } + } + }, + "403": { + "description": "Error 403", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorResponse" + } + } + } + }, "404": { "description": "Error 404", "content": { @@ -2052,6 +2182,16 @@ } } } + }, + "500": { + "description": "Error 500", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorResponse" + } + } + } } } } @@ -3854,32 +3994,42 @@ "properties": { "name": { "description": "Memory identifier (normalized to snake_case)", + "maxLength": 256, + "minLength": 1, "title": "Name", "type": "string" }, "content": { "description": "Memory content", "maxLength": 65536, + "minLength": 1, "title": "Content", "type": "string" }, "description": { - "default": "", - "description": "Short description for relevance matching", + "description": "Required non-empty description used for relevance matching", + "minLength": 1, "title": "Description", "type": "string" }, "type": { - "default": "general", - "description": "Memory type", - "enum": [ - "user", - "general", - "feedback", - "reference" + "anyOf": [ + { + "enum": [ + "user", + "general", + "feedback", + "reference" + ], + "type": "string" + }, + { + "type": "null" + } ], - "title": "Type", - "type": "string" + "default": null, + "description": "Memory type; omission preserves it on update and defaults on insert", + "title": "Type" }, "scope": { "default": "global", @@ -3901,7 +4051,8 @@ }, "required": [ "name", - "content" + "content", + "description" ], "title": "SaveMemoryRequest", "type": "object" @@ -3995,6 +4146,7 @@ "properties": { "query": { "description": "Search query text", + "minLength": 1, "title": "Query", "type": "string" }, diff --git a/sdk/typescript/src/server.ts b/sdk/typescript/src/server.ts index 0edfa646..18bc759b 100644 --- a/sdk/typescript/src/server.ts +++ b/sdk/typescript/src/server.ts @@ -394,7 +394,14 @@ export class TurnstoneServer extends BaseClient { } async saveMemory(opts: SaveMemoryRequest): Promise { - return this.request("POST", "/v1/api/memories", { json: opts }); + if (typeof opts.description !== "string" || !opts.description.trim()) { + throw new TypeError( + "memory description is required and must be non-empty", + ); + } + return this.request("POST", "/v1/api/memories", { + json: { ...opts, description: opts.description.trim() }, + }); } async searchMemories( diff --git a/sdk/typescript/src/types.ts b/sdk/typescript/src/types.ts index e718f5b7..a12a4c7b 100644 --- a/sdk/typescript/src/types.ts +++ b/sdk/typescript/src/types.ts @@ -894,7 +894,7 @@ export interface WorkstreamsOptions { export interface SaveMemoryRequest { name: string; content: string; - description?: string; + description: string; type?: "user" | "general" | "feedback" | "reference"; scope?: "global" | "workstream" | "user"; scope_id?: string; diff --git a/sdk/typescript/tests/server.test.ts b/sdk/typescript/tests/server.test.ts index abba8388..7686d088 100644 --- a/sdk/typescript/tests/server.test.ts +++ b/sdk/typescript/tests/server.test.ts @@ -75,6 +75,43 @@ describe("TurnstoneServer", () => { }); }); + it("saveMemory requires and normalizes the description", async () => { + const fetchFn = mockFetch({ + memory_id: "m1", + name: "deployment_process", + description: "Production deployment workflow", + type: "general", + scope: "global", + scope_id: "", + content: "Deploy from main", + created: "2026-08-11T00:00:00", + updated: "2026-08-11T00:00:00", + }); + const client = new TurnstoneServer({ + baseUrl: "http://test", + fetch: fetchFn, + }); + + await client.saveMemory({ + name: "deployment_process", + content: "Deploy from main", + description: " Production deployment workflow ", + }); + const [, init] = (fetchFn as ReturnType).mock.calls[0]; + expect(JSON.parse(init.body)).toMatchObject({ + description: "Production deployment workflow", + }); + + await expect( + client.saveMemory({ + name: "deployment_process", + content: "Deploy from main", + description: " ", + }), + ).rejects.toThrow("description is required"); + expect(fetchFn).toHaveBeenCalledTimes(1); + }); + it("send posts correct payload", async () => { const fetchFn = mockFetch({ status: "ok" }); const client = new TurnstoneServer({ diff --git a/tests/test_coordinator_tools.py b/tests/test_coordinator_tools.py index 8620c505..e55892d6 100644 --- a/tests/test_coordinator_tools.py +++ b/tests/test_coordinator_tools.py @@ -2261,6 +2261,7 @@ def test_prepare_and_write_path_refuse_in_the_same_words(coord_session): item = sess._prepare_tool(_tc("tasks", args)) assert "error" in item, args expected = sess._coord_tool_error("call-1", "tasks", f"{action}: {authoritative['error']}") + expected["_principal_id"] = sess._tool_prepare_principal_id() assert item == expected, (args, item["error"]) diff --git a/tests/test_eval_nudges.py b/tests/test_eval_nudges.py index 5f80524a..327cf4b3 100644 --- a/tests/test_eval_nudges.py +++ b/tests/test_eval_nudges.py @@ -256,6 +256,7 @@ class TestWorldSeeding: "memory": [ { "name": "proj-context", + "description": "Project deployment context", "content": "acme-api: staging tracks main.", "type": "reference", } diff --git a/tests/test_memory_api.py b/tests/test_memory_api.py index 4bcdb3c0..7fd99194 100644 --- a/tests/test_memory_api.py +++ b/tests/test_memory_api.py @@ -121,7 +121,7 @@ def _seed_memory(storage, name="test_key", content="test content", **kw): storage.create_structured_memory( mid, name, - kw.get("description", ""), + kw.get("description", "Seeded memory"), kw.get("mem_type", "general"), kw.get("scope", "global"), kw.get("scope_id", ""), @@ -130,6 +130,20 @@ def _seed_memory(storage, name="test_key", content="test content", **kw): return mid +def _seed_workstream(storage, ws_id: str = "ws1", user_id: str = "test-user") -> None: + storage.register_workstream(ws_id, user_id=user_id) + + +def _save_body(name: str, content: str, **overrides: Any) -> dict[str, Any]: + body: dict[str, Any] = { + "name": name, + "content": content, + "description": f"Description for {name}", + } + body.update(overrides) + return body + + # =========================================================================== # Server endpoint tests # =========================================================================== @@ -158,6 +172,7 @@ class TestServerListMemories: assert r.json()["memories"][0]["name"] == "a" def test_filter_by_scope(self, server_client, storage): + _seed_workstream(storage) _seed_memory(storage, "a", "x", scope="global") _seed_memory(storage, "b", "y", scope="workstream", scope_id="ws1") r = server_client.get("/v1/api/memories?scope=workstream&scope_id=ws1") @@ -174,12 +189,38 @@ class TestServerListMemories: r = server_client.get("/v1/api/memories?limit=abc") assert r.status_code == 400 + def test_unscoped_list_is_caller_bound(self, server_client, storage): + _seed_memory(storage, "global_visible", "g") + _seed_memory(storage, "own_visible", "u", scope="user", scope_id="test-user") + _seed_memory(storage, "victim_user", "secret", scope="user", scope_id="victim") + _seed_memory(storage, "victim_coord", "secret", scope="coordinator", scope_id="victim") + _seed_memory(storage, "private_project", "secret", scope="project", scope_id="p1") + + r = server_client.get("/v1/api/memories") + + assert r.status_code == 200 + assert {row["name"] for row in r.json()["memories"]} == { + "global_visible", + "own_visible", + } + + def test_internal_scopes_are_rejected(self, server_client): + for scope in ("coordinator", "project", "bogus"): + r = server_client.get(f"/v1/api/memories?scope={scope}&scope_id=victim") + assert r.status_code == 400 + + def test_workstream_scope_is_owner_bound(self, server_client, storage): + _seed_workstream(storage, "victim-ws", "victim") + _seed_memory(storage, "secret", "x", scope="workstream", scope_id="victim-ws") + r = server_client.get("/v1/api/memories?scope=workstream&scope_id=victim-ws") + assert r.status_code == 403 + class TestServerSaveMemory: def test_create(self, server_client): r = server_client.post( "/v1/api/memories", - json={"name": "my_key", "content": "my content"}, + json=_save_body("my_key", "my content"), ) assert r.status_code == 201 data = r.json() @@ -191,21 +232,23 @@ class TestServerSaveMemory: def test_upsert(self, server_client): server_client.post( "/v1/api/memories", - json={"name": "key", "content": "v1"}, + json=_save_body("key", "v1"), ) r = server_client.post( "/v1/api/memories", - json={"name": "key", "content": "v2"}, + json=_save_body("key", "v2", description="Updated key description"), ) assert r.status_code == 200 assert r.json()["content"] == "v2" - def test_with_type_and_scope(self, server_client): + def test_with_type_and_scope(self, server_client, storage): + _seed_workstream(storage) r = server_client.post( "/v1/api/memories", json={ "name": "feedback_key", "content": "data", + "description": "Feedback memory", "type": "feedback", "scope": "workstream", "scope_id": "ws1", @@ -216,17 +259,30 @@ class TestServerSaveMemory: assert r.json()["scope"] == "workstream" def test_missing_name(self, server_client): - r = server_client.post("/v1/api/memories", json={"content": "data"}) + r = server_client.post( + "/v1/api/memories", json={"content": "data", "description": "Missing name"} + ) assert r.status_code == 400 def test_missing_content(self, server_client): - r = server_client.post("/v1/api/memories", json={"name": "k"}) + r = server_client.post( + "/v1/api/memories", json={"name": "k", "description": "Missing content"} + ) assert r.status_code == 400 + @pytest.mark.parametrize("description", [None, "", " "]) + def test_missing_or_empty_description(self, server_client, description): + r = server_client.post( + "/v1/api/memories", + json={"name": "k", "content": "c", "description": description}, + ) + assert r.status_code == 400 + assert "description is required" in r.json()["error"] + def test_invalid_type(self, server_client): r = server_client.post( "/v1/api/memories", - json={"name": "k", "content": "c", "type": "bogus"}, + json=_save_body("k", "c", type="bogus"), ) assert r.status_code == 400 assert "invalid type" in r.json()["error"] @@ -234,7 +290,7 @@ class TestServerSaveMemory: def test_invalid_scope(self, server_client): r = server_client.post( "/v1/api/memories", - json={"name": "k", "content": "c", "scope": "bogus"}, + json=_save_body("k", "c", scope="bogus"), ) assert r.status_code == 400 assert "invalid scope" in r.json()["error"] @@ -242,7 +298,7 @@ class TestServerSaveMemory: def test_content_too_large(self, server_client): r = server_client.post( "/v1/api/memories", - json={"name": "k", "content": "x" * 70000}, + json=_save_body("k", "x" * 70000), ) assert r.status_code == 400 assert "limit" in r.json()["error"] @@ -250,18 +306,32 @@ class TestServerSaveMemory: def test_name_normalisation(self, server_client): r = server_client.post( "/v1/api/memories", - json={"name": "My-Key Name", "content": "data"}, + json=_save_body("My-Key Name", "data"), ) assert r.status_code == 201 assert r.json()["name"] == "my_key_name" + def test_create_and_update_are_audited(self, server_client, storage): + first = server_client.post( + "/v1/api/memories", + json=_save_body("audit_me", "v1"), + ) + second = server_client.post( + "/v1/api/memories", + json=_save_body("audit_me", "v2", description="Updated audit memory"), + ) + assert first.status_code == 201 + assert second.status_code == 200 + assert len(storage.list_audit_events(action="memory.save", user_id="test-user")) == 1 + assert len(storage.list_audit_events(action="memory.update", user_id="test-user")) == 1 + class TestServerUserScopeSecurity: def test_user_scope_binds_to_auth(self, server_client): """User scope auto-resolves scope_id from authenticated user.""" r = server_client.post( "/v1/api/memories", - json={"name": "priv", "content": "secret", "scope": "user"}, + json=_save_body("priv", "secret", scope="user"), ) assert r.status_code == 201 assert r.json()["scope_id"] == "test-user" @@ -270,7 +340,7 @@ class TestServerUserScopeSecurity: """Cannot access another user's memories via scope_id.""" r = server_client.post( "/v1/api/memories", - json={"name": "x", "content": "y", "scope": "user", "scope_id": "other-user"}, + json=_save_body("x", "y", scope="user", scope_id="other-user"), ) assert r.status_code == 403 @@ -278,7 +348,7 @@ class TestServerUserScopeSecurity: """Passing own user_id as scope_id is allowed.""" r = server_client.post( "/v1/api/memories", - json={"name": "x", "content": "y", "scope": "user", "scope_id": "test-user"}, + json=_save_body("x", "y", scope="user", scope_id="test-user"), ) assert r.status_code == 201 @@ -298,7 +368,7 @@ class TestServerScopeScopeIdValidation: def test_save_global_with_scope_id_rejected(self, server_client): r = server_client.post( "/v1/api/memories", - json={"name": "k", "content": "c", "scope": "global", "scope_id": "ws1"}, + json=_save_body("k", "c", scope="global", scope_id="ws1"), ) assert r.status_code == 400 assert "scope_id" in r.json()["error"] @@ -306,15 +376,16 @@ class TestServerScopeScopeIdValidation: def test_save_workstream_without_scope_id_rejected(self, server_client): r = server_client.post( "/v1/api/memories", - json={"name": "k", "content": "c", "scope": "workstream"}, + json=_save_body("k", "c", scope="workstream"), ) assert r.status_code == 400 assert "scope_id is required" in r.json()["error"] - def test_save_workstream_with_scope_id_ok(self, server_client): + def test_save_workstream_with_scope_id_ok(self, server_client, storage): + _seed_workstream(storage) r = server_client.post( "/v1/api/memories", - json={"name": "k", "content": "c", "scope": "workstream", "scope_id": "ws1"}, + json=_save_body("k", "c", scope="workstream", scope_id="ws1"), ) assert r.status_code == 201 @@ -380,6 +451,21 @@ class TestServerSearchMemories: r = server_client.post("/v1/api/memories/search", json={}) assert r.status_code == 400 + def test_unscoped_search_is_caller_bound(self, server_client, storage): + _seed_memory(storage, "own", "needle", scope="user", scope_id="test-user") + _seed_memory(storage, "victim", "needle", scope="user", scope_id="victim") + _seed_memory(storage, "project", "needle", scope="project", scope_id="p1") + r = server_client.post("/v1/api/memories/search", json={"query": "needle"}) + assert r.status_code == 200 + assert {row["name"] for row in r.json()["memories"]} == {"own"} + + def test_internal_scope_is_rejected(self, server_client): + r = server_client.post( + "/v1/api/memories/search", + json={"query": "x", "scope": "project", "scope_id": "p1"}, + ) + assert r.status_code == 400 + class TestServerDeleteMemory: def test_delete(self, server_client, storage): @@ -393,6 +479,7 @@ class TestServerDeleteMemory: assert r.status_code == 404 def test_delete_scoped(self, server_client, storage): + _seed_workstream(storage) _seed_memory(storage, "k", "data", scope="workstream", scope_id="ws1") # Wrong scope → not found r = server_client.delete("/v1/api/memories/k") @@ -405,6 +492,14 @@ class TestServerDeleteMemory: r = server_client.delete("/v1/api/memories/k?scope=bogus") assert r.status_code == 400 + def test_delete_is_audited(self, server_client, storage): + mid = _seed_memory(storage, "audited") + r = server_client.delete("/v1/api/memories/audited") + assert r.status_code == 200 + events = storage.list_audit_events(action="memory.delete", user_id="test-user") + assert len(events) == 1 + assert events[0]["resource_id"] == mid + # =========================================================================== # Console admin endpoint tests @@ -492,6 +587,26 @@ class TestAdminDeleteMemory: r = admin_client.delete("/v1/api/admin/memories/nonexistent-id") assert r.status_code == 404 + def test_no_audit_or_success_when_atomic_delete_misses( + self, admin_client, storage, monkeypatch + ): + mid = _seed_memory(storage, "still_here") + monkeypatch.setattr(storage, "delete_structured_memory_by_id_returning", lambda _mid: None) + + r = admin_client.delete(f"/v1/api/admin/memories/{mid}") + + assert r.status_code == 404 + assert storage.get_structured_memory(mid) is not None + assert storage.list_audit_events(action="memory.delete") == [] + + def test_storage_failure_is_500(self, admin_client, storage, monkeypatch): + def _raise(_memory_id): + raise RuntimeError("db down") + + monkeypatch.setattr(storage, "delete_structured_memory_by_id_returning", _raise) + r = admin_client.delete("/v1/api/admin/memories/m1") + assert r.status_code == 500 + # =========================================================================== # Storage: delete_structured_memory_by_id diff --git a/tests/test_memory_relevance.py b/tests/test_memory_relevance.py index ef33078c..94671449 100644 --- a/tests/test_memory_relevance.py +++ b/tests/test_memory_relevance.py @@ -1,7 +1,9 @@ """Tests for turnstone.core.memory_relevance — scoring, formatting, context extraction.""" +from typing import Any from unittest.mock import patch +from turnstone.core import auth from turnstone.core.memory_relevance import ( MemoryConfig, build_memory_context, @@ -298,6 +300,11 @@ def _make_session(fetch_limit: int = 5, relevance_k: int = 3, **kwargs: object): ) +def _execute_prepared_tool(session: Any, item: dict[str, Any]) -> tuple[str, str]: + item.setdefault("_principal_id", session._tool_prepare_principal_id()) + return item["execute"](item) + + class TestCompositionCandidateSelection: """Verify the query-aware candidate set in _init_system_messages.""" @@ -520,9 +527,11 @@ class TestMemorySearchToolExecution: """Multi-word query returns rows where ANY term matches — not all.""" from turnstone.core.memory import save_structured_memory - save_structured_memory("postgres_notes", "host=localhost port=5432") - save_structured_memory("redis_notes", "host=redis port=6379") - save_structured_memory("unrelated", "completely different") + save_structured_memory( + "postgres_notes", "host=localhost port=5432", description="Postgres notes" + ) + save_structured_memory("redis_notes", "host=redis port=6379", description="Redis notes") + save_structured_memory("unrelated", "completely different", description="Unrelated notes") session = _make_session() item = session._prepare_memory( @@ -532,12 +541,39 @@ class TestMemorySearchToolExecution: # Sanity: prepare returned a search-ready dispatch (not an error item) assert item.get("action") == "search" - call_id, msg = session._exec_memory(item) + call_id, msg = _execute_prepared_tool(session, item) assert call_id == "call-1" assert "postgres_notes" in msg # Other memories don't match any query term assert "unrelated" not in msg + def test_search_and_list_guidance_carries_the_displayed_scope(self, tmp_db, monkeypatch): + """Follow-up guidance must not drop a project result's scope.""" + from turnstone.core.memory import save_structured_memory + + save_structured_memory( + "july_digest", + "project day digest", + description="July project digest", + scope="project", + scope_id="p1", + ) + monkeypatch.setattr( + auth, + "resolve_project_access", + lambda *_a, **_k: auth.ProjectAccess(True, True, "P", "active"), + ) + session = _make_session(user_id="u1", project_id="p1") + + for args in ( + {"action": "search", "query": "digest"}, + {"action": "list"}, + ): + item = session._prepare_memory("call-1", args) + _, msg = _execute_prepared_tool(session, item) + assert "[general:project] july_digest" in msg + assert "call memory(action='get') with the displayed name and scope" in msg + class TestPerTurnSearchCache: """The per-turn cache spares redundant SQL across mid-turn rebuilds.""" @@ -545,7 +581,7 @@ class TestPerTurnSearchCache: def test_repeated_search_in_same_turn_hits_cache(self, tmp_db): from turnstone.core.memory import save_structured_memory - save_structured_memory("hello_mem", "alpha beta gamma") + save_structured_memory("hello_mem", "alpha beta gamma", description="Greeting memory") session = _make_session() with patch( "turnstone.core.session.search_visible_structured_memories", @@ -560,7 +596,7 @@ class TestPerTurnSearchCache: def test_user_turn_invalidates_cache(self, tmp_db): from turnstone.core.memory import save_structured_memory - save_structured_memory("hello_mem", "alpha") + save_structured_memory("hello_mem", "alpha", description="Greeting memory") session = _make_session() with patch( "turnstone.core.session.search_visible_structured_memories", diff --git a/tests/test_project_memory_scope.py b/tests/test_project_memory_scope.py index 5d675efc..f2c509be 100644 --- a/tests/test_project_memory_scope.py +++ b/tests/test_project_memory_scope.py @@ -1,11 +1,4 @@ -"""Phase 4: the ``project`` memory scope. - -Covers construction-time access resolution (``_project_id`` / ``_project_writable``) -and its effect on recall — ``_visible_scopes`` / ``_resolve_scope_id`` / -``_validate_scope`` — for both interactive and coordinator sessions. The ACL is -monkeypatched (it is unit-tested in ``test_project_storage.py``); here we assert -the session wiring around it. -""" +"""Actor-scoped, live ``project`` memory authorization.""" from __future__ import annotations @@ -35,10 +28,32 @@ def _session(**kwargs: Any) -> ChatSession: return ChatSession(**defaults) -class TestConstructionResolvesProjectAccess: - """Construction resolves the attached project through a single - ``resolve_project_access`` call; recall is gated on read access AND a - non-archived project.""" +def _execute_prepared_tool( + session: ChatSession, + item: dict[str, Any], +) -> tuple[str, str | list[dict[str, Any]]]: + item.setdefault("_principal_id", session._tool_prepare_principal_id()) + return item["execute"](item) + + +def _project_session( + monkeypatch: pytest.MonkeyPatch, + *, + writable: bool = True, + kind: WorkstreamKind = WorkstreamKind.INTERACTIVE, + user_id: str = "u1", +) -> ChatSession: + """Construct an attached session whose live ACL stays controllable.""" + monkeypatch.setattr( + auth, + "resolve_project_access", + lambda *_a, **_k: auth.ProjectAccess(True, writable, "P", "active"), + ) + return _session(user_id=user_id, ws_id="ws1", kind=kind, project_id="p1") + + +class TestLiveProjectAccess: + """Each access snapshot resolves the attachment for the current actor.""" def _access(self, can_read: bool, can_write: bool, state: str = "active") -> object: return auth.ProjectAccess(can_read, can_write, "P", state) @@ -48,9 +63,10 @@ class TestConstructionResolvesProjectAccess: auth, "resolve_project_access", lambda *a, **k: self._access(True, True) ) s = _session(user_id="u1", project_id="p1") - assert s._project_id == "p1" - assert s._project_writable is True - assert s._project_name == "P" + access = s._memory_access() + assert access.project_id == "p1" + assert access.project_writable is True + assert access.project_name == "P" def test_read_only_member(self, monkeypatch: pytest.MonkeyPatch) -> None: # Read access but no write (e.g. a non-member reading a public project). @@ -58,16 +74,19 @@ class TestConstructionResolvesProjectAccess: auth, "resolve_project_access", lambda *a, **k: self._access(True, False) ) s = _session(user_id="u1", project_id="p1") - assert s._project_id == "p1" - assert s._project_writable is False + access = s._memory_access() + assert access.project_id == "p1" + assert access.project_writable is False def test_denied_without_access(self, monkeypatch: pytest.MonkeyPatch) -> None: monkeypatch.setattr( auth, "resolve_project_access", lambda *a, **k: self._access(False, False) ) s = _session(user_id="u1", project_id="p1") - assert s._project_id == "" - assert s._project_writable is False + access = s._memory_access() + assert access.attached_project_id == "p1" + assert access.project_id == "" + assert access.project_writable is False def test_archived_project_not_recalled(self, monkeypatch: pytest.MonkeyPatch) -> None: # Full access but archived → not recalled (the owner still reaches it via @@ -76,13 +95,17 @@ class TestConstructionResolvesProjectAccess: auth, "resolve_project_access", lambda *a, **k: self._access(True, True, "archived") ) s = _session(user_id="u1", project_id="p1") - assert s._project_id == "" - assert s._project_writable is False + access = s._memory_access() + assert access.attached_project_id == "p1" + assert access.project_id == "" + assert access.project_writable is False def test_no_project_id_is_inert(self) -> None: s = _session(user_id="u1") - assert s._project_id == "" - assert s._project_writable is False + access = s._memory_access() + assert access.attached_project_id == "" + assert access.project_id == "" + assert access.project_writable is False def test_unauthenticated_never_resolves(self, monkeypatch: pytest.MonkeyPatch) -> None: # Even if the ACL would allow it, an empty user_id short-circuits before @@ -91,13 +114,16 @@ class TestConstructionResolvesProjectAccess: auth, "resolve_project_access", lambda *a, **k: self._access(True, True) ) s = _session(user_id="", project_id="p1") - assert s._project_id == "" + access = s._memory_access() + assert access.attached_project_id == "p1" + assert access.project_id == "" class TestProjectRecall: - def test_interactive_visible_scopes_includes_project(self) -> None: - s = _session(user_id="u1", ws_id="ws1") - s._project_id = "p1" + def test_interactive_visible_scopes_includes_project( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + s = _project_session(monkeypatch) scopes = s._visible_scopes() assert ("project", "p1") in scopes assert ("global", "") in scopes @@ -107,9 +133,10 @@ class TestProjectRecall: s = _session(user_id="u1", ws_id="ws1") assert all(scope != "project" for scope, _ in s._visible_scopes()) - def test_coordinator_adds_project_keeps_isolation(self) -> None: - s = _session(user_id="u1", kind=WorkstreamKind.COORDINATOR) - s._project_id = "p1" + def test_coordinator_adds_project_keeps_isolation( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + s = _project_session(monkeypatch, kind=WorkstreamKind.COORDINATOR) scopes = s._visible_scopes() assert ("coordinator", "u1") in scopes assert ("project", "p1") in scopes @@ -118,25 +145,24 @@ class TestProjectRecall: def test_visible_scopes_omits_empty_project(self) -> None: s = _session(user_id="u1", ws_id="ws1") - s._project_id = "" assert all(scope != "project" for scope, _ in s._visible_scopes()) class TestProjectScopeResolutionAndValidation: - def test_resolve_scope_id_project(self) -> None: - s = _session(user_id="u1") - s._project_id = "p1" + def test_resolve_scope_id_project(self, monkeypatch: pytest.MonkeyPatch) -> None: + s = _project_session(monkeypatch) assert s._resolve_scope_id("project") == "p1" - def test_validate_requires_attachment(self) -> None: + def test_validate_requires_attachment(self, monkeypatch: pytest.MonkeyPatch) -> None: s = _session(user_id="u1") assert s._validate_scope("project", "cid") is not None # not attached → rejected - s._project_id = "p1" - assert s._validate_scope("project", "cid") is None + attached = _project_session(monkeypatch) + assert attached._validate_scope("project", "cid") is None - def test_coordinator_allows_project_rejects_global(self) -> None: - s = _session(user_id="u1", kind=WorkstreamKind.COORDINATOR) - s._project_id = "p1" + def test_coordinator_allows_project_rejects_global( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + s = _project_session(monkeypatch, kind=WorkstreamKind.COORDINATOR) assert s._validate_scope("project", "cid") is None # project allowed for coord assert s._validate_scope("global", "cid") is not None # global still rejected @@ -172,78 +198,328 @@ class TestProjectInSystemContext: class TestProjectWriteGate: """The save AND delete memory paths block writes to a project the session can read but not write (a read-only member of a public project). Construction - resolves ``_project_writable``; these drive the preparer to assert the gate - actually fires (the resolution-level check lives in - ``TestConstructionResolvesProjectAccess``).""" + resolves live access; these drive the preparer to assert the gate actually + fires (the resolution-level check lives in ``TestLiveProjectAccess``).""" - def _attached(self, *, writable: bool) -> ChatSession: - s = _session(user_id="u1") - s._project_id = "p1" - s._project_writable = writable - return s + def _attached(self, monkeypatch: pytest.MonkeyPatch, *, writable: bool) -> ChatSession: + return _project_session(monkeypatch, writable=writable) - def test_save_blocked_when_read_only(self) -> None: - s = self._attached(writable=False) + def test_save_blocked_when_read_only(self, monkeypatch: pytest.MonkeyPatch) -> None: + s = self._attached(monkeypatch, writable=False) out = s._prepare_memory( - "cid", {"action": "save", "scope": "project", "name": "k", "content": "v"} + "cid", + { + "action": "save", + "scope": "project", + "name": "k", + "content": "v", + "description": "Test memory", + }, ) - assert "read-only access to this project" in out.get("error", "") + assert "read-only access to the attached project" in out.get("error", "") - def test_save_allowed_when_writable(self) -> None: - s = self._attached(writable=True) + def test_save_allowed_when_writable(self, monkeypatch: pytest.MonkeyPatch) -> None: + s = self._attached(monkeypatch, writable=True) out = s._prepare_memory( - "cid", {"action": "save", "scope": "project", "name": "k", "content": "v"} + "cid", + { + "action": "save", + "scope": "project", + "name": "k", + "content": "v", + "description": "Test memory", + }, ) assert "error" not in out assert out.get("execute") is not None # would proceed to the save exec - def test_delete_blocked_when_read_only(self) -> None: - s = self._attached(writable=False) + def test_delete_blocked_when_read_only(self, monkeypatch: pytest.MonkeyPatch) -> None: + s = self._attached(monkeypatch, writable=False) out = s._prepare_memory("cid", {"action": "delete", "scope": "project", "name": "k"}) - assert "read-only access to this project" in out.get("error", "") + assert "read-only access to the attached project" in out.get("error", "") - def test_delete_allowed_when_writable(self) -> None: - s = self._attached(writable=True) + def test_delete_allowed_when_writable(self, monkeypatch: pytest.MonkeyPatch) -> None: + s = self._attached(monkeypatch, writable=True) out = s._prepare_memory("cid", {"action": "delete", "scope": "project", "name": "k"}) assert "error" not in out assert out.get("execute") is not None -class TestProjectDefaultSaveScope: - """A writable attached project becomes the DEFAULT save scope (both kinds); - a read-only or unattached session keeps the kind default.""" +class TestActingPrincipalProjectAuthority: + @staticmethod + def _tool_call(call_id: str, **arguments: Any) -> dict[str, Any]: + import json - def test_writable_project_is_default(self) -> None: - s = _session(user_id="u1") - s._project_id = "p1" - s._project_writable = True + return { + "id": call_id, + "function": {"name": "memory", "arguments": json.dumps(arguments)}, + } + + def test_guest_cannot_inherit_owner_project_access( + self, tmp_db: str, monkeypatch: pytest.MonkeyPatch + ) -> None: + def resolve(user_id: str, _project_id: str, **_kwargs: Any) -> auth.ProjectAccess: + if user_id == "owner": + return auth.ProjectAccess(True, True, "Owner Project", "active") + return auth.ProjectAccess(False, False, "", "") + + monkeypatch.setattr(auth, "resolve_project_access", resolve) + session = _session(user_id="owner", ws_id="shared", project_id="p1") + session.bind_acting_user("guest") + + assert all(scope != "project" for scope, _ in session._visible_scopes()) + for action in ("get", "save", "delete"): + arguments: dict[str, Any] = { + "action": action, + "name": "owner_secret", + "scope": "project", + } + if action == "save": + arguments["content"] = "guest write" + arguments["description"] = "Guest write attempt" + item = session._prepare_tool(self._tool_call(action, **arguments)) + assert item["_principal_id"] == "guest" + assert "error" in item + assert "acting user cannot access" in item["error"] + + def test_project_delete_revalidates_prepared_principal_and_live_acl( + self, tmp_db: str, monkeypatch: pytest.MonkeyPatch + ) -> None: + from turnstone.core.memory import ( + get_structured_memory_by_name, + save_structured_memory, + ) + + guest_access = {"value": auth.ProjectAccess(True, True, "Shared", "active")} + + def resolve(user_id: str, _project_id: str, **_kwargs: Any) -> auth.ProjectAccess: + if user_id == "guest": + return guest_access["value"] + return auth.ProjectAccess(True, True, "Shared", "active") + + monkeypatch.setattr(auth, "resolve_project_access", resolve) + save_structured_memory( + "shared_secret", + "keep", + description="Shared project secret", + scope="project", + scope_id="p1", + ) + session = _session(user_id="owner", ws_id="shared", project_id="p1") + session.bind_acting_user("guest") + item = session._prepare_tool( + self._tool_call( + "delete", + action="delete", + name="shared_secret", + scope="project", + ) + ) + assert "error" not in item + assert item["_principal_id"] == "guest" + + guest_access["value"] = auth.ProjectAccess(False, False, "", "") + session.bind_acting_user("owner") + _, message = _execute_prepared_tool(session, item) + + assert "acting user cannot access" in message + assert get_structured_memory_by_name("shared_secret", "project", "p1") is not None + + def test_archived_project_is_removed_from_live_visibility( + self, tmp_db: str, monkeypatch: pytest.MonkeyPatch + ) -> None: + project_state = {"value": "active"} + + monkeypatch.setattr( + auth, + "resolve_project_access", + lambda *_args, **_kwargs: auth.ProjectAccess( + True, True, "Shared", project_state["value"] + ), + ) + session = _session(user_id="owner", ws_id="shared", project_id="p1") + assert ("project", "p1") in session._visible_scopes() + + project_state["value"] = "archived" + + assert all(scope != "project" for scope, _ in session._visible_scopes()) + item = session._prepare_memory( + "get", {"action": "get", "name": "anything", "scope": "project"} + ) + assert "error" in item + assert "active attached project" in item["error"] + + +class TestProjectDefaultSaveScope: + """An attachment is the inherited target even when it is read-only.""" + + def test_writable_project_is_default(self, monkeypatch: pytest.MonkeyPatch) -> None: + s = _project_session(monkeypatch) assert s._default_memory_scope() == "project" - def test_read_only_project_keeps_kind_default(self) -> None: - s = _session(user_id="u1") - s._project_id = "p1" - s._project_writable = False - assert s._default_memory_scope() == "global" + def test_read_only_project_remains_inherited_target( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + s = _project_session(monkeypatch, writable=False) + assert s._default_memory_scope() == "project" def test_no_project_keeps_kind_default(self) -> None: assert _session(user_id="u1")._default_memory_scope() == "global" - def test_coordinator_writable_project_is_default(self) -> None: - s = _session(user_id="u1", kind=WorkstreamKind.COORDINATOR) - s._project_id = "p1" - s._project_writable = True + def test_coordinator_writable_project_is_default(self, monkeypatch: pytest.MonkeyPatch) -> None: + s = _project_session(monkeypatch, kind=WorkstreamKind.COORDINATOR) assert s._default_memory_scope() == "project" def test_coordinator_without_project_is_coordinator(self) -> None: s = _session(user_id="u1", kind=WorkstreamKind.COORDINATOR) assert s._default_memory_scope() == "coordinator" - def test_save_without_scope_lands_in_project(self) -> None: + def test_save_without_scope_lands_in_project(self, monkeypatch: pytest.MonkeyPatch) -> None: # End-to-end: an unscoped save in a writable-project session resolves to # scope=project / scope_id=project_id (not the global default). - s = _session(user_id="u1") - s._project_id = "p1" - s._project_writable = True - out = s._prepare_memory("cid", {"action": "save", "name": "k", "content": "v"}) + s = _project_session(monkeypatch) + out = s._prepare_memory( + "cid", + { + "action": "save", + "name": "k", + "content": "v", + "description": "Test memory", + }, + ) assert out.get("scope") == "project" assert out.get("scope_id") == "p1" + + +class TestProjectDefaultGetDeleteScope: + """An attached project is the inherited get/delete target. + + This aligns the name-based lifecycle: a memory saved without an explicit + scope can be fetched or removed the same way while the workstream remains + attached to that project. + """ + + @staticmethod + def _attached( + monkeypatch: pytest.MonkeyPatch, + *, + writable: bool = True, + kind: WorkstreamKind = WorkstreamKind.INTERACTIVE, + ) -> ChatSession: + return _project_session(monkeypatch, writable=writable, kind=kind) + + def test_get_without_scope_targets_project(self, monkeypatch: pytest.MonkeyPatch) -> None: + # Read access is sufficient for the inherited get target; writability + # only controls save/delete. + s = self._attached(monkeypatch, writable=False) + item = s._prepare_memory("cid", {"action": "get", "name": "k"}) + assert item["scopes_to_try"] == [("project", "p1")] + + def test_delete_without_scope_targets_writable_project( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + s = self._attached(monkeypatch) + item = s._prepare_memory("cid", {"action": "delete", "name": "k"}) + assert item["scopes_to_try"] == [("project", "p1")] + + def test_delete_without_scope_rejects_read_only_project( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + s = self._attached(monkeypatch, writable=False) + item = s._prepare_memory("cid", {"action": "delete", "name": "k"}) + assert "read-only access to the attached project" in item.get("error", "") + + def test_read_only_project_does_not_block_explicit_other_scope( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + s = self._attached(monkeypatch, writable=False) + item = s._prepare_memory( + "cid", + {"action": "delete", "name": "k", "scope": "global"}, + ) + assert "error" not in item + assert item["scopes_to_try"] == [("global", "")] + + def test_coordinator_inherits_project_too(self, monkeypatch: pytest.MonkeyPatch) -> None: + s = self._attached(monkeypatch, kind=WorkstreamKind.COORDINATOR) + get_item = s._prepare_memory("get", {"action": "get", "name": "k"}) + delete_item = s._prepare_memory("delete", {"action": "delete", "name": "k"}) + assert get_item["scopes_to_try"] == [("project", "p1")] + assert delete_item["scopes_to_try"] == [("project", "p1")] + + def test_unscoped_get_and_delete_round_trip_project_memory( + self, tmp_db: str, monkeypatch: pytest.MonkeyPatch + ) -> None: + from turnstone.core.memory import ( + get_structured_memory_by_name, + save_structured_memory, + ) + + row, _ = save_structured_memory( + "july_digest", + "full digest", + description="July digest", + scope="project", + scope_id="p1", + ) + assert row is not None + + s = self._attached(monkeypatch) + get_item = s._prepare_memory("get", {"action": "get", "name": "july_digest"}) + _, get_msg = _execute_prepared_tool(s, get_item) + assert "[general:project] july_digest" in get_msg + assert "full digest" in get_msg + + delete_item = s._prepare_memory("delete", {"action": "delete", "name": "july_digest"}) + _, delete_msg = _execute_prepared_tool(s, delete_item) + assert "Deleted memory 'july_digest' (scope=project)" in delete_msg + assert get_structured_memory_by_name("july_digest", "project", "p1") is None + + def test_wrong_explicit_scope_hints_at_attached_project( + self, tmp_db: str, monkeypatch: pytest.MonkeyPatch + ) -> None: + from turnstone.core.memory import save_structured_memory + + row, _ = save_structured_memory( + "july_digest", + "full digest", + description="July digest", + scope="project", + scope_id="p1", + ) + assert row is not None + + s = self._attached(monkeypatch) + for action in ("get", "delete"): + item = s._prepare_memory( + action, + {"action": action, "name": "july_digest", "scope": "global"}, + ) + _, msg = _execute_prepared_tool(s, item) + assert "not found (scope=global)" in msg + assert "exists in scope='project'" in msg + assert "retry with scope='project'" in msg + + def test_project_default_miss_hints_at_other_visible_scope( + self, tmp_db: str, monkeypatch: pytest.MonkeyPatch + ) -> None: + from turnstone.core.memory import save_structured_memory + + row, _ = save_structured_memory( + "shared_runbook", + "global content", + description="Shared runbook", + scope="global", + ) + assert row is not None + + s = self._attached(monkeypatch) + for action in ("get", "delete"): + item = s._prepare_memory( + action, + {"action": action, "name": "shared_runbook"}, + ) + _, msg = _execute_prepared_tool(s, item) + assert "not found (scope=project)" in msg + assert "exists in scope='global'" in msg + assert "retry with scope='global'" in msg diff --git a/tests/test_project_storage.py b/tests/test_project_storage.py index cdb6b6c2..324aa813 100644 --- a/tests/test_project_storage.py +++ b/tests/test_project_storage.py @@ -68,9 +68,9 @@ class TestProjectStore: # a sibling project's nor other scopes' rows. backend.create_project("p1", "A", "u1") backend.create_project("p2", "B", "u1") - backend.create_structured_memory("m1", "k", "", "general", "project", "p1", "v") - backend.create_structured_memory("m2", "k", "", "general", "project", "p2", "v") - backend.create_structured_memory("m3", "k", "", "general", "user", "u1", "v") + backend.create_structured_memory("m1", "k", "Test memory", "general", "project", "p1", "v") + backend.create_structured_memory("m2", "k", "Test memory", "general", "project", "p2", "v") + backend.create_structured_memory("m3", "k", "Test memory", "general", "user", "u1", "v") assert backend.delete_project("p1") assert backend.get_structured_memory("m1") is None # purged assert backend.get_structured_memory("m2") is not None # sibling project intact diff --git a/tests/test_sdk_server.py b/tests/test_sdk_server.py index d02fd1da..0e113fac 100644 --- a/tests/test_sdk_server.py +++ b/tests/test_sdk_server.py @@ -361,6 +361,62 @@ async def test_logout(): assert resp.status == "ok" +# --------------------------------------------------------------------------- +# Memories +# --------------------------------------------------------------------------- + + +@pytest.mark.anyio +async def test_save_memory_requires_and_sends_description(): + captured: dict = {} + + def handler(request: httpx.Request) -> httpx.Response: + captured.update(json.loads(request.content)) + return _json_response( + { + "memory_id": "m1", + "name": "deployment_process", + "description": captured["description"], + "type": "general", + "scope": "global", + "scope_id": "", + "content": "Deploy from main", + "created": "2026-08-11T00:00:00", + "updated": "2026-08-11T00:00:00", + }, + status=201, + ) + + transport = httpx.MockTransport(handler) + async with httpx.AsyncClient(transport=transport, base_url="http://test") as hc: + client = AsyncTurnstoneServer(httpx_client=hc) + memory = await client.save_memory( + "deployment_process", + "Deploy from main", + description=" Production deployment workflow ", + ) + + assert captured["description"] == "Production deployment workflow" + assert memory.description == "Production deployment workflow" + + +@pytest.mark.anyio +@pytest.mark.parametrize("description", [None, "", " "]) +async def test_save_memory_rejects_empty_description(description): + def unexpected_request(_request: httpx.Request) -> httpx.Response: + raise AssertionError("invalid memory must not reach the server") + + transport = httpx.MockTransport(unexpected_request) + async with httpx.AsyncClient(transport=transport, base_url="http://test") as hc: + client = AsyncTurnstoneServer(httpx_client=hc) + with pytest.raises(ValueError, match="description is required"): + await client.save_memory( + "deployment_process", + "Deploy from main", + description=description, # type: ignore[arg-type] + ) + + # --------------------------------------------------------------------------- # Health # --------------------------------------------------------------------------- diff --git a/tests/test_session.py b/tests/test_session.py index 40077def..2e51f815 100644 --- a/tests/test_session.py +++ b/tests/test_session.py @@ -29,7 +29,12 @@ from turnstone.core.model_turn import ( provider_extra_params, serialized_tool_chars, ) -from turnstone.core.session import _IMAGE_EXTENSIONS, _IMAGE_SIZE_CAP, ChatSession +from turnstone.core.session import ( + _IMAGE_EXTENSIONS, + _IMAGE_SIZE_CAP, + _MEMORY_MIXED_BATCH_ERROR, + ChatSession, +) from turnstone.core.trajectory import ( Role, Turn, @@ -129,6 +134,15 @@ def _make_session( return session +def _execute_prepared_tool( + session: ChatSession, + item: dict[str, Any], +) -> tuple[str, str | list[dict[str, Any]]]: + """Mirror the dispatch boundary for tests that call a preparer directly.""" + item.setdefault("_principal_id", session._tool_prepare_principal_id()) + return item["execute"](item) + + @contextlib.contextmanager def _send_with_mocks(session, responses, mock_execute, **extra_patches): """Stand up the mock context that the queued-message ``send()`` tests share. @@ -458,12 +472,20 @@ class TestTaskExec: "func_name": "task_agent", "needs_approval": True, "execute": execute, + "_needs_origin_context": True, + "_requires_fresh_system_prefix": True, } + + def prepare(_tool_call): + seen["prepare"] = session._tool_prepare_principal_id() + return item + judge = MagicMock(side_effect=evaluate_intent) with ( - patch.object(session, "_safe_prepare_tool", return_value=item), + patch.object(session, "_safe_prepare_tool", side_effect=prepare), patch.object(session, "_evaluate_intent", judge), patch.object(session.ui, "approve_tools", side_effect=approve_tools), + patch.object(session, "_ensure_system_prefix_fresh") as ensure_system_prefix, ): session._execute_tools( [{"id": "c1", "function": {"name": "task_agent", "arguments": "{}"}}], @@ -471,9 +493,14 @@ class TestTaskExec: my_generation=generation, ) + assert seen["prepare"] == "user-a" assert seen["worker"] == "user-a" assert seen["generation"] == generation assert seen["event"] is generation_event + ensure_system_prefix.assert_called_once_with( + principal_id="user-a", + origin_generation=generation, + ) assert judge.call_args.kwargs["principal_id"] == "user-a" assert seen["execution_item"] is not item approval_witness = seen["approval_item"]["_approval_cancel_witness"] @@ -6199,6 +6226,96 @@ class TestSafePrepareTool: assert "RuntimeError" in output +class TestToolBatchPolicy: + @staticmethod + def _tool_calls(count: int) -> list[dict[str, Any]]: + return [ + { + "id": f"call_{index}", + "function": {"name": "state_tool", "arguments": "{}"}, + } + for index in range(count) + ] + + def test_mixed_read_write_batch_is_rejected(self, tmp_db): + session = _make_session() + executed: list[str] = [] + + def execute(item): + executed.append(item["call_id"]) + return item["call_id"], "unexpected" + + items = [ + { + "call_id": "call_0", + "func_name": "state_tool", + "execute": execute, + "needs_approval": False, + "_batch_policy": { + "group": "state", + "access": "write", + "mixed_access_error": "Error: state reads and writes cannot run together", + "serialize": True, + }, + }, + { + "call_id": "call_1", + "func_name": "state_tool", + "execute": execute, + "needs_approval": False, + "_batch_policy": { + "group": "state", + "access": "read", + "mixed_access_error": "Error: state reads and writes cannot run together", + }, + }, + ] + with ( + patch.object(session, "_safe_prepare_tool", side_effect=items), + patch.object(session.ui, "approve_tools", return_value=(True, None)), + ): + results, _ = session._execute_tools(self._tool_calls(2)) + + assert executed == [] + assert all("cannot run together" in str(result) for _, result in results) + + def test_write_batch_executes_serially_in_model_order(self, tmp_db): + session = _make_session() + executed: list[str] = [] + + def execute(item): + executed.append(item["call_id"]) + return item["call_id"], "ok" + + items = [ + { + "call_id": f"call_{index}", + "func_name": "state_tool", + "execute": execute, + "needs_approval": False, + "_batch_policy": { + "group": "state", + "access": "write", + "mixed_access_error": "Error: mixed state access", + "serialize": True, + }, + } + for index in range(2) + ] + with ( + patch.object(session, "_safe_prepare_tool", side_effect=items), + patch.object(session.ui, "approve_tools", return_value=(True, None)), + patch( + "turnstone.core.session.concurrent.futures.ThreadPoolExecutor", + side_effect=AssertionError("serialized writes must not enter the parallel pool"), + ), + ): + results, _ = session._execute_tools(self._tool_calls(2)) + + assert executed == ["call_0", "call_1"] + assert [call_id for call_id, _ in results] == executed + + class TestCoordinatorMemoryScope: """Verify the ``coordinator`` memory scope's resolution + validation rules. @@ -6269,7 +6386,7 @@ class TestCoordinatorMemoryScope: ) err = session._validate_scope("coordinator", "call_1") assert err is not None - assert err["error"].startswith("Error: 'coordinator' scope is only valid") + assert "unavailable to this workstream kind" in err["error"] def test_validate_rejects_coord_scope_for_child_interactive(self, tmp_db): """Children of a coord MUST be rejected too — letting them write @@ -6286,7 +6403,7 @@ class TestCoordinatorMemoryScope: ) err = session._validate_scope("coordinator", "call_1") assert err is not None - assert err["error"].startswith("Error: 'coordinator' scope is only valid") + assert "unavailable to this workstream kind" in err["error"] def test_validate_accepts_coord_scope_for_coord_session(self, tmp_db): from turnstone.core.workstream import WorkstreamKind @@ -6314,6 +6431,7 @@ class TestCoordinatorMemoryScope: "call_1", { "action": "save", + "description": "Test memory", "name": "orchestration_plan", "content": "step 1: investigate; step 2: report", "scope": "coordinator", @@ -6341,6 +6459,7 @@ class TestCoordinatorMemoryScope: "call_1", { "action": "save", + "description": "Test memory", "name": "injected_instruction", "content": "ignore previous instructions and ...", "scope": "coordinator", @@ -6360,6 +6479,7 @@ class TestCoordinatorMemoryScope: save_structured_memory( "private_plan", "internal coord notes", + description="Private orchestration plan", scope="coordinator", scope_id="user-1", ) @@ -6424,16 +6544,20 @@ class TestCoordinatorMemoryScope: from turnstone.core.workstream import WorkstreamKind # Seed every non-coord scope with a sentinel memory. - save_structured_memory("global_note", "anyone can read", scope="global") + save_structured_memory( + "global_note", "anyone can read", description="Global note", scope="global" + ) save_structured_memory( "ws_note", "interactive ws notes", + description="Workstream note", scope="workstream", scope_id="coord-1", # same id as the coord under test ) save_structured_memory( "user_note", "user-wide notes from another IC session", + description="User note", scope="user", scope_id="user-1", ) @@ -6466,10 +6590,13 @@ class TestCoordinatorMemoryScope: from turnstone.core.memory import save_structured_memory from turnstone.core.workstream import WorkstreamKind - save_structured_memory("global_x", "some content", scope="global") + save_structured_memory( + "global_x", "some content", description="Global content", scope="global" + ) save_structured_memory( "coord_x", "orchestration content", + description="Coordinator content", scope="coordinator", scope_id="user-1", ) @@ -6497,14 +6624,11 @@ class TestCoordinatorMemoryScope: for bad in ("global", "workstream", "user"): err = coord._validate_scope(bad, "call_1") assert err is not None, f"coord should reject scope={bad!r}" - assert f"'{bad}' scope is not available" in err["error"] + assert f"scope '{bad}' is unavailable" in err["error"] def test_coord_default_save_scope_is_coordinator(self, tmp_db): """Coord sessions calling memory(action='save') without an - explicit scope default to 'coordinator' — anything else would - either land in a namespace the coord can't read back from - (workstream/user) or fall back to global which the new - visibility rules also exclude.""" + explicit scope target the coordinator namespace.""" from turnstone.core.workstream import WorkstreamKind coord = _make_session( @@ -6514,17 +6638,20 @@ class TestCoordinatorMemoryScope: ) item = coord._prepare_memory( "call_1", - {"action": "save", "name": "auto_scope", "content": "x"}, + { + "action": "save", + "name": "auto_scope", + "content": "x", + "description": "Automatic scope test", + }, ) assert "error" not in item assert item["scope"] == "coordinator" assert item["scope_id"] == "user-1" - def test_coord_implicit_walk_only_coordinator(self, tmp_db): + def test_coord_inherited_get_targets_only_coordinator(self, tmp_db): """Coord ``memory(action='get')`` with no explicit scope must - walk only the coordinator scope — the IC walk - (workstream → user → global) would be wasted lookups against - rows the coord can't see.""" + target only the coordinator scope.""" from turnstone.core.workstream import WorkstreamKind coord = _make_session( @@ -6539,10 +6666,8 @@ class TestCoordinatorMemoryScope: assert "error" not in item assert [s for s, _ in item["scopes_to_try"]] == ["coordinator"] - def test_ic_implicit_walk_unchanged(self, tmp_db): - """Interactive sessions retain the narrowest-to-widest walk: - workstream → user → global. Coord scope is excluded — IC - sessions can't see/write it anyway.""" + def test_ic_unscoped_get_uses_single_global_target(self, tmp_db): + """Without a project, interactive save/get/delete all inherit global.""" from turnstone.core.workstream import WorkstreamKind ic = _make_session( @@ -6555,8 +6680,7 @@ class TestCoordinatorMemoryScope: {"action": "get", "name": "anything"}, ) assert "error" not in item - scopes = [s for s, _ in item["scopes_to_try"]] - assert scopes == ["workstream", "user", "global"] + assert item["scopes_to_try"] == [("global", "")] def test_coord_memory_persists_across_sessions(self, tmp_db): """End-to-end through the real save lane: a memory saved by one @@ -6575,13 +6699,14 @@ class TestCoordinatorMemoryScope: "call_1", { "action": "save", + "description": "Test memory", "name": "deploy_runbook", "content": "drain node before rotating certs", "scope": "coordinator", }, ) assert "error" not in item - result = item["execute"](item) + result = _execute_prepared_tool(first, item) assert "Saved" in str(result) or "saved" in str(result).lower() # Brand-new coordinator session, new ws_id, same user. @@ -6595,7 +6720,7 @@ class TestCoordinatorMemoryScope: {"action": "get", "name": "deploy_runbook"}, ) assert "error" not in get_item - out = str(get_item["execute"](get_item)) + out = str(_execute_prepared_tool(second, get_item)) assert "drain node before rotating certs" in out def test_coordinator_session_requires_user_id(self, tmp_db): @@ -6636,11 +6761,17 @@ class TestCoordinatorMemoryScope: coord._user_id = "" # simulate a constructor-bypassing double err = coord._validate_scope("coordinator", "call_1") assert err is not None - assert "requires authenticated user identity" in err["error"] + assert "requires an authenticated acting user" in err["error"] assert coord._coordinator_scope_id() == "" item = coord._prepare_memory( "call_1", - {"action": "save", "name": "x", "content": "y", "scope": "coordinator"}, + { + "action": "save", + "name": "x", + "content": "y", + "description": "Authentication backstop test", + "scope": "coordinator", + }, ) assert "error" in item @@ -6654,6 +6785,7 @@ class TestCoordinatorMemoryScope: save_structured_memory( "other_users_row", "must not leak", + description="Another user's row", scope="coordinator", scope_id="user-9", ) @@ -6682,7 +6814,48 @@ class TestMemoryToolAudit: return get_storage().list_audit_events(action=action) - def test_save_new_emits_memory_save(self, tmp_db): + def test_preparer_declares_generic_batch_policy(self, tmp_db): + session = _make_session(ws_id="ws-1", user_id="user-1") + saved = session._prepare_memory( + "save-call", + { + "action": "save", + "name": "fact_one", + "content": "alpha content", + "description": "Alpha fact", + }, + ) + fetched = session._prepare_memory( + "get-call", + {"action": "get", "name": "fact_one"}, + ) + + assert saved["_batch_policy"] == { + "group": "memory", + "access": "write", + "mixed_access_error": _MEMORY_MIXED_BATCH_ERROR, + "serialize": True, + } + assert fetched["_batch_policy"] == { + "group": "memory", + "access": "read", + "mixed_access_error": _MEMORY_MIXED_BATCH_ERROR, + "serialize": False, + } + + def test_unstamped_executor_does_not_inherit_session_actor(self, tmp_db): + session = _make_session(ws_id="ws-1", user_id="user-1") + item = session._prepare_memory( + "get-call", + {"action": "get", "name": "private_fact", "scope": "user"}, + ) + + _call_id, message = item["execute"](item) + + assert "requires an authenticated acting user" in message + + @pytest.mark.parametrize("description", [None, "", " "]) + def test_save_requires_non_empty_description(self, tmp_db, description): session = _make_session(ws_id="ws-1", user_id="user-1") item = session._prepare_memory( "call_1", @@ -6690,12 +6863,27 @@ class TestMemoryToolAudit: "action": "save", "name": "fact_one", "content": "alpha content", + "description": description, + }, + ) + assert "error" in item + assert "description' must be non-empty" in item["error"] + + def test_save_new_emits_memory_save(self, tmp_db): + session = _make_session(ws_id="ws-1", user_id="user-1") + item = session._prepare_memory( + "call_1", + { + "action": "save", + "description": "Test memory", + "name": "fact_one", + "content": "alpha content", "scope": "user", "type": "reference", }, ) assert "error" not in item - session._exec_memory(item) + _execute_prepared_tool(session, item) rows = self._audit_rows("memory.save") assert len(rows) == 1 @@ -6712,6 +6900,71 @@ class TestMemoryToolAudit: # The "create" path must NOT also stamp an update row. assert self._audit_rows("memory.update") == [] + def test_prepared_user_save_stays_bound_to_acting_principal(self, tmp_db): + from turnstone.core.memory import get_structured_memory_by_name + + session = _make_session(ws_id="shared", user_id="owner") + session.bind_acting_user("guest") + item = session._prepare_tool( + { + "id": "call_1", + "function": { + "name": "memory", + "arguments": json.dumps( + { + "action": "save", + "description": "Test memory", + "name": "private_note", + "content": "guest content", + "scope": "user", + } + ), + }, + } + ) + assert item["_principal_id"] == "guest" + assert item["scope_id"] == "guest" + + session.bind_acting_user("owner") + _, message = _execute_prepared_tool(session, item) + + assert "Saved memory" in message + assert get_structured_memory_by_name("private_note", "user", "guest") is not None + assert get_structured_memory_by_name("private_note", "user", "owner") is None + rows = self._audit_rows("memory.save") + assert len(rows) == 1 + assert rows[0]["user_id"] == "guest" + + def test_guest_user_get_does_not_probe_owner_namespace(self, tmp_db): + from turnstone.core.memory import save_structured_memory + + save_structured_memory( + "owner_secret", + "must not leak", + description="Owner secret", + scope="user", + scope_id="owner", + ) + session = _make_session(ws_id="shared", user_id="owner") + session.bind_acting_user("guest") + item = session._prepare_tool( + { + "id": "call_1", + "function": { + "name": "memory", + "arguments": json.dumps( + {"action": "get", "name": "owner_secret", "scope": "user"} + ), + }, + } + ) + + _, message = _execute_prepared_tool(session, item) + + assert "not found" in message + assert "must not leak" not in message + assert "exists in scope" not in message + def test_save_global_scope_emits_empty_scope_id(self, tmp_db): """Global memories have no scope_id — the audit row's detail must still carry the key (with value ``""``) so a forensic @@ -6722,13 +6975,14 @@ class TestMemoryToolAudit: "call_1", { "action": "save", + "description": "Test memory", "name": "fact_global", "content": "shared content", "scope": "global", }, ) assert "error" not in item - session._exec_memory(item) + _execute_prepared_tool(session, item) rows = self._audit_rows("memory.save") assert len(rows) == 1 @@ -6744,13 +6998,14 @@ class TestMemoryToolAudit: "call_x", { "action": "save", + "description": "Test memory", "name": "fact_one", "content": content, "scope": "user", "type": "reference", }, ) - session._exec_memory(item) + _execute_prepared_tool(session, item) saves = self._audit_rows("memory.save") updates = self._audit_rows("memory.update") @@ -6765,20 +7020,21 @@ class TestMemoryToolAudit: "call_1", { "action": "save", + "description": "Test memory", "name": "fact_one", "content": "alpha", "scope": "user", "type": "reference", }, ) - session._exec_memory(save_item) + _execute_prepared_tool(session, save_item) saved_memory_id = self._audit_rows("memory.save")[0]["resource_id"] delete_item = session._prepare_memory( "call_2", {"action": "delete", "name": "fact_one", "scope": "user"}, ) - _, msg = session._exec_memory(delete_item) + _, msg = _execute_prepared_tool(session, delete_item) assert "Deleted memory" in msg rows = self._audit_rows("memory.delete") @@ -6797,23 +7053,70 @@ class TestMemoryToolAudit: "call_1", {"action": "delete", "name": "no_such_mem", "scope": "user"}, ) - _, msg = session._exec_memory(delete_item) + _, msg = _execute_prepared_tool(session, delete_item) assert "not found" in msg assert self._audit_rows("memory.delete") == [] + def test_committed_delete_is_truthful_and_next_prefix_refresh_fails_closed(self, tmp_db): + from turnstone.core.memory import get_structured_memory_by_name, save_structured_memory + + save_structured_memory("doomed", "value", description="Memory to delete", scope="global") + session = _make_session(ws_id="ws-1", user_id="user-1") + item = session._prepare_memory( + "call_1", {"action": "delete", "name": "doomed", "scope": "global"} + ) + + _, message = _execute_prepared_tool(session, item) + + assert "Deleted memory 'doomed'" in message + assert get_structured_memory_by_name("doomed", "global", "") is None + assert len(self._audit_rows("memory.delete")) == 1 + assert session._system_prefix_dirty is True + with ( + patch.object( + session, + "_init_system_messages", + side_effect=RuntimeError("composition failed"), + ), + pytest.raises(RuntimeError, match="composition failed"), + ): + session._ensure_system_prefix_fresh() + + def test_storage_failure_is_not_reported_as_not_found(self, tmp_db): + from turnstone.core.storage import get_storage + + session = _make_session(ws_id="ws-1", user_id="user-1") + storage = get_storage() + operations = ( + ( + "get_structured_memory_by_name", + {"action": "get", "name": "key", "scope": "global"}, + ), + ( + "delete_structured_memory_returning", + {"action": "delete", "name": "key", "scope": "global"}, + ), + ) + for method_name, arguments in operations: + item = session._prepare_memory("call_1", arguments) + with patch.object(storage, method_name, side_effect=RuntimeError("db down")): + _, message = _execute_prepared_tool(session, item) + assert "storage operation failed" in message + assert "not found" not in message + def test_reads_emit_no_audit(self, tmp_db): session = _make_session(ws_id="ws-1", user_id="user-1") - session._exec_memory( - session._prepare_memory( - "call_save", - { - "action": "save", - "name": "fact_one", - "content": "alpha", - "scope": "user", - }, - ) + save_item = session._prepare_memory( + "call_save", + { + "action": "save", + "description": "Test memory", + "name": "fact_one", + "content": "alpha", + "scope": "user", + }, ) + _execute_prepared_tool(session, save_item) for spec in ( {"action": "get", "name": "fact_one", "scope": "user"}, @@ -6822,7 +7125,7 @@ class TestMemoryToolAudit: ): item = session._prepare_memory("call_read", spec) assert "error" not in item - session._exec_memory(item) + _execute_prepared_tool(session, item) # Only the save above should have audited. save_count = len(self._audit_rows("memory.save")) @@ -6842,6 +7145,7 @@ class TestMemoryToolAudit: "call_1", { "action": "save", + "description": "Test memory", "name": "fact_one", "content": "alpha", "scope": "user", @@ -6851,7 +7155,7 @@ class TestMemoryToolAudit: "turnstone.core.audit.record_audit", side_effect=RuntimeError("audit storage exploded"), ): - _, msg = session._exec_memory(item) + _, msg = _execute_prepared_tool(session, item) assert "Saved memory 'fact_one'" in msg # The save itself still landed. from turnstone.core.memory import get_structured_memory_by_name @@ -6863,10 +7167,10 @@ class TestPerKindToolVariants: """Verify the ``kind_variants`` metadata applies per-kind tool overrides. Each kind sees only the tool surface it can actually use — the - coord sees ``scope`` enum ``["coordinator"]`` and a coord-flavored - description; the IC sees ``["global", "workstream", "user"]`` and - the existing IC-flavored description. The union ``TOOLS`` list - keeps the full schema for introspection / docs / eval catalogs. + coord sees coordinator/project scopes and a coord-flavored description; + the IC sees global/workstream/user/project and the IC-flavored description. + The union ``TOOLS`` list keeps the full schema for introspection / docs / + eval catalogs. """ def test_coord_memory_tool_has_coord_only_scope_enum(self): @@ -6877,6 +7181,10 @@ class TestPerKindToolVariants: # v1.7: a coordinator attached to a project also reads/writes the shared # 'project' scope, alongside its isolated 'coordinator' namespace. assert scope["enum"] == ["coordinator", "project"] + scope_desc = scope["description"] + assert "Save/get/delete without scope target project when attached" in scope_desc + assert "otherwise coordinator" in scope_desc + assert "valid explicit scope selects exactly that scope" in scope_desc def test_coord_memory_tool_description_mentions_orchestration(self): from turnstone.core.tools import COORDINATOR_TOOLS @@ -6896,6 +7204,10 @@ class TestPerKindToolVariants: scope = memory["function"]["parameters"]["properties"]["scope"] # v1.7: 'project' is offered (usable when the workstream is attached). assert scope["enum"] == ["global", "workstream", "user", "project"] + scope_desc = scope["description"] + assert "Save/get/delete without scope target project when attached" in scope_desc + assert "otherwise global" in scope_desc + assert "valid explicit scope selects exactly that scope" in scope_desc def test_ic_memory_tool_description_omits_coord_scope(self): from turnstone.core.tools import INTERACTIVE_TOOLS @@ -7024,7 +7336,12 @@ class TestMemoryAccessTouch: def _save(name: str, content: str) -> None: from turnstone.core.memory import save_structured_memory - save_structured_memory(name, content, scope="global") + save_structured_memory( + name, + content, + description=f"Test memory for {name}", + scope="global", + ) @staticmethod def _empty_session() -> ChatSession: @@ -7134,7 +7451,7 @@ class TestMemoryAccessTouch: self._save("kafka_runbook", "restart the kafka broker pods") item = session._prepare_memory("call_1", {"action": "search", "query": "kafka"}) assert "error" not in item - session._exec_memory(item) + _execute_prepared_tool(session, item) assert self._access_count("kafka_runbook") == 1 def test_get_action_touches_fetched_memory(self, tmp_db): @@ -7144,7 +7461,7 @@ class TestMemoryAccessTouch: "call_1", {"action": "get", "name": "kafka_runbook", "scope": "global"} ) assert "error" not in item - session._exec_memory(item) + _execute_prepared_tool(session, item) assert self._access_count("kafka_runbook") == 1 def test_get_miss_touches_nothing(self, tmp_db): @@ -7153,7 +7470,7 @@ class TestMemoryAccessTouch: item = session._prepare_memory( "call_1", {"action": "get", "name": "no_such_mem", "scope": "global"} ) - _, msg = session._exec_memory(item) + _, msg = _execute_prepared_tool(session, item) assert "not found" in msg # The existing row must not be collaterally touched by a miss. assert self._access_count("kafka_runbook") == 0 @@ -7162,7 +7479,7 @@ class TestMemoryAccessTouch: session = self._empty_session() self._save("kafka_runbook", "restart the kafka broker pods") item = session._prepare_memory("call_1", {"action": "list"}) - session._exec_memory(item) + _execute_prepared_tool(session, item) assert self._access_count("kafka_runbook") == 0 def test_save_action_does_not_touch_access_count(self, tmp_db): @@ -7174,9 +7491,15 @@ class TestMemoryAccessTouch: session = self._empty_session() item = session._prepare_memory( "call_1", - {"action": "save", "name": "kafka_runbook", "content": "x", "scope": "global"}, + { + "action": "save", + "name": "kafka_runbook", + "content": "x", + "description": "Kafka runbook", + "scope": "global", + }, ) - session._exec_memory(item) + _execute_prepared_tool(session, item) assert self._access_count("kafka_runbook") == 0 def test_save_through_exec_does_not_recompose_prefix(self, tmp_db): @@ -7202,13 +7525,14 @@ class TestMemoryAccessTouch: "call_1", { "action": "save", + "description": "Test memory", "name": "kafka_scaling", "content": "restart kafka and scale the broker pods cluster", "scope": "global", }, ) assert "error" not in item - session._exec_memory(item) + _execute_prepared_tool(session, item) # 1. Prefix byte-for-byte unchanged -> no prompt-cache bust. after = "\n".join(m["content"] for m in session.system_messages if m["role"] == "system") @@ -7226,11 +7550,8 @@ class TestMemoryAccessTouch: ) assert ' _exec_memory: a - content-only re-save keeps the stored type/description, while an - explicit field overwrites it. Guards the _prepare_memory omit->None - logic that the storage-level tests don't exercise.""" + def test_save_through_tool_requires_and_updates_description(self, tmp_db): + """Every tool save describes the row; an omitted type stays preserved.""" from turnstone.core.memory import get_structured_memory_by_name session = self._empty_session() @@ -7246,48 +7567,58 @@ class TestMemoryAccessTouch: }, ) assert "error" not in item - session._exec_memory(item) + _execute_prepared_tool(session, item) - # Content-only re-save (omits type/description) -> both preserved. + # An update supplies a fresh description while omitting type. item2 = session._prepare_memory( - "c2", {"action": "save", "name": "digest", "content": "v2", "scope": "global"} + "c2", + { + "action": "save", + "name": "digest", + "content": "v2", + "description": "revised daily digest", + "scope": "global", + }, ) - session._exec_memory(item2) + _execute_prepared_tool(session, item2) mem = get_structured_memory_by_name("digest", "global", "") assert mem is not None assert mem["content"] == "v2" assert mem["type"] == "reference" - assert mem["description"] == "daily digest" + assert mem["description"] == "revised daily digest" - # An invalid/typo'd type is treated as unset -> stored type preserved, - # not silently downgraded to "general". + # Invalid/typo'd types fail preparation and do not mutate the row. item_bad = session._prepare_memory( "c2b", { "action": "save", + "description": "Test memory", "name": "digest", "content": "v2b", "type": "nonsense", "scope": "global", }, ) - session._exec_memory(item_bad) + assert "error" in item_bad + assert "invalid memory type" in item_bad["error"] mem = get_structured_memory_by_name("digest", "global", "") assert mem is not None - assert mem["type"] == "reference" # invalid type ignored, not downgraded + assert mem["content"] == "v2" + assert mem["type"] == "reference" # An explicit field -> overwrites (the behaviour the None-sentinel enables). item3 = session._prepare_memory( "c3", { "action": "save", + "description": "Test memory", "name": "digest", "content": "v3", "type": "general", "scope": "global", }, ) - session._exec_memory(item3) + _execute_prepared_tool(session, item3) mem = get_structured_memory_by_name("digest", "global", "") assert mem is not None assert mem["type"] == "general" diff --git a/tests/test_session_lifecycle_commands.py b/tests/test_session_lifecycle_commands.py index 3997d660..224dc71e 100644 --- a/tests/test_session_lifecycle_commands.py +++ b/tests/test_session_lifecycle_commands.py @@ -124,9 +124,10 @@ def test_nonfork_resume_rebinds_project_memory_context_before_recomposition(tmp_ assert session.resume("target-ws") is True assert session.ws_id == "target-ws" - assert session._project_id == "target-project" - assert session._project_name == "Target Project" - assert session._project_writable is True + access = session._memory_access() + assert access.project_id == "target-project" + assert access.project_name == "Target Project" + assert access.project_writable is True assert ("project", "target-project") in session._visible_scopes() assert ("project", "source-project") not in session._visible_scopes() assert stale_cache_key not in session._mem_search_cache diff --git a/tests/test_skills_tool.py b/tests/test_skills_tool.py index 6a736a59..726f145d 100644 --- a/tests/test_skills_tool.py +++ b/tests/test_skills_tool.py @@ -6,6 +6,7 @@ hint pattern, and the skill catalog disclosure in system messages. from __future__ import annotations +import threading from typing import Any from unittest.mock import MagicMock, patch @@ -1375,15 +1376,14 @@ class TestSkillCatalogDisclosure: session._tools = [] session._client_type = ClientType.CLI session._username = "" - # _init_system_messages renders the attached project into the Session - # Context; this __new__-built session skips __init__'s project resolution, - # so seed the (unattached) defaults it reads. - session._project_name = "" - session._project_id = "" - session._project_writable = False + # This __new__-built session skips __init__'s attachment setup. + session._memory_attached_project_id = "" + session._system_prefix_lock = threading.RLock() + session._system_prefix_dirty = True + session._system_prefix_signature = None session._kind = "interactive" - # Persona snapshot attrs (set by __init__, bypassed here) — legacy - # defaults: no override, unrestricted tools, MCP + memory on. + # Persona snapshot attrs (set by __init__, bypassed here): open + # defaults with no override, unrestricted tools, MCP + memory on. session._persona_name = "" session._persona_prompt = "" session._persona_tools = None @@ -1393,6 +1393,7 @@ class TestSkillCatalogDisclosure: session._memory_config = MagicMock() session._memory_config.fetch_limit = 0 session._user_id = "test-user" + session._acting_user_id = "" # _init_system_messages -> _recompute_shared_state reads the session # owner (_mcp_user_id) to decide shared-workstream framing; __init__ # normally sets it from user_id, so seed it here for the __new__ build. diff --git a/tests/test_structured_memory.py b/tests/test_structured_memory.py index c87ed2ce..271ad1af 100644 --- a/tests/test_structured_memory.py +++ b/tests/test_structured_memory.py @@ -1,5 +1,7 @@ """Tests for turnstone.core.memory — structured memory facade functions.""" +import pytest + from turnstone.core.memory import ( count_structured_memories, delete_structured_memory, @@ -7,31 +9,42 @@ from turnstone.core.memory import ( list_structured_memories, normalize_key, save_structured_memory, + save_structured_memory_strict, search_structured_memories, ) +def _save(name, content, **kwargs): + kwargs.setdefault("description", "test memory description") + return save_structured_memory(name, content, **kwargs) + + class TestSaveStructuredMemory: + @pytest.mark.parametrize("description", [None, "", " "]) + def test_description_is_required(self, tmp_db, description): + with pytest.raises(ValueError, match="description is required"): + save_structured_memory_strict("test_key", "hello world", description=description) + def test_save_new(self, tmp_db): - row, was_update = save_structured_memory("test_key", "hello world") + row, was_update = _save("test_key", "hello world") assert row and row["memory_id"] assert was_update is False def test_save_upsert(self, tmp_db): - row1, was_update1 = save_structured_memory("test_key", "first") - row2, was_update2 = save_structured_memory("test_key", "second") + row1, was_update1 = _save("test_key", "first") + row2, was_update2 = _save("test_key", "second") assert was_update1 is False assert was_update2 is True assert row2 and row1 and row2["memory_id"] == row1["memory_id"] # same row assert row2["content"] == "second" def test_save_normalizes_key(self, tmp_db): - save_structured_memory("My-Key", "value") + _save("My-Key", "value") mems = list_structured_memories() assert any(m["name"] == "my_key" for m in mems) def test_save_with_type_and_scope(self, tmp_db): - save_structured_memory("k", "v", mem_type="user", scope="workstream", scope_id="ws1") + _save("k", "v", mem_type="user", scope="workstream", scope_id="ws1") mems = list_structured_memories(scope="workstream", scope_id="ws1") assert len(mems) == 1 assert mems[0]["type"] == "user" @@ -39,14 +52,14 @@ class TestSaveStructuredMemory: class TestDeleteStructuredMemory: def test_delete_existing(self, tmp_db): - save_structured_memory("mykey", "val") + _save("mykey", "val") assert delete_structured_memory("mykey") def test_delete_nonexistent(self, tmp_db): assert not delete_structured_memory("nope") def test_delete_normalizes_key(self, tmp_db): - save_structured_memory("my_key", "val") + _save("my_key", "val") assert delete_structured_memory("My-Key") @@ -55,25 +68,25 @@ class TestListStructuredMemories: assert list_structured_memories() == [] def test_list_returns_saved(self, tmp_db): - save_structured_memory("a", "alpha") - save_structured_memory("b", "beta") + _save("a", "alpha") + _save("b", "beta") mems = list_structured_memories() assert len(mems) == 2 class TestSearchStructuredMemories: def test_search_finds_match(self, tmp_db): - save_structured_memory("db_host", "localhost", description="database hostname") - save_structured_memory("api_url", "http://example.com") + _save("db_host", "localhost", description="database hostname") + _save("api_url", "http://example.com") results = search_structured_memories("database") assert len(results) >= 1 assert any(r["name"] == "db_host" for r in results) def test_multiword_or_matches_partial(self, tmp_db): """OR-of-terms: memory matching only 1 of 3 query terms is returned.""" - save_structured_memory("postgres_config", "host=localhost port=5432") - save_structured_memory("redis_config", "host=redis port=6379") - save_structured_memory("unrelated", "nothing relevant here") + _save("postgres_config", "host=localhost port=5432") + _save("redis_config", "host=redis port=6379") + _save("unrelated", "nothing relevant here") # "postgres missing_word_a missing_word_b": only postgres_config matches "postgres" results = search_structured_memories("postgres missing_word_a missing_word_b") @@ -83,9 +96,9 @@ class TestSearchStructuredMemories: def test_multiword_or_multiple_partial_matches(self, tmp_db): """Multiple memories each matching different terms are all returned.""" - save_structured_memory("key_alpha", "alpha content here") - save_structured_memory("key_beta", "beta content here") - save_structured_memory("key_other", "completely different") + _save("key_alpha", "alpha content here") + _save("key_beta", "beta content here") + _save("key_other", "completely different") results = search_structured_memories("alpha beta") names = {r["name"] for r in results} @@ -95,9 +108,9 @@ class TestSearchStructuredMemories: def test_search_scope_filtering_preserved(self, tmp_db): """Search with scope filter only returns memories in that scope.""" - save_structured_memory("ws1_fact", "alpha info", scope="workstream", scope_id="ws1") - save_structured_memory("ws2_fact", "alpha info", scope="workstream", scope_id="ws2") - save_structured_memory("global_fact", "alpha info", scope="global") + _save("ws1_fact", "alpha info", scope="workstream", scope_id="ws1") + _save("ws2_fact", "alpha info", scope="workstream", scope_id="ws2") + _save("global_fact", "alpha info", scope="global") results = search_structured_memories("alpha", scope="workstream", scope_id="ws1") names = {r["name"] for r in results} @@ -108,7 +121,7 @@ class TestSearchStructuredMemories: class TestGetStructuredMemoryByName: def test_get_existing(self, tmp_db): - save_structured_memory("my_mem", "full content here that is quite long") + _save("my_mem", "full content here that is quite long") mem = get_structured_memory_by_name("my_mem", "global", "") assert mem is not None assert mem["content"] == "full content here that is quite long" @@ -118,12 +131,12 @@ class TestGetStructuredMemoryByName: assert get_structured_memory_by_name("nope", "global", "") is None def test_get_wrong_scope(self, tmp_db): - save_structured_memory("ws_mem", "data", scope="workstream", scope_id="ws1") + _save("ws_mem", "data", scope="workstream", scope_id="ws1") assert get_structured_memory_by_name("ws_mem", "global", "") is None assert get_structured_memory_by_name("ws_mem", "workstream", "ws1") is not None def test_get_normalizes_key(self, tmp_db): - save_structured_memory("My-Key", "value") + _save("My-Key", "value") mem = get_structured_memory_by_name("My-Key", "global", "") assert mem is not None assert mem["name"] == "my_key" @@ -134,8 +147,8 @@ class TestCountStructuredMemories: assert count_structured_memories() == 0 def test_count_after_save(self, tmp_db): - save_structured_memory("a", "1") - save_structured_memory("b", "2") + _save("a", "1") + _save("b", "2") assert count_structured_memories() == 2 @@ -154,11 +167,11 @@ class TestScopeIsolation: def _seed(self): """Create memories across multiple scopes.""" - save_structured_memory("global_note", "visible to all", scope="global") - save_structured_memory("ws1_note", "belongs to ws1", scope="workstream", scope_id="ws1") - save_structured_memory("ws2_note", "belongs to ws2", scope="workstream", scope_id="ws2") - save_structured_memory("u1_note", "belongs to user1", scope="user", scope_id="u1") - save_structured_memory("u2_note", "belongs to user2", scope="user", scope_id="u2") + _save("global_note", "visible to all", scope="global") + _save("ws1_note", "belongs to ws1", scope="workstream", scope_id="ws1") + _save("ws2_note", "belongs to ws2", scope="workstream", scope_id="ws2") + _save("u1_note", "belongs to user1", scope="user", scope_id="u1") + _save("u2_note", "belongs to user2", scope="user", scope_id="u2") @staticmethod def _list_visible(ws_id: str, user_id: str, mem_type: str = "", limit: int = 50): diff --git a/tests/test_structured_memory_storage.py b/tests/test_structured_memory_storage.py index c3aaa002..8c1dcac2 100644 --- a/tests/test_structured_memory_storage.py +++ b/tests/test_structured_memory_storage.py @@ -2,6 +2,15 @@ class TestCreateAndGet: + def test_create_requires_non_empty_description(self, backend): + import pytest + + for description in (None, "", " "): + with pytest.raises(ValueError, match="description is required"): + backend.create_structured_memory( + "m1", "test_key", description, "general", "global", "", "data" + ) + def test_create_and_get_by_id(self, backend): backend.create_structured_memory("m1", "test_key", "desc", "general", "global", "", "data") mem = backend.get_structured_memory("m1") @@ -43,34 +52,44 @@ class TestSaveUpsert: import pytest import sqlalchemy as sa - backend.create_structured_memory("m1", "dup", "", "general", "global", "", "a") + backend.create_structured_memory("m1", "dup", "Test memory", "general", "global", "", "a") with pytest.raises(sa.exc.IntegrityError): - backend.create_structured_memory("m2", "dup", "", "general", "global", "", "b") + backend.create_structured_memory( + "m2", "dup", "Test memory", "general", "global", "", "b" + ) def test_save_same_key_updates_in_place(self, backend): from turnstone.core.memory import save_structured_memory - row1, was_update1 = save_structured_memory("upsert_key", "v1", scope="global") + row1, was_update1 = save_structured_memory( + "upsert_key", "v1", description="first description", scope="global" + ) assert row1 and was_update1 is False # inserted - row2, was_update2 = save_structured_memory("upsert_key", "v2", scope="global") + row2, was_update2 = save_structured_memory( + "upsert_key", "v2", description="updated description", scope="global" + ) assert row2 and was_update2 is True # updated in place assert row2["memory_id"] == row1["memory_id"] # same row, not a duplicate assert row2["content"] == "v2" names = [r["name"] for r in backend.list_structured_memories(scope="global")] assert names.count("upsert_key") == 1 - def test_save_same_key_preserves_description_and_type_on_default_resave(self, backend): - from turnstone.core.memory import save_structured_memory + def test_save_same_key_requires_and_updates_description(self, backend): + from turnstone.core.memory import save_structured_memory, save_structured_memory_strict save_structured_memory( "meta_key", "c1", description="orig desc", mem_type="fact", scope="global" ) - # A re-save that omits description/type (defaults) must not clobber them. - save_structured_memory("meta_key", "c2", scope="global") + # Every update must describe the revised memory; type can still be omitted. + import pytest + + with pytest.raises(ValueError, match="description is required"): + save_structured_memory_strict("meta_key", "c2", description=None, scope="global") + save_structured_memory("meta_key", "c2", description="revised description", scope="global") row = backend.get_structured_memory_by_name("meta_key", "global", "") assert row["content"] == "c2" - assert row["description"] == "orig desc" + assert row["description"] == "revised description" assert row["type"] == "fact" def test_upsert_method_updates_in_place_no_raise(self, backend): @@ -89,20 +108,75 @@ class TestSaveUpsert: names = [r["name"] for r in backend.list_structured_memories(scope="global")] assert names.count("k") == 1 - def test_upsert_none_preserves_explicit_overwrites(self, backend): - """None description/type keep the stored value on conflict; an explicit - value (including "" / "general") overwrites it.""" + def test_upsert_requires_description_and_preserves_omitted_type(self, backend): + """Description is mandatory; an omitted type keeps the stored value.""" + import pytest + backend.create_structured_memory("m1", "k", "keepdesc", "fact", "global", "", "v1") - # None -> preserve stored description/type (a content-only save). - row, _ = backend.upsert_structured_memory("m2", "k", None, None, "global", "", "v2") + with pytest.raises(ValueError, match="description is required"): + backend.upsert_structured_memory("m2", "k", None, None, "global", "", "v2") + with pytest.raises(ValueError, match="description is required"): + backend.upsert_structured_memory("m2", "k", " ", None, "global", "", "v2") + + row, _ = backend.upsert_structured_memory( + "m2", "k", "new description", None, "global", "", "v2" + ) assert row["content"] == "v2" - assert row["description"] == "keepdesc" + assert row["description"] == "new description" assert row["type"] == "fact" - # Explicit "" / "general" -> overwrite. - row2, _ = backend.upsert_structured_memory("m3", "k", "", "general", "global", "", "v3") - assert row2["description"] == "" + row2, _ = backend.upsert_structured_memory( + "m3", "k", "final description", "general", "global", "", "v3" + ) + assert row2["description"] == "final description" assert row2["type"] == "general" + def test_active_project_guard_accepts_only_active_project(self, backend): + import pytest + + backend.create_project("active", "Active", "u1") + row, was_update = backend.upsert_structured_memory( + "m1", + "guarded", + "guarded description", + None, + "project", + "active", + "value", + require_active_project=True, + ) + assert row["scope_id"] == "active" + assert was_update is False + + backend.create_project("archived", "Archived", "u1", state="archived") + for project_id in ("archived", "missing"): + with pytest.raises(ValueError, match="missing, archived"): + backend.upsert_structured_memory( + f"m-{project_id}", + "guarded", + "guarded description", + None, + "project", + project_id, + "value", + require_active_project=True, + ) + assert backend.get_structured_memory_by_name("guarded", "project", project_id) is None + + def test_active_project_guard_rejects_non_project_scope(self, backend): + import pytest + + with pytest.raises(ValueError, match="requires project scope"): + backend.upsert_structured_memory( + "m1", + "guarded", + "guarded description", + None, + "global", + "", + "value", + require_active_project=True, + ) + class TestDelete: def test_delete_existing(self, backend): @@ -118,63 +192,119 @@ class TestDelete: assert not backend.delete_structured_memory("k", "global", "") assert backend.delete_structured_memory("k", "workstream", "ws1") + def test_delete_returning_is_atomic_and_truthful(self, backend): + backend.create_structured_memory( + "m1", "k", "description", "reference", "user", "u1", "data" + ) + + deleted = backend.delete_structured_memory_returning("k", "user", "u1") + + assert deleted is not None + assert deleted["memory_id"] == "m1" + assert deleted["description"] == "description" + assert deleted["type"] == "reference" + assert backend.get_structured_memory("m1") is None + assert backend.delete_structured_memory_returning("k", "user", "u1") is None + + def test_delete_by_id_returning_is_atomic_and_truthful(self, backend): + backend.create_structured_memory("m1", "k", "Test memory", "general", "global", "", "data") + + deleted = backend.delete_structured_memory_by_id_returning("m1") + + assert deleted is not None + assert deleted["name"] == "k" + assert backend.get_structured_memory("m1") is None + assert backend.delete_structured_memory_by_id_returning("m1") is None + + +class TestFindScopes: + def test_finds_only_requested_same_name_scopes(self, backend): + backend.create_structured_memory("m1", "same", "Test memory", "general", "global", "", "g") + backend.create_structured_memory( + "m2", "same", "Test memory", "general", "user", "u1", "own" + ) + backend.create_structured_memory( + "m3", "same", "Test memory", "general", "user", "victim", "secret" + ) + backend.create_structured_memory( + "m4", "other", "Test memory", "general", "workstream", "ws1", "other" + ) + + found = backend.find_structured_memory_scopes( + "same", [("global", ""), ("user", "u1"), ("workstream", "ws1")] + ) + + assert set(found) == {("global", ""), ("user", "u1")} + class TestList: def test_list_all(self, backend): - backend.create_structured_memory("m1", "a", "", "general", "global", "", "1") - backend.create_structured_memory("m2", "b", "", "user", "global", "", "2") + backend.create_structured_memory("m1", "a", "Test memory", "general", "global", "", "1") + backend.create_structured_memory("m2", "b", "Test memory", "user", "global", "", "2") mems = backend.list_structured_memories() assert len(mems) == 2 def test_list_by_type(self, backend): - backend.create_structured_memory("m1", "a", "", "general", "global", "", "1") - backend.create_structured_memory("m2", "b", "", "user", "global", "", "2") + backend.create_structured_memory("m1", "a", "Test memory", "general", "global", "", "1") + backend.create_structured_memory("m2", "b", "Test memory", "user", "global", "", "2") mems = backend.list_structured_memories(mem_type="user") assert len(mems) == 1 assert mems[0]["name"] == "b" def test_list_by_scope(self, backend): - backend.create_structured_memory("m1", "a", "", "general", "global", "", "1") - backend.create_structured_memory("m2", "b", "", "general", "workstream", "ws1", "2") + backend.create_structured_memory("m1", "a", "Test memory", "general", "global", "", "1") + backend.create_structured_memory( + "m2", "b", "Test memory", "general", "workstream", "ws1", "2" + ) mems = backend.list_structured_memories(scope="workstream") assert len(mems) == 1 def test_list_respects_limit(self, backend): for i in range(10): - backend.create_structured_memory(f"m{i}", f"k{i}", "", "general", "global", "", f"{i}") + backend.create_structured_memory( + f"m{i}", f"k{i}", "Test memory", "general", "global", "", f"{i}" + ) mems = backend.list_structured_memories(limit=3) assert len(mems) == 3 class TestSearch: def test_search_by_name(self, backend): - backend.create_structured_memory("m1", "database_config", "", "general", "global", "", "pg") - backend.create_structured_memory("m2", "api_key", "", "general", "global", "", "secret") + backend.create_structured_memory( + "m1", "database_config", "Test memory", "general", "global", "", "pg" + ) + backend.create_structured_memory( + "m2", "api_key", "Test memory", "general", "global", "", "secret" + ) results = backend.search_structured_memories("database") assert len(results) == 1 assert results[0]["name"] == "database_config" def test_search_by_content(self, backend): - backend.create_structured_memory("m1", "a", "", "general", "global", "", "postgresql host") + backend.create_structured_memory( + "m1", "a", "Test memory", "general", "global", "", "postgresql host" + ) results = backend.search_structured_memories("postgresql") assert len(results) == 1 def test_search_empty_lists_all(self, backend): - backend.create_structured_memory("m1", "a", "", "general", "global", "", "1") - backend.create_structured_memory("m2", "b", "", "general", "global", "", "2") + backend.create_structured_memory("m1", "a", "Test memory", "general", "global", "", "1") + backend.create_structured_memory("m2", "b", "Test memory", "general", "global", "", "2") results = backend.search_structured_memories("") assert len(results) == 2 class TestCount: def test_count_all(self, backend): - backend.create_structured_memory("m1", "a", "", "general", "global", "", "1") - backend.create_structured_memory("m2", "b", "", "general", "global", "", "2") + backend.create_structured_memory("m1", "a", "Test memory", "general", "global", "", "1") + backend.create_structured_memory("m2", "b", "Test memory", "general", "global", "", "2") assert backend.count_structured_memories() == 2 def test_count_by_scope(self, backend): - backend.create_structured_memory("m1", "a", "", "general", "global", "", "1") - backend.create_structured_memory("m2", "b", "", "general", "workstream", "ws1", "2") + backend.create_structured_memory("m1", "a", "Test memory", "general", "global", "", "1") + backend.create_structured_memory( + "m2", "b", "Test memory", "general", "workstream", "ws1", "2" + ) assert backend.count_structured_memories(scope="global") == 1 assert backend.count_structured_memories(scope="workstream") == 1 @@ -184,8 +314,12 @@ class TestSearchOrOfTerms: def test_single_matching_term_in_multi_word_query(self, backend): """Memory with content 'apple' found when query is 'apple banana cherry'.""" - backend.create_structured_memory("m1", "apple_mem", "", "general", "global", "", "apple") - backend.create_structured_memory("m2", "other_mem", "", "general", "global", "", "grape") + backend.create_structured_memory( + "m1", "apple_mem", "Test memory", "general", "global", "", "apple" + ) + backend.create_structured_memory( + "m2", "other_mem", "Test memory", "general", "global", "", "grape" + ) results = backend.search_structured_memories("apple banana cherry") names = {r["name"] for r in results} @@ -194,10 +328,18 @@ class TestSearchOrOfTerms: def test_partial_overlap_across_memories(self, backend): """Each memory matches one of three terms; all three are returned.""" - backend.create_structured_memory("m1", "alpha_doc", "", "general", "global", "", "alpha") - backend.create_structured_memory("m2", "beta_doc", "", "general", "global", "", "beta") - backend.create_structured_memory("m3", "gamma_doc", "", "general", "global", "", "gamma") - backend.create_structured_memory("m4", "unrelated", "", "general", "global", "", "delta") + backend.create_structured_memory( + "m1", "alpha_doc", "Test memory", "general", "global", "", "alpha" + ) + backend.create_structured_memory( + "m2", "beta_doc", "Test memory", "general", "global", "", "beta" + ) + backend.create_structured_memory( + "m3", "gamma_doc", "Test memory", "general", "global", "", "gamma" + ) + backend.create_structured_memory( + "m4", "unrelated", "Test memory", "general", "global", "", "delta" + ) results = backend.search_structured_memories("alpha beta gamma") names = {r["name"] for r in results} @@ -209,12 +351,14 @@ class TestSearchOrOfTerms: def test_scope_filter_preserved(self, backend): """OR-of-terms search still respects scope / scope_id filters.""" backend.create_structured_memory( - "m1", "ws1_note", "", "general", "workstream", "ws1", "info" + "m1", "ws1_note", "Test memory", "general", "workstream", "ws1", "info" ) backend.create_structured_memory( - "m2", "ws2_note", "", "general", "workstream", "ws2", "info" + "m2", "ws2_note", "Test memory", "general", "workstream", "ws2", "info" + ) + backend.create_structured_memory( + "m3", "global_note", "Test memory", "general", "global", "", "info" ) - backend.create_structured_memory("m3", "global_note", "", "general", "global", "", "info") results = backend.search_structured_memories("info", scope="workstream", scope_id="ws1") names = {r["name"] for r in results} @@ -224,9 +368,11 @@ class TestSearchOrOfTerms: def test_term_cap_normalizes_unbounded_query(self, backend): """A multi-KB query collapses to <= MAX terms (de-dupe + length filter).""" - backend.create_structured_memory("m1", "alpha_doc", "", "general", "global", "", "alpha") backend.create_structured_memory( - "m2", "other_doc", "", "general", "global", "", "irrelevant" + "m1", "alpha_doc", "Test memory", "general", "global", "", "alpha" + ) + backend.create_structured_memory( + "m2", "other_doc", "Test memory", "general", "global", "", "irrelevant" ) # Build a noisy query: same word repeated, plus 1-char tokens that @@ -241,10 +387,18 @@ class TestVisibleStructuredMemories: """Single-query union helpers used by the composition path.""" def test_list_visible_unions_global_workstream_user(self, backend): - backend.create_structured_memory("m1", "g_note", "", "general", "global", "", "g") - backend.create_structured_memory("m2", "ws_note", "", "general", "workstream", "ws1", "w") - backend.create_structured_memory("m3", "u_note", "", "general", "user", "u1", "u") - backend.create_structured_memory("m4", "other_ws", "", "general", "workstream", "ws2", "x") + backend.create_structured_memory( + "m1", "g_note", "Test memory", "general", "global", "", "g" + ) + backend.create_structured_memory( + "m2", "ws_note", "Test memory", "general", "workstream", "ws1", "w" + ) + backend.create_structured_memory( + "m3", "u_note", "Test memory", "general", "user", "u1", "u" + ) + backend.create_structured_memory( + "m4", "other_ws", "Test memory", "general", "workstream", "ws2", "x" + ) scopes = [("global", ""), ("workstream", "ws1"), ("user", "u1")] rows = backend.list_visible_structured_memories(scopes) @@ -252,12 +406,14 @@ class TestVisibleStructuredMemories: assert names == {"g_note", "ws_note", "u_note"} # ws2 excluded def test_search_visible_unions_scopes_and_terms(self, backend): - backend.create_structured_memory("m1", "g_alpha", "", "general", "global", "", "alpha") backend.create_structured_memory( - "m2", "ws_beta", "", "general", "workstream", "ws1", "beta" + "m1", "g_alpha", "Test memory", "general", "global", "", "alpha" ) backend.create_structured_memory( - "m3", "ws_other", "", "general", "workstream", "ws2", "alpha" + "m2", "ws_beta", "Test memory", "general", "workstream", "ws1", "beta" + ) + backend.create_structured_memory( + "m3", "ws_other", "Test memory", "general", "workstream", "ws2", "alpha" ) scopes = [("global", ""), ("workstream", "ws1")] @@ -268,7 +424,9 @@ class TestVisibleStructuredMemories: assert "ws_other" not in names # ws2 -> outside visibility def test_visible_helpers_handle_empty_scopes(self, backend): - backend.create_structured_memory("m1", "anything", "", "general", "global", "", "x") + backend.create_structured_memory( + "m1", "anything", "Test memory", "general", "global", "", "x" + ) assert backend.list_visible_structured_memories([]) == [] assert backend.search_visible_structured_memories("x", []) == [] @@ -288,7 +446,7 @@ class TestStableOrderingOnTimestampTies: # batch lands them in the same second. for mid in ("zebra_id", "apple_id", "mango_id"): backend.create_structured_memory( - mid, f"name_{mid}", "", "general", "global", "", "shared content" + mid, f"name_{mid}", "Test memory", "general", "global", "", "shared content" ) import sqlalchemy as sa diff --git a/turnstone/api/server_schemas.py b/turnstone/api/server_schemas.py index 9d2d7699..8fbc5e4e 100644 --- a/turnstone/api/server_schemas.py +++ b/turnstone/api/server_schemas.py @@ -753,10 +753,20 @@ MemoryScope = Literal["global", "workstream", "user"] class SaveMemoryRequest(BaseModel): - name: str = Field(description="Memory identifier (normalized to snake_case)") - content: str = Field(description="Memory content", max_length=65536) - description: str = Field(default="", description="Short description for relevance matching") - type: MemoryType = Field(default="general", description="Memory type") + name: str = Field( + description="Memory identifier (normalized to snake_case)", + min_length=1, + max_length=256, + ) + content: str = Field(description="Memory content", min_length=1, max_length=65536) + description: str = Field( + description="Required non-empty description used for relevance matching", + min_length=1, + ) + type: MemoryType | None = Field( + default=None, + description="Memory type; omission preserves it on update and defaults on insert", + ) scope: MemoryScope = Field(default="global", description="Memory scope") scope_id: str = Field( default="", @@ -765,6 +775,8 @@ class SaveMemoryRequest(BaseModel): @model_validator(mode="after") def _validate_scope_scope_id(self) -> SaveMemoryRequest: + if not self.description.strip(): + raise ValueError("description is required and must be non-empty") scope_id = self.scope_id.strip() if self.scope == "global" and scope_id: raise ValueError("scope_id is not allowed with global scope") @@ -795,7 +807,7 @@ MemoryScopeFilter = Literal["", "global", "workstream", "user"] class SearchMemoriesRequest(BaseModel): - query: str = Field(description="Search query text") + query: str = Field(description="Search query text", min_length=1) type: MemoryTypeFilter = Field(default="", description="Filter by memory type") scope: MemoryScopeFilter = Field(default="", description="Filter by scope") scope_id: str = Field(default="", description="Filter by scope_id") @@ -808,6 +820,8 @@ class SearchMemoriesRequest(BaseModel): raise ValueError("scope_id is not allowed with global scope") if scope_id and not self.scope: raise ValueError("scope is required when scope_id is provided") + if self.scope == "workstream" and not scope_id: + raise ValueError("scope_id is required for workstream scope") return self diff --git a/turnstone/api/server_spec.py b/turnstone/api/server_spec.py index cd0cc628..405f9eb3 100644 --- a/turnstone/api/server_spec.py +++ b/turnstone/api/server_spec.py @@ -496,16 +496,17 @@ SERVER_ENDPOINTS: list[EndpointSpec] = [ EndpointSpec( "/v1/api/memories", "GET", - "List structured memories", + "List structured memories. Without a scope, returns global plus the authenticated user's memories; workstream scope is owner-bound.", response_model=ListMemoriesResponse, query_params=[ QueryParam("type", "Filter by memory type"), - QueryParam("scope", "Filter by scope"), + QueryParam("scope", "Filter by public scope: global, workstream, or user"), QueryParam("scope_id", "Filter by scope identifier"), QueryParam( "limit", "Max results (default 100, max 200)", schema_type="integer", default=100 ), ], + error_codes=[400, 403, 404, 500], tags=["Memories"], ), EndpointSpec( @@ -514,15 +515,16 @@ SERVER_ENDPOINTS: list[EndpointSpec] = [ "Save (upsert) a structured memory", request_model=SaveMemoryRequest, response_model=MemoryInfo, - error_codes=[400], + error_codes=[400, 403, 404, 500], tags=["Memories"], ), EndpointSpec( "/v1/api/memories/search", "POST", - "Search structured memories by query", + "Search structured memories by query. Without a scope, searches global plus the authenticated user's memories.", request_model=SearchMemoriesRequest, response_model=ListMemoriesResponse, + error_codes=[400, 403, 404, 500], tags=["Memories"], ), EndpointSpec( @@ -534,7 +536,7 @@ SERVER_ENDPOINTS: list[EndpointSpec] = [ QueryParam("scope", "Scope (default: global)"), QueryParam("scope_id", "Scope identifier"), ], - error_codes=[404], + error_codes=[400, 403, 404, 500], tags=["Memories"], ), # --- Admin settings --- diff --git a/turnstone/console/server.py b/turnstone/console/server.py index 5a920b23..66d1ef41 100644 --- a/turnstone/console/server.py +++ b/turnstone/console/server.py @@ -9544,12 +9544,14 @@ async def admin_delete_memory(request: Request) -> JSONResponse: return err memory_id = request.path_params["memory_id"] - existing = storage.get_structured_memory(memory_id) + try: + existing = storage.delete_structured_memory_by_id_returning(memory_id) + except Exception: + log.warning("memory.admin_delete_failed memory_id=%s", memory_id, exc_info=True) + return JSONResponse({"error": "Failed to delete memory"}, status_code=500) if not existing: return JSONResponse({"error": "Memory not found"}, status_code=404) - storage.delete_structured_memory_by_id(memory_id) - audit_uid, ip = _audit_context(request) record_audit( storage, diff --git a/turnstone/core/memory.py b/turnstone/core/memory.py index b2b5903e..7576cb38 100644 --- a/turnstone/core/memory.py +++ b/turnstone/core/memory.py @@ -858,10 +858,12 @@ def search_history_recent(limit: int = 20, *, user_id: str | None = None) -> lis def save_structured_memory( name: str, content: str, - description: str | None = None, + description: str, mem_type: str | None = None, scope: str = "global", scope_id: str = "", + *, + require_active_project: bool = False, ) -> tuple[dict[str, str] | None, bool]: """Save a structured memory as a single atomic upsert by name+scope+scope_id. @@ -872,22 +874,62 @@ def save_structured_memory( IntegrityError round-trip, no TOCTOU window. ``(row, was_update)`` comes straight from that upsert (this passes a fresh ``memory_id``, so a differing returned id means an existing row was updated in place). A ``None`` - description / ``mem_type`` means "leave unset" -- the column default applies - on insert and the stored value is kept on conflict. + ``description`` is required and must contain non-whitespace text for both + inserts and updates. A ``None`` ``mem_type`` keeps the stored value on an + update and uses the column default on insert. """ - import uuid - - name = normalize_key(name) try: - row, was_update = get_storage().upsert_structured_memory( - str(uuid.uuid4()), name, description, mem_type, scope, scope_id, content + return save_structured_memory_strict( + name, + content, + description=description, + mem_type=mem_type, + scope=scope, + scope_id=scope_id, + require_active_project=require_active_project, ) - return (row, was_update) if row else (None, False) except Exception: log.warning("Failed to save structured memory name=%s", name, exc_info=True) return None, False +def save_structured_memory_strict( + name: str, + content: str, + description: str, + mem_type: str | None = None, + scope: str = "global", + scope_id: str = "", + *, + require_active_project: bool = False, +) -> tuple[dict[str, str], bool]: + """Strict structured-memory upsert for mutation-facing boundaries. + + Unlike :func:`save_structured_memory`, storage failures propagate so an + API or tool cannot report a database outage as an ordinary failed/not-found + result. Prompt composition keeps using the best-effort facade. + """ + import uuid + + normalized = normalize_key(name) + normalized_description = (description or "").strip() + if not normalized_description: + raise ValueError("memory description is required and must be non-empty") + row, was_update = get_storage().upsert_structured_memory( + str(uuid.uuid4()), + normalized, + normalized_description, + mem_type, + scope, + scope_id, + content, + require_active_project=require_active_project, + ) + if not row: + raise RuntimeError("structured memory upsert returned no row") + return row, was_update + + def get_structured_memory_by_name( name: str, scope: str = "global", scope_id: str = "" ) -> dict[str, str] | None: @@ -900,6 +942,13 @@ def get_structured_memory_by_name( return None +def get_structured_memory_by_name_strict( + name: str, scope: str = "global", scope_id: str = "" +) -> dict[str, str] | None: + """Strict scoped-name lookup; storage failures propagate.""" + return get_storage().get_structured_memory_by_name(normalize_key(name), scope, scope_id) + + def delete_structured_memory(name: str, scope: str = "global", scope_id: str = "") -> bool: """Delete a structured memory by name+scope. Returns True if existed.""" name = normalize_key(name) @@ -919,6 +968,36 @@ def delete_structured_memory_by_id(memory_id: str) -> bool: return False +def delete_structured_memory_returning_strict( + name: str, scope: str = "global", scope_id: str = "" +) -> dict[str, str] | None: + """Atomically delete and return one scoped-name memory. + + Storage failures propagate. A ``None`` return therefore means only that + no matching row existed at the mutation point. + """ + return get_storage().delete_structured_memory_returning(normalize_key(name), scope, scope_id) + + +def delete_structured_memory_by_id_returning_strict( + memory_id: str, +) -> dict[str, str] | None: + """Atomically delete and return one memory by id; failures propagate.""" + return get_storage().delete_structured_memory_by_id_returning(memory_id) + + +def find_structured_memory_scopes( + name: str, + scopes: list[tuple[str, str]], +) -> list[tuple[str, str]]: + """Find visible same-name scope pairs in one metadata-only query.""" + try: + return get_storage().find_structured_memory_scopes(normalize_key(name), scopes) + except Exception: + log.warning("Failed to find structured memory scopes name=%s", name, exc_info=True) + return [] + + def list_structured_memories( mem_type: str = "", scope: str = "", diff --git a/turnstone/core/session.py b/turnstone/core/session.py index 267371b1..646b0c51 100644 --- a/turnstone/core/session.py +++ b/turnstone/core/session.py @@ -36,7 +36,7 @@ import traceback import uuid from datetime import UTC, datetime, timedelta from html import escape as _html_escape -from typing import TYPE_CHECKING, Any, ClassVar, Protocol, cast +from typing import TYPE_CHECKING, Any, ClassVar, Literal, Protocol, cast import httpx @@ -79,16 +79,16 @@ from turnstone.core.lowering import ( from turnstone.core.mcp_client import try_prime_user_pools from turnstone.core.memory import ( count_structured_memories, - delete_structured_memory_by_id, + delete_structured_memory_returning_strict, delete_workstream, + find_structured_memory_scopes, get_attachments, get_compaction_checkpoint, get_skill_by_name, - get_structured_memory_by_name, + get_structured_memory_by_name_strict, get_workstream_display_name, list_default_skills, list_skills_by_activation, - list_structured_memories, list_visible_structured_memories, list_workstreams_with_history, load_message_turns, @@ -97,13 +97,12 @@ from turnstone.core.memory import ( resolve_workstream, save_message, save_messages_bulk, - save_structured_memory, + save_structured_memory_strict, save_tool_message_with_attachments, save_user_message_with_attachments, save_workstream_config, search_history, search_history_recent, - search_structured_memories, search_visible_structured_memories, set_workstream_alias, touch_structured_memories, @@ -1182,6 +1181,9 @@ _active_task_agent_cancel_scope: contextvars.ContextVar[_ParallelModelCancelScop _active_tool_origin_generation: contextvars.ContextVar[int] = contextvars.ContextVar( "turnstone_active_tool_origin_generation", default=0 ) +_active_tool_prepare_principal: contextvars.ContextVar[str] = contextvars.ContextVar( + "turnstone_active_tool_prepare_principal", default="" +) # Generation whose bounded live commit is currently staging durable closures # on this thread. State UIs use the captured value to refuse a delayed tail @@ -1562,12 +1564,49 @@ _VALID_MEMORY_SCOPES: tuple[str, ...] = ( "project", ) -# Implicit-scope walk for INTERACTIVE ``memory(action='get'/'delete')`` -# when no scope is specified. Narrowest → widest so the most -# session-specific row wins on a name collision. Coord sessions use a -# different walk (just ``("coordinator",)``) — see -# :meth:`ChatSession._implicit_scope_walk`. -_IMPLICIT_SCOPE_WALK: tuple[str, ...] = ("workstream", "user", "global") +_MEMORY_MIXED_BATCH_ERROR = ( + "Error: memory read and write actions cannot run in the same tool batch. " + "Split the mutation and the follow-up read across separate assistant turns." +) + + +def _batch_policy( + group: str, + access: Literal["read", "write"], + mixed_access_error: str, + *, + serialize: bool = False, +) -> dict[str, str | bool]: + """Declare action-dependent execution constraints on a prepared tool item.""" + return { + "group": group, + "access": access, + "mixed_access_error": mixed_access_error, + "serialize": serialize, + } + + +@dataclasses.dataclass(frozen=True) +class _MemoryAccess: + """One actor-specific, live memory authorization snapshot.""" + + principal_id: str + attached_project_id: str + project_id: str + project_name: str + project_writable: bool + + @property + def signature(self) -> tuple[str, str, str, str, bool]: + """Cache witness for actor/project visibility and prompt metadata.""" + return ( + self.principal_id, + self.attached_project_id, + self.project_id, + self.project_name, + self.project_writable, + ) + # ``list_nodes`` reserves four top-level kwargs for control parameters # (filters / paging / output verbosity / liveness toggle). Anything @@ -1584,6 +1623,13 @@ _LIST_NODES_RESERVED_ARGS: frozenset[str] = frozenset( # result; all reads can't race) and reject only the mixed read+write # shape where ``tasks(list)`` paralleled with ``tasks(add=...)`` has # unspecified ordering inside ``_execute_tools``'s ThreadPoolExecutor. +_TASKS_MIXED_BATCH_ERROR = ( + "Error: tasks(...) read (`list`) and write " + "(`add` / `update` / `remove` / `reorder`) actions cannot run in the " + "same parallel tool batch — the read-after-write ordering is not guaranteed. " + "All-reads or all-writes are fine; mix only by splitting them across separate " + "assistant turns." +) _TASKS_READ_ACTIONS: frozenset[str] = frozenset({"list"}) _TASKS_WRITE_ACTIONS: frozenset[str] = frozenset({"add", "update", "remove", "reorder"}) @@ -2588,11 +2634,24 @@ class ChatSession: # fires many times within one turn (state transitions, MCP refresh, # tool results) and the recent-context string is identical across # them. Invalidated on user-turn append and on memory write/delete. - self._mem_search_cache: dict[tuple[str, str, int], list[dict[str, str]]] = {} + self._mem_search_cache: dict[ + tuple[str, str, int, tuple[tuple[str, str], ...]], + list[dict[str, str]], + ] = {} # Per-turn dedup for composition touches: ``_init_system_messages`` runs # many times within a turn, so the injected set is touched at most once # per memory per turn. Cleared alongside the search cache. self._touched_memory_keys: set[tuple[str, str, str]] = set() + # Prefix publication and memory mutations share a short critical + # section. Composition itself stays outside the lock because storage, + # MCP, and relevance lookups may block; each attempt carries an epoch + # and only the newest still-current attempt may publish. A memory write + # bumps the epoch before releasing the lane, so an older composition + # can never reinstall a row the write changed or deleted. + self._system_prefix_lock = threading.RLock() + self._system_prefix_epoch = 0 + self._system_prefix_dirty = True + self._system_prefix_signature: tuple[str, str, str, str, bool] | None = None # Per-send memo for the wire attachment resolver (set in send(), None # outside a send). _resolve_attachments re-runs on every agentic # round-trip, so this caches the materialized part by @@ -2605,28 +2664,9 @@ class ChatSession: # Internal destination-incarnation witness installed by # SessionManager after construction for HTTP fork creates. self._fork_reservation_token = fork_reservation_token - # Project attachment + access, resolved ONCE here (mid-session attach or - # access-revoke takes effect on the next session load — same contract as - # user_id / coordinator scope). ``_project_id`` is set only when the user - # can READ the project, so it gates recall in ``_visible_scopes``; - # ``_project_writable`` additionally gates the memory(save) path so a - # non-member of a *public* project can read but not write its memory. - self._project_id = "" - self._project_name = "" - self._project_writable = False - if project_id and self._user_id: - from turnstone.core.auth import resolve_project_access - - # One fetch resolves read access, write access, the display name, and - # the project state (vs three round-trips). Recall is gated on READ - # access AND a non-archived project: an archived project is "not - # recalled" per the schema contract, even though its owner can still - # reach it through the management routes (to rename / unarchive). - acc = resolve_project_access(self._user_id, project_id) - if acc.can_read and acc.state != "archived": - self._project_id = project_id - self._project_writable = acc.can_write - self._project_name = acc.name + # Keep only the durable attachment. Authorization and display metadata + # are resolved live for the acting principal at each security boundary. + self._memory_attached_project_id = (project_id or "").strip() # Persona snapshot — the four levers, resolved ONCE at workstream # creation and immutable for the session's lifetime. Everything # below (tool merge, MCP gate, composition, memory) reads these @@ -5524,13 +5564,14 @@ class ChatSession: if storage is None: raise RuntimeError("storage is not initialized") persona = self._current_persona_snapshot() + project_access = self._memory_access(principal_id) expected_session = ForkCloneExpectation( persona_config=( tuple(sorted(persona.to_config().items())) if persona is not None else () ), - project_id=self._project_id, - project_name=self._project_name, - project_writable=self._project_writable, + project_id=project_access.project_id, + project_name=project_access.project_name, + project_writable=project_access.project_writable, destination_reservation_token=self._fork_reservation_token, source_reservation_token=source_reservation_token, ) @@ -5591,9 +5632,7 @@ class ChatSession: if _fork_snapshot is not None else load_workstream_config(ws_id) ) - resumed_project_id = "" - resumed_project_name = "" - resumed_project_writable = False + resumed_attached_project_id = "" if not fork: storage = get_storage() target_row = storage.get_workstream(ws_id) if storage is not None else None @@ -5603,18 +5642,7 @@ class ChatSession: if isinstance(raw_project_id, str) and raw_project_id.strip() else "" ) - if target_project_id and self._user_id: - from turnstone.core.auth import resolve_project_access - - access = resolve_project_access( - self._user_id, - target_project_id, - storage=storage, - ) - if access.can_read and access.state != "archived": - resumed_project_id = target_project_id - resumed_project_name = access.name - resumed_project_writable = access.can_write + resumed_attached_project_id = target_project_id # Parse every scalar that can reject persisted input before either # identity/history adoption or a fork's durable bulk copy. A corrupt # value must not leave a half-adopted live session, nor committed fork @@ -5657,9 +5685,7 @@ class ChatSession: "construction — open the workstream fresh instead" ) self._ws_id = ws_id - self._project_id = resumed_project_id - self._project_name = resumed_project_name - self._project_writable = resumed_project_writable + self._memory_attached_project_id = resumed_attached_project_id # A non-fork resume repoints this session at a DIFFERENT existing # workstream's identity (fork keeps self._ws_id, so its nonces stay # correctly scoped to the ws they were minted for). The sender-label @@ -5961,7 +5987,29 @@ class ChatSession: } return messages - def _init_system_messages(self, *, origin_generation: int = 0) -> bool: + def _init_system_messages( + self, + *, + origin_generation: int = 0, + principal_id: str | None = None, + ) -> bool: + """Compose and atomically publish the newest system-prefix attempt.""" + with self._system_prefix_lock: + self._system_prefix_epoch = getattr(self, "_system_prefix_epoch", 0) + 1 + composition_epoch = self._system_prefix_epoch + return self._compose_system_messages( + origin_generation=origin_generation, + principal_id=principal_id, + composition_epoch=composition_epoch, + ) + + def _compose_system_messages( + self, + *, + origin_generation: int = 0, + principal_id: str | None = None, + composition_epoch: int | None = None, + ) -> bool: """Build the system/developer prefix messages. Developer message contains the composed system message (persona @@ -5972,7 +6020,13 @@ class ChatSession: atomically so concurrent readers (e.g. background thread callbacks) never see a partially-built system message. """ + if composition_epoch is None: + with self._system_prefix_lock: + self._system_prefix_epoch = getattr(self, "_system_prefix_epoch", 0) + 1 + composition_epoch = self._system_prefix_epoch + new_system_messages: list[dict[str, Any]] = [] + memory_access = self._memory_access(principal_id) shared_state_plan = self._plan_shared_state() owner = (self._mcp_user_id or "").strip() planned_senders = set(self._known_senders) | shared_state_plan[1] @@ -5983,7 +6037,7 @@ class ChatSession: ) planned_shared = self._shared_workstream or any(s != owner for s in planned_senders) memory_cache_updates: dict[ - tuple[str, str, int], + tuple[str, str, int, tuple[tuple[str, str], ...]], list[dict[str, str]], ] = {} planned_touch_keys: list[tuple[str, str, str]] = [] @@ -6018,10 +6072,10 @@ class ChatSession: current_datetime=now.strftime("%Y-%m-%dT%H:00"), timezone=now.tzname() or "UTC", username=self._username or self._user_id or "unknown", - project=self._project_name, + project=memory_access.project_name, shared=planned_shared, ws_id=self._ws_id, - project_id=self._project_id, + project_id=memory_access.project_id, ) composed = compose_system_message( client_type=self._client_type, @@ -6210,6 +6264,7 @@ class ChatSession: visible_mems, candidate_source = ( self._select_memory_candidates( context, + access=memory_access, cache_updates=memory_cache_updates, ) if self._persona_memory @@ -6260,22 +6315,31 @@ class ChatSession: if skill_context: new_system_messages.append({"role": "user", "content": skill_context}) + installed = False + def _install() -> None: - self._apply_shared_state_plan(*shared_state_plan) - self._mem_search_cache.update(memory_cache_updates) - fresh_touch_keys = [ - key for key in planned_touch_keys if key not in self._touched_memory_keys - ] - self._touched_memory_keys.update(fresh_touch_keys) - accepted_touch_keys.extend(fresh_touch_keys) - # Atomic swaps — readers see either old or new, never partial. - self._agent_system_messages = new_agent_system_messages - self.system_messages = new_system_messages - if composed_with_context: - # Composed against a real user-message query at least once; - # send() uses this to know the deferred first-turn recompose is - # done. Publish this latch with the prefix it describes. - self._system_composed_with_context = True + nonlocal installed + with self._system_prefix_lock: + if composition_epoch != getattr(self, "_system_prefix_epoch", 0): + return + self._apply_shared_state_plan(*shared_state_plan) + self._mem_search_cache.update(memory_cache_updates) + self._system_prefix_signature = memory_access.signature + fresh_touch_keys = [ + key for key in planned_touch_keys if key not in self._touched_memory_keys + ] + self._touched_memory_keys.update(fresh_touch_keys) + accepted_touch_keys.extend(fresh_touch_keys) + # Atomic swaps — readers see either old or new, never partial. + self._agent_system_messages = new_agent_system_messages + self.system_messages = new_system_messages + self._system_prefix_dirty = False + if composed_with_context: + # Composed against a real user-message query at least once; + # send() uses this to know the deferred first-turn recompose is + # done. Publish this latch with the prefix it describes. + self._system_composed_with_context = True + installed = True if origin_generation: published = self._publish_for_generation( @@ -6286,10 +6350,41 @@ class ChatSession: else: _install() published = True + published = published and installed if published and accepted_touch_keys: touch_structured_memories(accepted_touch_keys) return published + def _ensure_system_prefix_fresh( + self, + *, + principal_id: str | None = None, + origin_generation: int = 0, + ) -> None: + """Refresh the cached prefix when its actor/project witness changed.""" + for _attempt in range(2): + access = self._memory_access(principal_id) + with self._system_prefix_lock: + if ( + not self._system_prefix_dirty + and self._system_prefix_signature == access.signature + ): + return + self._system_prefix_dirty = True + if not self._init_system_messages( + origin_generation=origin_generation, + principal_id=principal_id, + ): + break + current_signature = self._memory_access(principal_id).signature + with self._system_prefix_lock: + if ( + not self._system_prefix_dirty + and self._system_prefix_signature == current_signature + ): + return + raise RuntimeError("memory context could not be refreshed safely") + def _full_messages(self) -> list[dict[str, Any]]: """System messages + conversation history as wire dicts. @@ -10256,6 +10351,15 @@ class ChatSession: """ return self._acting_user_id or self._user_id or None + def _tool_prepare_principal_id(self) -> str: + """Principal pinned for the current tool-preparation batch.""" + return ( + _active_tool_prepare_principal.get() + or getattr(self, "_acting_user_id", "") + or getattr(self, "_user_id", "") + or "" + ).strip() + def bind_acting_user(self, user_id: str) -> None: """Bind directly, or defer a worker-thread bind until generation claim. @@ -10315,7 +10419,9 @@ class ChatSession: if not user_id or user_id == (self._acting_user_id or self._user_id): self._acting_user_id = self._acting_user_id or user_id return - self._acting_user_id = user_id + with self._system_prefix_lock: + self._acting_user_id = user_id + self._invalidate_memory_cache() mcp = self._mcp_client # Coordinators participate fully (#725): they are multi-sender by # design (any admin.coordinator operator with project visibility @@ -10339,21 +10445,14 @@ class ChatSession: mcp.add_prompt_listener(self._mcp_prompt_cb, user_id=new_listener_uid) self._mcp_listener_user_id = new_listener_uid try_prime_user_pools(mcp, new_listener_uid, context="acting-user-change") - # Rebuild the merged tool list and resource/prompt-dependent - # state under the new identity NOW — the prime above completes + # Rebuild the merged tool list under the new identity now — the prime above completes # asynchronously and only notifies on catalog changes, while # already-warm pool entries for this user produce no - # notification at all. ONE _init_system_messages() covers both - # the resource and prompt catalogs: the _on_mcp_resources_changed - # / _on_mcp_prompts_changed wrappers are pure passthroughs to it - # (they exist for the manager's separate notification channels, - # which still fire them independently), and it rebuilds the full - # system message copy-on-write — calling it twice per handoff was - # a redundant list_prompt_policies() read + compose per rebind. + # notification at all. The next provider boundary rebuilds the + # actor-sensitive prefix after the new user turn has been appended. # The persona-catalog read inside the tools rebuild stays as-is: # sender-independent but handoff-frequency, not worth memoizing. self._on_mcp_tools_changed() - self._init_system_messages() def _initialize_send_generation( self, @@ -12533,6 +12632,11 @@ class ChatSession: ``_api_call`` documents the equivalent 2-way stack); every layer stops immediately on a non-retryable class. """ + principal_id = self._generation_principals.get(my_generation) + self._ensure_system_prefix_fresh( + principal_id=principal_id, + origin_generation=my_generation, + ) attempt = 0 # The latest non-empty dead attempt's flushed text. Wrapper-LOCAL # (read off the frame's consumer, never a session slot), so an @@ -12547,8 +12651,6 @@ class ChatSession: # attempt's lane. dead_provenance: TurnProvenance | None = None consumer = _StreamTurnConsumer(self, my_generation) - principal_id = self._generation_principals.get(my_generation) - debug_printed = False def _prepare(lowered: list[dict[str, Any]], lane: ModelLane) -> list[dict[str, Any]]: @@ -13854,6 +13956,9 @@ class ChatSession: """ if not rows or budget <= 0: return "" + complete = heading + "".join(rows) + if len(complete) <= budget: + return complete kept: list[str] = [] used = len(heading) for i, row in enumerate(rows): @@ -16606,62 +16711,51 @@ class ChatSession: allow_cancelled=True, ) + execution_principal = ( + (self._mcp_effective_user_id or "") if principal_id is None else principal_id + ).strip() + prepare_principal_token = _active_tool_prepare_principal.set(execution_principal) try: items = [self._safe_prepare_tool(tc) for tc in tool_calls] self._check_cancelled(my_generation) except GenerationCancelled: _stage_unstarted_tool_calls() raise - execution_principal = ( - (self._mcp_effective_user_id or "") if principal_id is None else principal_id - ).strip() + finally: + _active_tool_prepare_principal.reset(prepare_principal_token) for item in items: item["_principal_id"] = execution_principal item["_approval_cancel_witness"] = approval_cancel_witness - # Reject the read+write mix on ``tasks`` within a single - # parallel batch. ``tasks`` mutates an ordered planning - # list and supports a ``list`` read; a batch like - # ``[tasks(add=...), tasks(list)]`` has unspecified - # ordering inside ``_execute_tools.run_one``'s - # ThreadPoolExecutor — the read can land before or after - # the write and produce inconsistent state to the model. - # - # All-write and all-read batches are SAFE: - # - Writes serialise under the per-ws lock in - # ``CoordinatorClient.tasks_*``, AND a batch containing - # any ``tasks`` write runs serially in input order (see - # the run-loop branch below) so the final task list - # ordering matches what the model emitted, not the - # scheduler's acquisition order. - # - Reads can't race against anything. - # - # The rule below only fires on the MIX, so the natural - # batch shapes ("add four tasks at once", "list nodes + - # list skills + tasks(list) for a planning snapshot") are - # both permitted; only the genuinely-broken shape gets - # rejected. Non-tasks siblings paralleled with tasks() - # are unaffected — they don't touch the tasks state. + # Apply action-dependent consistency rules declared by each preparer. + # The executor deliberately knows nothing about tool names: a future + # stateful tool opts into the same read/write and serialization policy + # by attaching ``_batch_policy`` to its prepared item. if len(items) > 1: - tasks_items = [ - it - for it in items - if it.get("func_name") == "tasks" and not it.get("error") and not it.get("denied") - ] - if tasks_items: - has_read = any(it.get("action") in _TASKS_READ_ACTIONS for it in tasks_items) - has_write = any(it.get("action") in _TASKS_WRITE_ACTIONS for it in tasks_items) - if has_read and has_write: - for it in tasks_items: - it["error"] = ( - "Error: tasks(...) read (`list`) and write " - "(`add` / `update` / `remove` / `reorder`) actions " - "cannot run in the same parallel tool batch — the " - "read-after-write ordering is not guaranteed. " - "All-reads or all-writes are fine; mix only by " - "splitting them across separate assistant turns." - ) - it["needs_approval"] = False + policy_groups: dict[str, list[tuple[dict[str, Any], dict[str, Any]]]] = {} + for item in items: + policy = item.get("_batch_policy") + if item.get("error") or item.get("denied") or not isinstance(policy, dict): + continue + group = str(policy.get("group") or "") + access = str(policy.get("access") or "") + if group and access in {"read", "write"}: + policy_groups.setdefault(group, []).append((item, policy)) + for grouped_items in policy_groups.values(): + accesses = {str(policy["access"]) for _item, policy in grouped_items} + if accesses != {"read", "write"}: + continue + error = next( + ( + str(policy.get("mixed_access_error") or "") + for _item, policy in grouped_items + if policy.get("mixed_access_error") + ), + "Error: read and write actions cannot run in the same tool batch.", + ) + for item, _policy in grouped_items: + item["error"] = error + item["needs_approval"] = False # Intent validation (advisory, non-blocking). Detach the prior main # generation under the same lifecycle lock used by callback delivery @@ -16874,7 +16968,7 @@ class ChatSession: raise GenerationCancelled() started_call_ids.add(str(item.get("call_id") or "")) execute_item = item - if item.get("func_name") in {"task_agent", "web_fetch"}: + if item.get("_needs_origin_context"): # Synchronization objects are execution-only state. Keep # them out of the prepared item that crosses the intent # judge, approval UI, and any serialization boundary. @@ -16883,6 +16977,11 @@ class ChatSession: "_origin_cancel_event": execution_cancel_event, "_origin_generation": my_generation, } + if item.get("_requires_fresh_system_prefix"): + self._ensure_system_prefix_fresh( + principal_id=str(item.get("_principal_id") or "").strip(), + origin_generation=my_generation, + ) result: tuple[str, str | list[dict[str, Any]]] = item["execute"](execute_item) return result except (KeyboardInterrupt, GenerationCancelled): @@ -16930,22 +17029,16 @@ class ChatSession: if len(items) == 1: results = [run_one(items[0])] else: - # When the batch contains any ``tasks`` write, run every - # item serially in input order. ``tasks_add`` appends - # under a per-ws lock; a parallel ThreadPoolExecutor's - # scheduler-dependent acquisition order would otherwise - # produce a final task list whose ordering varies - # run-to-run, even though the SET of tasks is consistent. - # The model emitted the writes in a particular order; - # respecting that is the deterministic shape both - # operators and the model expect. Other batches stay - # parallel — the perf payoff is real and there's no - # ordering hazard against state outside ``tasks``. - has_tasks_write = any( - it.get("func_name") == "tasks" and it.get("action") in _TASKS_WRITE_ACTIONS - for it in items + # A preparer can require model-order execution for a write whose + # observable result depends on sibling ordering. Serialize the + # whole batch so unrelated siblings cannot interleave between + # those writes; ordinary batches retain parallel execution. + serialize_batch = any( + isinstance(policy := item.get("_batch_policy"), dict) + and bool(policy.get("serialize")) + for item in items ) - if has_tasks_write: + if serialize_batch: results = [run_one(it) for it in items] else: with concurrent.futures.ThreadPoolExecutor(max_workers=4) as pool: @@ -17021,6 +17114,12 @@ class ChatSession: } def _prepare_tool(self, tc: dict[str, Any]) -> dict[str, Any]: + """Prepare one tool call and stamp its immutable acting principal.""" + item = self._prepare_tool_item(tc) + item["_principal_id"] = self._tool_prepare_principal_id() + return item + + def _prepare_tool_item(self, tc: dict[str, Any]) -> dict[str, Any]: """Parse a tool call and prepare preview info for display.""" call_id = tc["id"] func_name = tc["function"]["name"].strip() @@ -17174,13 +17273,14 @@ class ChatSession: } preparer = preparers.get(func_name) if not preparer: + prepare_user_id = self._tool_prepare_principal_id() or None # Check if this is an MCP tool. Pass the effective ``user_id`` # (acting user on shared workstreams) so per-user pool tools # become reachable here — without this kwarg the gate stays # static-only and pool dispatch is structurally unreachable # from ``ChatSession._prepare_tool`` (RFC §3, invariant 8). if self._mcp_client and self._mcp_client.is_mcp_tool( - func_name, user_id=self._mcp_effective_user_id + func_name, user_id=prepare_user_id ): return self._prepare_mcp_tool(call_id, func_name, args) self.ui.on_error(f"Model called unknown tool: {func_name!r}") @@ -17189,7 +17289,7 @@ class ChatSession: available.extend( sorted( t["function"]["name"] - for t in self._mcp_client.get_tools(user_id=self._mcp_effective_user_id) + for t in self._mcp_client.get_tools(user_id=prepare_user_id) ) ) return { @@ -17872,6 +17972,7 @@ class ChatSession: "url": url, "question": question, "allow_private_origin": private_origin, + "_needs_origin_context": True, } def _prepare_open_preview(self, call_id: str, args: dict[str, Any]) -> dict[str, Any]: @@ -18283,192 +18384,173 @@ class ChatSession: "persona_tools": persona_tools, "persona_mcp": persona_mcp, "persona_memory": persona_memory, + "_needs_origin_context": True, + "_requires_fresh_system_prefix": True, } - def _resolve_scope_id(self, scope: str) -> str: - """Map a scope name to its scope_id. + def _memory_principal_id(self, principal_id: str | None = None) -> str: + """Resolve the actor for memory authorization.""" + if principal_id is not None: + return principal_id.strip() + return (self._acting_user_id or self._user_id or "").strip() - ``coordinator`` is COORDINATOR-only \u2014 the coord can save and - read memories in its own private namespace, but its child - interactive workstreams cannot see or write the row. This - closes the cross-session prompt-injection lane that an - adversarially-steered child would otherwise have through the - coord's system message: the coord's children consume external - content (MCP tool output, attachments) which can be steered to - plant instructions, and the new scope must not become a - delivery channel back into the parent's prompt. + def _memory_access(self, principal_id: str | None = None) -> _MemoryAccess: + """Resolve live project access for one immutable acting principal.""" + actor = self._memory_principal_id(principal_id) + attached = self._memory_attached_project_id + project_id = "" + project_name = "" + project_writable = False + if actor and attached: + from turnstone.core.auth import resolve_project_access - The containment gate is the session KIND (children are always - INTERACTIVE \u2014 :meth:`_validate_scope` rejects them before this - resolver runs), not secrecy of the scope_id value. That is - what lets the coord scope key on the durable ``user_id`` - (shared with children, visible cluster-wide as display - metadata) without widening the write surface: no lane \u2014 memory - tool or REST (``_VALID_MEMORY_SCOPES`` in ``server.py`` omits - ``coordinator``) \u2014 accepts a caller-supplied coordinator - scope_id. - """ + project_access = resolve_project_access(actor, attached) + if project_access.can_read and project_access.state == "active": + project_id = attached + project_name = project_access.name + project_writable = project_access.can_write + return _MemoryAccess( + principal_id=actor, + attached_project_id=attached, + project_id=project_id, + project_name=project_name, + project_writable=project_writable, + ) + + def _resolve_scope_id(self, scope: str, access: _MemoryAccess | None = None) -> str: + """Map a validated scope to the actor-specific storage key.""" + resolved = access or self._memory_access() if scope == "workstream": return self._ws_id if scope == "user": - return self._user_id + return resolved.principal_id if scope == "coordinator": - return self._coordinator_scope_id() + return self._coordinator_scope_id(resolved) if scope == "project": - return self._project_id + return resolved.project_id return "" - def _coordinator_scope_id(self) -> str: - """Return the user_id anchoring the ``coordinator`` memory scope, or ``""``. + def _coordinator_scope_id(self, access: _MemoryAccess | None = None) -> str: + """Return this coordinator actor's private memory namespace.""" + if self._kind != WorkstreamKind.COORDINATOR: + return "" + return (access or self._memory_access()).principal_id - Only a coordinator session has a coord scope \u2014 returns - ``self._user_id`` for ``kind == COORDINATOR``, ``""`` otherwise. - Keying on the user (not the ws_id) makes the namespace durable: - every coordinator session the same user runs shares one - orchestration memory, so notes survive close/reopen. Children - of a coord get an empty scope_id, which :meth:`_validate_scope` - translates into an explicit reject \u2014 children must use - ``workstream`` or ``user`` scope for their own memories. + @staticmethod + def _memory_prepare_error(call_id: str, header: str, error: str) -> dict[str, Any]: + return { + "call_id": call_id, + "func_name": "memory", + "header": header, + "preview": "", + "needs_approval": False, + "error": error, + } - ``""`` for an unauthenticated coordinator is unreachable in - practice (``__init__`` refuses to construct one) but kept - fail-closed: an empty scope_id never resolves to a readable or - writable namespace. - - See :meth:`_resolve_scope_id`'s docstring for the security - rationale (cross-session prompt-injection containment). - """ - if self._kind == WorkstreamKind.COORDINATOR: - return self._user_id - return "" - - def _validate_scope(self, scope: str, call_id: str) -> dict[str, Any] | None: - """Return an error dict if scope is invalid, None if OK. - - Coord sessions are isolated to coord-scope: they reject every - other scope (``global`` / ``workstream`` / ``user``) so the - coord's memory namespace stays focused on orchestration and - doesn't accidentally mutate or read user-context rows. - - Interactive sessions reject ``coordinator`` for the symmetric - reason \u2014 coord-scope rows belong to a per-user namespace read - only by that user's COORDINATOR sessions, and an IC writer - (children share the parent's user_id, so the kind check is the - gate) could otherwise be a cross-session prompt-injection lane - into the parent coord's system message. - """ - if scope == "user" and not self._user_id: - return { - "call_id": call_id, - "func_name": "memory", - "header": "\u2717 memory: user scope requires authentication", - "preview": "", - "needs_approval": False, - "error": "Error: 'user' scope requires authenticated user identity", - } - if scope == "project" and not self._project_id: - return { - "call_id": call_id, - "func_name": "memory", - "header": "✗ memory: not attached to an accessible project", - "preview": "", - "needs_approval": False, - "error": ( - "Error: 'project' scope requires this workstream to be " - "attached to a project you can access." - ), - } - if ( - scope == "coordinator" - and self._kind == WorkstreamKind.COORDINATOR - and not self._user_id - ): - # Backstop for the save lane: search/list reject empty - # scope_ids in _exec_memory, but save would otherwise write - # a ("coordinator", "") row shared by every unauthenticated - # session. Unreachable through real hosts (__init__ refuses - # COORDINATOR without a user_id); guards test doubles and - # future hosts. Kind-scoped so non-coordinator callers keep - # the clearer kind-mismatch error below regardless of their - # auth state. - return { - "call_id": call_id, - "func_name": "memory", - "header": "\u2717 memory: coordinator scope requires authentication", - "preview": "", - "needs_approval": False, - "error": "Error: 'coordinator' scope requires authenticated user identity", - } - if self._kind == WorkstreamKind.COORDINATOR and scope not in ( - "coordinator", - "project", - ): - return { - "call_id": call_id, - "func_name": "memory", - "header": f"\u2717 memory: scope '{scope}' unavailable to coordinator", - "preview": "", - "needs_approval": False, - "error": ( - f"Error: '{scope}' scope is not available to coordinator " - "sessions. Coord sessions only see and write the " - "'coordinator' scope \u2014 their orchestration namespace is " - "isolated from the user's interactive memory. Use " - "scope='coordinator' or omit scope (it defaults to " - "'coordinator' for coord sessions)." - ), - } - if scope == "coordinator" and self._kind != WorkstreamKind.COORDINATOR: - return { - "call_id": call_id, - "func_name": "memory", - "header": "\u2717 memory: coordinator scope unavailable", - "preview": "", - "needs_approval": False, - "error": ( - "Error: 'coordinator' scope is only valid for coordinator " - "sessions. This is an interactive workstream \u2014 use " - "'workstream' or 'user' scope for context private to this " - "session, or ask the parent coordinator to manage shared " - "context on your behalf." - ), - } + def _validate_scope( + self, + scope: str, + call_id: str, + *, + access: _MemoryAccess | None = None, + write: bool = False, + ) -> dict[str, Any] | None: + """Validate one exact scope against kind, actor identity, and live ACL.""" + resolved = access or self._memory_access() + allowed = ( + {"coordinator", "project"} + if self._kind == WorkstreamKind.COORDINATOR + else {"global", "workstream", "user", "project"} + ) + if scope not in allowed: + guidance = ( + "Use coordinator, project, or omit scope to inherit the active target." + if self._kind == WorkstreamKind.COORDINATOR + else "Use global, workstream, user, or an accessible attached project." + ) + return self._memory_prepare_error( + call_id, + f"✗ memory: scope '{scope}' unavailable", + f"Error: scope '{scope}' is unavailable to this workstream kind. {guidance}", + ) + if scope in {"user", "coordinator"} and not resolved.principal_id: + return self._memory_prepare_error( + call_id, + f"✗ memory: {scope} scope requires authentication", + f"Error: '{scope}' scope requires an authenticated acting user", + ) + if scope == "project": + if not resolved.attached_project_id: + reason = "this workstream is not attached to a project" + elif not resolved.project_id: + reason = "the acting user cannot access the active attached project" + elif write and not resolved.project_writable: + reason = "the acting user has read-only access to the attached project" + else: + reason = "" + if reason: + operation = "changed" if write else "read" + return self._memory_prepare_error( + call_id, + "✗ memory: project scope unavailable", + f"Error: {reason}; project-scoped memory cannot be {operation}.", + ) return None - def _default_memory_scope(self) -> str: - """Default ``scope`` for a memory(action='save') with no explicit scope. - - An attached, WRITABLE project wins for both kinds: work done inside a - project lands in the project bucket by default instead of leaking into - the kind default (``global`` / ``coordinator``). The model can still - target another scope explicitly (a genuine cross-project ``user`` fact, - say). A read-only project session keeps the kind default — it can't - write the project anyway, so defaulting there would only hit the - write-gate. - - Otherwise: coord sessions default to ``coordinator`` (the only scope - they can write); interactive sessions default to ``global``. - """ - if self._project_id and self._project_writable: + def _inherited_memory_scope(self, access: _MemoryAccess | None = None) -> str: + """Return the single target inherited by unscoped save/get/delete.""" + resolved = access or self._memory_access() + if resolved.attached_project_id: return "project" if self._kind == WorkstreamKind.COORDINATOR: return "coordinator" return "global" - def _implicit_scope_walk(self) -> tuple[str, ...]: - """Walk for memory(action='get'/'delete') with no explicit scope. + def _default_memory_scope(self) -> str: + """Compatibility name for the single inherited target.""" + return self._inherited_memory_scope() - Coord sessions only walk ``coordinator`` \u2014 anything else would - search namespaces the coord can't write to. Interactive sessions - keep the narrowest-first walk (workstream \u2192 user \u2192 global); a - ``coordinator`` step there would always resolve to empty - scope_id and be a wasted lookup. - """ - if self._kind == WorkstreamKind.COORDINATOR: - return ("coordinator",) - return _IMPLICIT_SCOPE_WALK + def _memory_scope_miss_hint( + self, + name: str, + scopes: list[tuple[str, str]], + *, + action: str, + access: _MemoryAccess | None = None, + ) -> str: + """Point a wrong-scope miss at visible same-name rows.""" + resolved = access or self._memory_access() + attempted = set(scopes) + visible = self._visible_scopes(resolved) + matches = set(find_structured_memory_scopes(name, visible)) + matching_scopes = [ + scope + for scope, scope_id in visible + if (scope, scope_id) not in attempted and (scope, scope_id) in matches + ] + if not matching_scopes: + return "" + if len(matching_scopes) == 1: + scope = matching_scopes[0] + if action == "delete" and scope == "project" and not resolved.project_writable: + return ( + "\nHint: a memory with this name exists in scope='project', " + "but your project access is read-only." + ) + return ( + f"\nHint: a memory with this name exists in scope='{scope}'; " + f"retry with scope='{scope}'." + ) + rendered_scopes = ", ".join(f"'{scope}'" for scope in matching_scopes) + hint = ( + "\nHint: memories with this name exist in visible scopes: " + f"{rendered_scopes}; retry with the intended scope explicitly." + ) + if action == "delete" and "project" in matching_scopes and not resolved.project_writable: + hint += " Project scope is read-only." + return hint - def _visible_memory_count(self) -> int: + def _visible_memory_count(self, access: _MemoryAccess | None = None) -> int: """Count memories visible to this session. Coord sessions are isolated to their own coord-scope namespace — @@ -18478,32 +18560,22 @@ class ChatSession: (same user, different workstream) into the coord's system message, which the coord shouldn't be reasoning over. """ - if self._kind == WorkstreamKind.COORDINATOR: - scope_id = self._coordinator_scope_id() - n = 0 - if scope_id: - n += count_structured_memories(scope="coordinator", scope_id=scope_id) - if self._project_id: - n += count_structured_memories(scope="project", scope_id=self._project_id) - return n - n = count_structured_memories(scope="global") - n += count_structured_memories(scope="workstream", scope_id=self._ws_id) - if self._user_id: - n += count_structured_memories(scope="user", scope_id=self._user_id) - if self._project_id: - n += count_structured_memories(scope="project", scope_id=self._project_id) - return n + return sum( + count_structured_memories(scope=scope, scope_id=scope_id) + for scope, scope_id in self._visible_scopes(access) + ) - def _visible_scopes(self) -> list[tuple[str, str]]: + def _visible_scopes(self, access: _MemoryAccess | None = None) -> list[tuple[str, str]]: """Return the (scope, scope_id) pairs visible to this session. Coord sessions see their coord-scope; interactive sessions see global + their workstream + their user (when uid present). Either kind also sees - its attached ``project`` scope when the session resolved read access to a - project at construction. Drives the single-query visibility helpers. + its attached ``project`` scope when the acting principal currently has + read access. Drives the single-query visibility helpers. """ + resolved = access or self._memory_access() if self._kind == WorkstreamKind.COORDINATOR: - scope_id = self._coordinator_scope_id() + scope_id = self._coordinator_scope_id(resolved) # Fail-closed on an empty scope_id (unreachable through real hosts — # __init__ refuses anonymous coordinators): the storage helpers treat # a falsy scope_id as "no scope_id filter" (that's how ``global`` @@ -18514,24 +18586,30 @@ class ChatSession: coord_scopes.append(("coordinator", scope_id)) # A coordinator attached to a project also recalls the shared project # bucket (read + write), alongside its isolated coordinator scope. - if self._project_id: - coord_scopes.append(("project", self._project_id)) + if resolved.project_id: + coord_scopes.append(("project", resolved.project_id)) return coord_scopes scopes: list[tuple[str, str]] = [("global", ""), ("workstream", self._ws_id)] - if self._user_id: - scopes.append(("user", self._user_id)) - if self._project_id: - scopes.append(("project", self._project_id)) + if resolved.principal_id: + scopes.append(("user", resolved.principal_id)) + if resolved.project_id: + scopes.append(("project", resolved.project_id)) return scopes - def _list_visible_memories(self, mem_type: str = "", limit: int = 50) -> list[dict[str, str]]: + def _list_visible_memories( + self, + mem_type: str = "", + limit: int = 50, + *, + access: _MemoryAccess | None = None, + ) -> list[dict[str, str]]: """List memories visible to this session with optional type filter. Single SQL round-trip — collapses the prior per-scope fan-out. See :meth:`_visible_memory_count` for the coord-isolation rule. """ return list_visible_structured_memories( - self._visible_scopes(), mem_type=mem_type, limit=limit + self._visible_scopes(access), mem_type=mem_type, limit=limit ) def _search_visible_memories( @@ -18540,7 +18618,12 @@ class ChatSession: mem_type: str = "", limit: int = 20, *, - cache_updates: dict[tuple[str, str, int], list[dict[str, str]]] | None = None, + access: _MemoryAccess | None = None, + cache_updates: dict[ + tuple[str, str, int, tuple[tuple[str, str], ...]], + list[dict[str, str]], + ] + | None = None, ) -> list[dict[str, str]]: """Search memories visible to this session (scope-filtered). @@ -18550,7 +18633,8 @@ class ChatSession: Cache is cleared on each new user turn and after memory writes/deletes. See :meth:`_visible_memory_count` for the coord-isolation rule. """ - cache_key = (query, mem_type, limit) + scopes = tuple(self._visible_scopes(access)) + cache_key = (query, mem_type, limit, scopes) cached = self._mem_search_cache.get(cache_key) if cached is not None: return cached @@ -18559,7 +18643,7 @@ class ChatSession: if planned is not None: return planned rows = search_visible_structured_memories( - query, self._visible_scopes(), mem_type=mem_type, limit=limit + query, list(scopes), mem_type=mem_type, limit=limit ) if cache_updates is None: self._mem_search_cache[cache_key] = rows @@ -18569,14 +18653,22 @@ class ChatSession: def _invalidate_memory_cache(self) -> None: """Drop the per-turn search cache; call on user-turn append + memory writes.""" - self._mem_search_cache.clear() - self._touched_memory_keys.clear() + with self._system_prefix_lock: + self._mem_search_cache.clear() + self._touched_memory_keys.clear() + self._system_prefix_epoch = getattr(self, "_system_prefix_epoch", 0) + 1 + self._system_prefix_dirty = True def _select_memory_candidates( self, context: str, *, - cache_updates: dict[tuple[str, str, int], list[dict[str, str]]] | None = None, + access: _MemoryAccess | None = None, + cache_updates: dict[ + tuple[str, str, int, tuple[tuple[str, str], ...]], + list[dict[str, str]], + ] + | None = None, ) -> tuple[list[dict[str, str]], str]: """Pick the candidate set fed into BM25 ranking. @@ -18602,15 +18694,16 @@ class ChatSession: """ fetch_limit = self._mem_cfg.fetch_limit if not context: - return self._list_visible_memories(limit=fetch_limit), "recency" + return self._list_visible_memories(limit=fetch_limit, access=access), "recency" search_hits = self._search_visible_memories( context, limit=fetch_limit, + access=access, cache_updates=cache_updates, ) if len(search_hits) >= fetch_limit: return search_hits, "search" - recency = self._list_visible_memories(limit=fetch_limit) + recency = self._list_visible_memories(limit=fetch_limit, access=access) seen = {m["memory_id"] for m in search_hits} extra = [m for m in recency if m["memory_id"] not in seen] if not search_hits: @@ -21360,13 +21453,13 @@ class ChatSession: if self._coord_client is None: return self._coord_tool_error(call_id, "tasks", "coordinator client unavailable") action = self._coord_str_arg(args, "action").strip().lower() - if action not in {"add", "update", "remove", "reorder", "list"}: + if action not in _TASKS_READ_ACTIONS | _TASKS_WRITE_ACTIONS: return self._coord_tool_error( call_id, "tasks", "action must be one of: add, update, remove, reorder, list", ) - if action == "list": + if action in _TASKS_READ_ACTIONS: return { "call_id": call_id, "func_name": "tasks", @@ -21375,6 +21468,11 @@ class ChatSession: "needs_approval": False, "execute": self._exec_tasks, "action": "list", + "_batch_policy": _batch_policy( + "tasks", + "read", + _TASKS_MIXED_BATCH_ERROR, + ), } # --- mutating actions ------------------------------------------------- item: dict[str, Any] = { @@ -21383,6 +21481,12 @@ class ChatSession: "needs_approval": True, "execute": self._exec_tasks, "action": action, + "_batch_policy": _batch_policy( + "tasks", + "write", + _TASKS_MIXED_BATCH_ERROR, + serialize=True, + ), } # Deferred import shared by every mutating branch below — ONE site, @@ -21928,6 +22032,7 @@ class ChatSession: def _prepare_memory(self, call_id: str, args: dict[str, Any]) -> dict[str, Any]: """Prepare a memory tool action (save/get/search/delete/list).""" action = (args.get("action") or "").strip().lower() + access = self._memory_access(self._tool_prepare_principal_id() or None) if action == "save": name = (args.get("name") or args.get("key") or "").strip() @@ -21942,6 +22047,12 @@ class ChatSession: "needs_approval": False, "error": "Error: 'name' is required for save", } + if len(name) > 256: + return self._memory_prepare_error( + call_id, + "✗ memory save: name too long", + "Error: memory name exceeds 256 characters", + ) if not content: return { "call_id": call_id, @@ -21960,43 +22071,34 @@ class ChatSession: "needs_approval": False, "error": f"Error: content exceeds {self._mem_cfg.max_content} character limit", } - # None (field omitted) means "leave unset": the upsert keeps the - # stored value on update and defaults on insert; an explicit value - # (including "" / "general") overwrites. - description = args.get("description") - if description is not None: - description = str(description).strip() + description = str(args.get("description") or "").strip() + if not description: + return self._memory_prepare_error( + call_id, + "✗ memory save: missing description", + "Error: 'description' must be non-empty for save", + ) mem_type = args.get("type") if mem_type is not None: mem_type = str(mem_type).strip().lower() if mem_type not in ("user", "general", "feedback", "reference"): - # An unrecognized type (e.g. a typo) is treated as unset - # (preserve the stored type on update / default on insert) - # rather than silently overwriting it with "general". - mem_type = None - # Default scope is kind-aware: coord sessions default to - # ``coordinator`` (their only writable scope); IC sessions - # default to ``global`` (matches pre-fix behaviour). - default_scope = self._default_memory_scope() - scope = (args.get("scope") or default_scope).strip().lower() + return self._memory_prepare_error( + call_id, + "✗ memory save: invalid type", + f"Error: invalid memory type '{mem_type}'", + ) + default_scope = self._inherited_memory_scope(access) + scope = str(args.get("scope") or default_scope).strip().lower() if scope not in _VALID_MEMORY_SCOPES: - scope = default_scope - scope_err = self._validate_scope(scope, call_id) + return self._memory_prepare_error( + call_id, + "✗ memory save: invalid scope", + f"Error: invalid scope '{scope}'. Valid: {', '.join(_VALID_MEMORY_SCOPES)}", + ) + scope_err = self._validate_scope(scope, call_id, access=access, write=True) if scope_err: return scope_err - if scope == "project" and not self._project_writable: - return { - "call_id": call_id, - "func_name": "memory", - "header": "\u2717 memory save: project is read-only for you", - "preview": "", - "needs_approval": False, - "error": ( - "Error: you have read-only access to this project; you " - "cannot save project-scoped memory." - ), - } - scope_id = self._resolve_scope_id(scope) + scope_id = self._resolve_scope_id(scope, access) return { "call_id": call_id, "func_name": "memory", @@ -22011,6 +22113,12 @@ class ChatSession: "mem_type": mem_type, "scope": scope, "scope_id": scope_id, + "_batch_policy": _batch_policy( + "memory", + "write", + _MEMORY_MIXED_BATCH_ERROR, + serialize=True, + ), } if action == "get": @@ -22024,6 +22132,12 @@ class ChatSession: "needs_approval": False, "error": "Error: 'name' is required for get", } + if len(name) > 256: + return self._memory_prepare_error( + call_id, + "✗ memory get: name too long", + "Error: memory name exceeds 256 characters", + ) explicit_scope = (args.get("scope") or "").strip().lower() valid_scopes = _VALID_MEMORY_SCOPES if explicit_scope and explicit_scope not in valid_scopes: @@ -22035,21 +22149,11 @@ class ChatSession: "needs_approval": False, "error": f"Error: invalid scope '{explicit_scope}'. Valid: {', '.join(valid_scopes)}", } - if explicit_scope: - scope_err = self._validate_scope(explicit_scope, call_id) - if scope_err: - return scope_err - scopes_to_try = [(explicit_scope, self._resolve_scope_id(explicit_scope))] - else: - # Implicit fallback walk \u2014 kind-aware narrowest-to-widest. - # Coord sessions only walk ``coordinator``; IC sessions - # walk workstream \u2192 user \u2192 global. See - # :meth:`_implicit_scope_walk`. - scopes_to_try = [] - for s in self._implicit_scope_walk(): - sid = self._resolve_scope_id(s) - if sid or s == "global": - scopes_to_try.append((s, sid)) + scope = explicit_scope or self._inherited_memory_scope(access) + scope_err = self._validate_scope(scope, call_id, access=access) + if scope_err: + return scope_err + scopes_to_try = [(scope, self._resolve_scope_id(scope, access))] return { "call_id": call_id, "func_name": "memory", @@ -22060,6 +22164,11 @@ class ChatSession: "action": "get", "name": name, "scopes_to_try": scopes_to_try, + "_batch_policy": _batch_policy( + "memory", + "read", + _MEMORY_MIXED_BATCH_ERROR, + ), } if action == "delete": @@ -22073,6 +22182,12 @@ class ChatSession: "needs_approval": False, "error": "Error: name is required for delete", } + if len(name) > 256: + return self._memory_prepare_error( + call_id, + "✗ memory delete: name too long", + "Error: memory name exceeds 256 characters", + ) explicit_scope = (args.get("scope") or "").strip().lower() valid_scopes = _VALID_MEMORY_SCOPES if explicit_scope and explicit_scope not in valid_scopes: @@ -22087,38 +22202,11 @@ class ChatSession: f"Valid scopes: {', '.join(valid_scopes)}" ), } - if explicit_scope: - scope_err = self._validate_scope(explicit_scope, call_id) - if scope_err: - return scope_err - # _validate_scope gates on READ access (_project_id); deleting a - # shared project row is a WRITE, so gate it on _project_writable - # too — mirrors the save path so a read-only member of a public - # project can't destroy project-scoped memory. (The implicit - # walk below never includes project scope, so it needs no gate.) - if explicit_scope == "project" and not self._project_writable: - return { - "call_id": call_id, - "func_name": "memory", - "header": "✗ memory delete: project is read-only for you", - "preview": "", - "needs_approval": False, - "error": ( - "Error: you have read-only access to this project; you " - "cannot delete project-scoped memory." - ), - } - scope_id = self._resolve_scope_id(explicit_scope) - scopes_to_try = [(explicit_scope, scope_id)] - else: - # Kind-aware implicit walk — coord sessions stay in coord-scope; - # IC sessions walk narrowest-to-widest (workstream → user → global). - # See :meth:`_implicit_scope_walk`. - scopes_to_try = [] - for s in self._implicit_scope_walk(): - sid = self._resolve_scope_id(s) - if sid or s == "global": - scopes_to_try.append((s, sid)) + scope = explicit_scope or self._inherited_memory_scope(access) + scope_err = self._validate_scope(scope, call_id, access=access, write=True) + if scope_err: + return scope_err + scopes_to_try = [(scope, self._resolve_scope_id(scope, access))] return { "call_id": call_id, "func_name": "memory", @@ -22129,27 +22217,43 @@ class ChatSession: "action": "delete", "name": name, "scopes_to_try": scopes_to_try, + "_batch_policy": _batch_policy( + "memory", + "write", + _MEMORY_MIXED_BATCH_ERROR, + serialize=True, + ), } if action == "search": query = (args.get("query") or "").strip() mem_type = (args.get("type") or "").strip().lower() if mem_type and mem_type not in ("user", "general", "feedback", "reference"): - mem_type = "" + return self._memory_prepare_error( + call_id, + "✗ memory search: invalid type", + f"Error: invalid memory type '{mem_type}'", + ) scope = (args.get("scope") or "").strip().lower() if scope and scope not in _VALID_MEMORY_SCOPES: - scope = "" + return self._memory_prepare_error( + call_id, + "✗ memory search: invalid scope", + f"Error: invalid scope '{scope}'. Valid: {', '.join(_VALID_MEMORY_SCOPES)}", + ) if scope: - scope_err = self._validate_scope(scope, call_id) + scope_err = self._validate_scope(scope, call_id, access=access) if scope_err: return scope_err - scope_id = self._resolve_scope_id(scope) if scope else "" - limit = args.get("limit", 20) - if isinstance(limit, str): - try: - limit = int(limit) - except ValueError: - limit = 20 + scope_id = self._resolve_scope_id(scope, access) if scope else "" + try: + limit = int(args.get("limit", 20)) + except (TypeError, ValueError): + return self._memory_prepare_error( + call_id, + "✗ memory search: invalid limit", + "Error: limit must be an integer", + ) return { "call_id": call_id, "func_name": "memory", @@ -22163,26 +22267,41 @@ class ChatSession: "scope": scope, "scope_id": scope_id, "limit": max(1, min(limit, 50)), + "_batch_policy": _batch_policy( + "memory", + "read", + _MEMORY_MIXED_BATCH_ERROR, + ), } if action == "list": mem_type = (args.get("type") or "").strip().lower() if mem_type and mem_type not in ("user", "general", "feedback", "reference"): - mem_type = "" + return self._memory_prepare_error( + call_id, + "✗ memory list: invalid type", + f"Error: invalid memory type '{mem_type}'", + ) scope = (args.get("scope") or "").strip().lower() if scope and scope not in _VALID_MEMORY_SCOPES: - scope = "" + return self._memory_prepare_error( + call_id, + "✗ memory list: invalid scope", + f"Error: invalid scope '{scope}'. Valid: {', '.join(_VALID_MEMORY_SCOPES)}", + ) if scope: - scope_err = self._validate_scope(scope, call_id) + scope_err = self._validate_scope(scope, call_id, access=access) if scope_err: return scope_err - scope_id = self._resolve_scope_id(scope) if scope else "" - limit = args.get("limit", 20) - if isinstance(limit, str): - try: - limit = int(limit) - except ValueError: - limit = 20 + scope_id = self._resolve_scope_id(scope, access) if scope else "" + try: + limit = int(args.get("limit", 20)) + except (TypeError, ValueError): + return self._memory_prepare_error( + call_id, + "✗ memory list: invalid limit", + "Error: limit must be an integer", + ) return { "call_id": call_id, "func_name": "memory", @@ -22195,6 +22314,11 @@ class ChatSession: "scope": scope, "scope_id": scope_id, "limit": max(1, min(limit, 50)), + "_batch_policy": _batch_policy( + "memory", + "read", + _MEMORY_MIXED_BATCH_ERROR, + ), } return { @@ -22240,7 +22364,7 @@ class ChatSession: # the queue must search as the user whose turn requested it, # not whoever binds the session later (same discipline as # ``mcp_user_id`` in ``_prepare_mcp_tool``). - "scope_user_id": self._history_scope_user_id(), + "scope_user_id": self._tool_prepare_principal_id() or None, } # -- skill prepare/execute ------------------------------------------------- @@ -22274,7 +22398,7 @@ class ChatSession: # Pin the credential identity at prepare time: an item that # sits pending approval must execute under the user whose # turn requested it, not whoever binds the session later. - "mcp_user_id": self._mcp_effective_user_id, + "mcp_user_id": self._tool_prepare_principal_id() or None, } def _exec_mcp_tool(self, item: dict[str, Any]) -> tuple[str, str]: @@ -24436,6 +24560,7 @@ class ChatSession: scope: str, scope_id: str, mem_type: str, + principal_id: str, ) -> None: """Emit an audit row for a mutating memory tool action. @@ -24468,7 +24593,7 @@ class ChatSession: } record_audit( get_storage(), - self._user_id, + principal_id, action, "memory", memory_id, @@ -24481,113 +24606,134 @@ class ChatSession: """Execute a memory tool action.""" call_id = item["call_id"] action = item["action"] + # Real dispatch stamps every prepared tool item generically. Never + # substitute the mutable session actor here: an unstamped item must not + # acquire whichever principal happens to be bound at execution time. + principal_id = str(item.get("_principal_id") or "").strip() try: if action == "save": - row, was_update = save_structured_memory( - item["name"], - item["content"], - description=item["description"], - mem_type=item["mem_type"], - scope=item["scope"], - scope_id=item["scope_id"], - ) - if not row: - msg = f"Error: failed to save memory '{item['name']}'" - self._report_tool_result(call_id, "memory", msg, is_error=True) - return call_id, msg - # Invalidate the per-turn search cache so an in-turn - # memory(search)/(list) reflects this write. Deliberately do NOT - # recompose the system prefix here: injected memories ride in the - # cached system block, so re-initing on every save/update busts - # the prompt cache (a full system + history re-write) — for a - # memory the model already holds via this tool result. The write - # folds into the prefix at the next natural recompose - # (skill/model/MCP/resume/compaction) or the next session. - self._invalidate_memory_cache() - self._audit_memory_event( - "memory.update" if was_update else "memory.save", - row["memory_id"], - name=row["name"], - scope=row["scope"], - scope_id=row["scope_id"], - mem_type=row["type"], - ) + with self._system_prefix_lock: + access = self._memory_access(principal_id) + scope = str(item["scope"]) + scope_err = self._validate_scope( + scope, + call_id, + access=access, + write=True, + ) + if scope_err: + msg = str(scope_err["error"]) + self._report_tool_result(call_id, "memory", msg, is_error=True) + return call_id, msg + scope_id = self._resolve_scope_id(scope, access) + row, was_update = save_structured_memory_strict( + item["name"], + item["content"], + description=item["description"], + mem_type=item["mem_type"], + scope=scope, + scope_id=scope_id, + require_active_project=scope == "project", + ) + self._invalidate_memory_cache() + self._audit_memory_event( + "memory.update" if was_update else "memory.save", + row["memory_id"], + name=row["name"], + scope=row["scope"], + scope_id=row["scope_id"], + mem_type=row["type"], + principal_id=principal_id, + ) verb = "Updated" if was_update else "Saved" msg = f"{verb} memory '{row['name']}' (type={row['type']}, scope={row['scope']})" self._report_tool_result(call_id, "memory", msg) return call_id, msg if action == "get": - scopes = item["scopes_to_try"] - mem = None - found_scope = "" - for scope, scope_id in scopes: - mem = get_structured_memory_by_name(item["name"], scope, scope_id) - if mem: - found_scope = scope - break + access = self._memory_access(principal_id) + scope = str(item["scopes_to_try"][0][0]) + scope_err = self._validate_scope(scope, call_id, access=access) + if scope_err: + msg = str(scope_err["error"]) + self._report_tool_result(call_id, "memory", msg, is_error=True) + return call_id, msg + scope_id = self._resolve_scope_id(scope, access) + scopes = [(scope, scope_id)] + mem = get_structured_memory_by_name_strict(item["name"], scope, scope_id) if mem: self._touch_read_memories([mem]) content = mem.get("content", "") desc = mem.get("description", "") mem_type = mem.get("type", "") - header = f"[{mem_type}:{found_scope}] {item['name']}" + header = f"[{mem_type}:{scope}] {item['name']}" if desc: header += f" — {desc}" msg = f"{header}\n\n{content}" else: - tried = ", ".join(s for s, _ in scopes) - msg = f"Error: memory '{item['name']}' not found (searched scopes: {tried})" + msg = f"Error: memory '{item['name']}' not found (scope={scope})" + msg += self._memory_scope_miss_hint( + item["name"], scopes, action="get", access=access + ) self._report_tool_result(call_id, "memory", msg, is_error=mem is None) return call_id, msg if action == "delete": - scopes = item["scopes_to_try"] - deleted: dict[str, str] | None = None - deleted_scope = "" - deleted_scope_id = "" - # Look up first so the audit row can record the deleted - # memory_id + type (delete-by-name returns only a bool). - # Falling back through the scope walk keeps the current - # narrowest-first IC semantics; coord sessions only see - # ``coordinator`` here. - for scope, scope_id in scopes: - existing = get_structured_memory_by_name(item["name"], scope, scope_id) - if existing and delete_structured_memory_by_id(existing["memory_id"]): - deleted = existing - deleted_scope = scope - deleted_scope_id = scope_id - break - if deleted is None: - tried = ", ".join(s for s, _ in scopes) - msg = f"Error: memory '{item['name']}' not found (searched scopes: {tried})" - self._report_tool_result(call_id, "memory", msg, is_error=True) - else: + with self._system_prefix_lock: + access = self._memory_access(principal_id) + scope = str(item["scopes_to_try"][0][0]) + scope_err = self._validate_scope( + scope, + call_id, + access=access, + write=True, + ) + scope_id = self._resolve_scope_id(scope, access) + scopes = [(scope, scope_id)] + if scope_err: + msg = str(scope_err["error"]) + msg += self._memory_scope_miss_hint( + item["name"], scopes, action="delete", access=access + ) + self._report_tool_result(call_id, "memory", msg, is_error=True) + return call_id, msg + deleted = delete_structured_memory_returning_strict( + item["name"], scope, scope_id + ) + if deleted is None: + msg = f"Error: memory '{item['name']}' not found (scope={scope})" + msg += self._memory_scope_miss_hint( + item["name"], scopes, action="delete", access=access + ) + self._report_tool_result(call_id, "memory", msg, is_error=True) + return call_id, msg self._invalidate_memory_cache() - self._init_system_messages() self._audit_memory_event( "memory.delete", deleted["memory_id"], name=item["name"], - scope=deleted_scope, - scope_id=deleted_scope_id, + scope=scope, + scope_id=scope_id, mem_type=deleted.get("type", ""), + principal_id=principal_id, ) - msg = f"Deleted memory '{item['name']}' (scope={deleted_scope})" - self._report_tool_result(call_id, "memory", msg) + msg = f"Deleted memory '{item['name']}' (scope={scope})" + self._report_tool_result(call_id, "memory", msg) return call_id, msg if action == "search": + access = self._memory_access(principal_id) scope = item.get("scope", "") - scope_id = item.get("scope_id", "") - # Defense-in-depth: reject scoped queries with empty scope_id - if scope in ("user", "workstream", "coordinator") and not scope_id: - msg = f"Error: '{scope}' scope requires a valid identity" - self._report_tool_result(call_id, "memory", msg, is_error=True) - return call_id, msg + storage = get_storage() if scope: - rows = search_structured_memories( + scope_err = self._validate_scope(scope, call_id, access=access) + if scope_err: + msg = str(scope_err["error"]) + self._report_tool_result(call_id, "memory", msg, is_error=True) + return call_id, msg + scope_id = self._resolve_scope_id(scope, access) + rows = storage.search_structured_memories( item["query"], mem_type=item.get("mem_type", ""), scope=scope, @@ -24595,8 +24741,9 @@ class ChatSession: limit=item["limit"], ) else: - rows = self._search_visible_memories( + rows = storage.search_visible_structured_memories( item["query"], + self._visible_scopes(access), mem_type=item.get("mem_type", ""), limit=item["limit"], ) @@ -24618,7 +24765,10 @@ class ChatSession: f" [{m['type']}:{m['scope']}] {m['name']}{desc}\n {preview}" ) msg = f"Memories ({len(rows)} results):\n" + "\n".join(lines) - msg += "\n\nUse memory(action='get', name='...') for full content." + msg += ( + "\n\nFor full content, call memory(action='get') with the " + "displayed name and scope." + ) else: msg = ( f"No memories found for '{item['query']}'." @@ -24629,21 +24779,25 @@ class ChatSession: return call_id, msg if action == "list": + access = self._memory_access(principal_id) scope = item.get("scope", "") - scope_id = item.get("scope_id", "") - if scope in ("user", "workstream", "coordinator") and not scope_id: - msg = f"Error: '{scope}' scope requires a valid identity" - self._report_tool_result(call_id, "memory", msg, is_error=True) - return call_id, msg + storage = get_storage() if scope: - rows = list_structured_memories( + scope_err = self._validate_scope(scope, call_id, access=access) + if scope_err: + msg = str(scope_err["error"]) + self._report_tool_result(call_id, "memory", msg, is_error=True) + return call_id, msg + scope_id = self._resolve_scope_id(scope, access) + rows = storage.list_structured_memories( mem_type=item.get("mem_type", ""), scope=scope, scope_id=scope_id, limit=item["limit"], ) else: - rows = self._list_visible_memories( + rows = storage.list_visible_structured_memories( + self._visible_scopes(access), mem_type=item.get("mem_type", ""), limit=item["limit"], ) @@ -24658,14 +24812,23 @@ class ChatSession: f" [{m['type']}:{m['scope']}] {m['name']}{desc}\n {preview}" ) msg = f"Memories ({len(rows)}):\n" + "\n".join(lines) - msg += "\n\nUse memory(action='get', name='...') for full content." + msg += ( + "\n\nFor full content, call memory(action='get') with the " + "displayed name and scope." + ) else: msg = "No memories stored." self._report_tool_result(call_id, "memory", msg) return call_id, msg - except Exception as e: - msg = f"Error: {e}" + except Exception: + log.warning( + "memory.tool_storage_failed", + action=action, + principal_id=principal_id, + exc_info=True, + ) + msg = "Error: memory storage operation failed; retry after storage recovers." self._report_tool_result(call_id, "memory", msg, is_error=True) return call_id, msg diff --git a/turnstone/core/storage/_postgresql.py b/turnstone/core/storage/_postgresql.py index 409810cd..29cbf163 100644 --- a/turnstone/core/storage/_postgresql.py +++ b/turnstone/core/storage/_postgresql.py @@ -4746,6 +4746,9 @@ class PostgreSQLBackend(_KeyedAttachmentSaveWrappers): scope_id: str, content: str, ) -> None: + if description is None or not description.strip(): + raise ValueError("memory description is required and must be non-empty") + description = description.strip() now = datetime.now(UTC).strftime("%Y-%m-%dT%H:%M:%S") with self._conn() as conn: conn.execute( @@ -4770,19 +4773,24 @@ class PostgreSQLBackend(_KeyedAttachmentSaveWrappers): self, memory_id: str, name: str, - description: str | None, + description: str, mem_type: str | None, scope: str, scope_id: str, content: str, + *, + require_active_project: bool = False, ) -> tuple[dict[str, str], bool]: from sqlalchemy.dialects.postgresql import insert as pg_insert + if description is None or not description.strip(): + raise ValueError("memory description is required and must be non-empty") + description = description.strip() now = datetime.now(UTC).strftime("%Y-%m-%dT%H:%M:%S") insert_stmt = pg_insert(structured_memories).values( memory_id=memory_id, name=name, - description="" if description is None else description, + description=description, type="general" if mem_type is None else mem_type, scope=scope, scope_id=scope_id, @@ -4792,16 +4800,14 @@ class PostgreSQLBackend(_KeyedAttachmentSaveWrappers): last_accessed=now, access_count=0, ) - # On conflict, refresh content + timestamps. description/type are - # overwritten only when the caller supplied them; None means "unset" -> - # keep the stored value. created and access_count are left untouched. + # On conflict, refresh content, description, and timestamps. A None type means + # "unset" -> keep the stored value. created/access_count stay untouched. set_: dict[str, Any] = { "content": insert_stmt.excluded.content, "updated": now, "last_accessed": now, } - if description is not None: - set_["description"] = insert_stmt.excluded.description + set_["description"] = insert_stmt.excluded.description if mem_type is not None: set_["type"] = insert_stmt.excluded.type stmt = insert_stmt.on_conflict_do_update( @@ -4809,6 +4815,22 @@ class PostgreSQLBackend(_KeyedAttachmentSaveWrappers): set_=set_, ).returning(structured_memories) with self._conn() as conn: + if require_active_project: + if scope != "project" or not scope_id: + raise ValueError("active-project guard requires project scope") + project = conn.execute( + sa.select(projects.c.project_id) + .where( + sa.and_( + projects.c.project_id == scope_id, + projects.c.state == "active", + ) + ) + .with_for_update(read=True) + ).fetchone() + if project is None: + conn.rollback() + raise ValueError("project is missing, archived, or no longer writable") row = conn.execute(stmt).fetchone() conn.commit() if row is None: # unreachable: ON CONFLICT DO UPDATE returns one row @@ -4841,26 +4863,58 @@ class PostgreSQLBackend(_KeyedAttachmentSaveWrappers): def delete_structured_memory( self, name: str, scope: str = "global", scope_id: str = "" ) -> bool: - with self._conn() as conn: - result = conn.execute( - sa.delete(structured_memories).where( - sa.and_( - structured_memories.c.name == name, - structured_memories.c.scope == scope, - structured_memories.c.scope_id == scope_id, - ) + return self.delete_structured_memory_returning(name, scope, scope_id) is not None + + def delete_structured_memory_returning( + self, name: str, scope: str = "global", scope_id: str = "" + ) -> dict[str, str] | None: + stmt = ( + sa.delete(structured_memories) + .where( + sa.and_( + structured_memories.c.name == name, + structured_memories.c.scope == scope, + structured_memories.c.scope_id == scope_id, ) ) + .returning(structured_memories) + ) + with self._conn() as conn: + row = conn.execute(stmt).fetchone() conn.commit() - return result.rowcount > 0 + return dict(row._mapping) if row is not None else None def delete_structured_memory_by_id(self, memory_id: str) -> bool: + return self.delete_structured_memory_by_id_returning(memory_id) is not None + + def delete_structured_memory_by_id_returning(self, memory_id: str) -> dict[str, str] | None: with self._conn() as conn: - result = conn.execute( - sa.delete(structured_memories).where(structured_memories.c.memory_id == memory_id) - ) + row = conn.execute( + sa.delete(structured_memories) + .where(structured_memories.c.memory_id == memory_id) + .returning(structured_memories) + ).fetchone() conn.commit() - return result.rowcount > 0 + return dict(row._mapping) if row is not None else None + + def find_structured_memory_scopes( + self, + name: str, + scopes: list[tuple[str, str]], + ) -> list[tuple[str, str]]: + if not scopes: + return [] + with self._conn() as conn: + scope_clauses, params = self._build_scope_or_clause(scopes) + rows = conn.execute( + sa.text( + "SELECT scope, scope_id FROM structured_memories " + f"WHERE name = :name AND ({scope_clauses}) " + "ORDER BY scope, scope_id" + ), + {**params, "name": name}, + ).fetchall() + return [(str(row.scope), str(row.scope_id)) for row in rows] def list_structured_memories( self, @@ -5905,6 +5959,17 @@ class PostgreSQLBackend(_KeyedAttachmentSaveWrappers): def delete_project(self, project_id: str) -> bool: with self._conn() as conn: + # Serialize with guarded project-memory upserts. If a writer got + # the row first, its memory is committed before our purge; if this + # delete wins, the later writer's active-project check finds no row. + project = conn.execute( + sa.select(projects.c.project_id) + .where(projects.c.project_id == project_id) + .with_for_update() + ).fetchone() + if project is None: + conn.rollback() + return False # No FK cascade in the schema family, so purge the project's scoped # memory + member rows explicitly (same transaction) before the # project row — honouring the "destroys the container AND its scoped diff --git a/turnstone/core/storage/_protocol.py b/turnstone/core/storage/_protocol.py index 76d0afe1..bf697411 100644 --- a/turnstone/core/storage/_protocol.py +++ b/turnstone/core/storage/_protocol.py @@ -829,34 +829,41 @@ class StorageBackend(Protocol): scope_id: str, content: str, ) -> None: - """Create a structured memory record.""" + """Create a structured memory record with a non-empty description.""" ... def upsert_structured_memory( self, memory_id: str, name: str, - description: str | None, + description: str, mem_type: str | None, scope: str, scope_id: str, content: str, + *, + require_active_project: bool = False, ) -> tuple[dict[str, str], bool]: """Insert a structured memory, or update it in place on a ``(name, scope, scope_id)`` conflict. Atomic ``INSERT ... ON CONFLICT DO UPDATE ... RETURNING`` — no IntegrityError round-trip, race-safe under concurrent saves of the same - key. ``description`` / ``mem_type`` of ``None`` mean "unset": the - column default ("" / "general") is used on insert and the stored value - is kept on conflict; a non-``None`` value (including "" or "general") is - written. + key. ``description`` must contain non-whitespace text on every insert + or update. A ``mem_type`` of ``None`` means "unset": the column default + is used on insert and the stored value is kept on conflict. Returns ``(row, was_update)`` (like Django's ``update_or_create``): the full saved row, and ``True`` when an existing row was updated rather than inserted. Callers MUST supply a fresh unique ``memory_id`` — it is compared against the returned row's id to tell INSERT from UPDATE, so a reused id would report ``was_update=False`` on a real update. + + When ``require_active_project`` is true, ``scope`` must be + ``"project"`` and the backend must verify that the referenced project + still exists and is active in the same transaction as the upsert. The + project row is locked where the backend supports row locks so a + concurrent project delete cannot leave an orphaned memory behind. """ ... @@ -876,10 +883,28 @@ class StorageBackend(Protocol): """Delete a structured memory by (name, scope, scope_id). Returns True if existed.""" ... + def delete_structured_memory_returning( + self, name: str, scope: str = "global", scope_id: str = "" + ) -> dict[str, str] | None: + """Atomically delete and return a memory selected by its scoped name.""" + ... + def delete_structured_memory_by_id(self, memory_id: str) -> bool: """Delete a structured memory by its primary key. Returns True if existed.""" ... + def delete_structured_memory_by_id_returning(self, memory_id: str) -> dict[str, str] | None: + """Atomically delete and return a memory selected by primary key.""" + ... + + def find_structured_memory_scopes( + self, + name: str, + scopes: list[tuple[str, str]], + ) -> list[tuple[str, str]]: + """Return visible scope pairs containing ``name`` in one small query.""" + ... + def list_structured_memories( self, mem_type: str = "", diff --git a/turnstone/core/storage/_sqlite.py b/turnstone/core/storage/_sqlite.py index 70638781..b0d84af2 100644 --- a/turnstone/core/storage/_sqlite.py +++ b/turnstone/core/storage/_sqlite.py @@ -4818,6 +4818,9 @@ class SQLiteBackend(_KeyedAttachmentSaveWrappers): scope_id: str, content: str, ) -> None: + if description is None or not description.strip(): + raise ValueError("memory description is required and must be non-empty") + description = description.strip() now = datetime.now(UTC).strftime("%Y-%m-%dT%H:%M:%S") with self._conn() as conn: conn.execute( @@ -4842,19 +4845,24 @@ class SQLiteBackend(_KeyedAttachmentSaveWrappers): self, memory_id: str, name: str, - description: str | None, + description: str, mem_type: str | None, scope: str, scope_id: str, content: str, + *, + require_active_project: bool = False, ) -> tuple[dict[str, str], bool]: from sqlalchemy.dialects.sqlite import insert as sqlite_insert + if description is None or not description.strip(): + raise ValueError("memory description is required and must be non-empty") + description = description.strip() now = datetime.now(UTC).strftime("%Y-%m-%dT%H:%M:%S") insert_stmt = sqlite_insert(structured_memories).values( memory_id=memory_id, name=name, - description="" if description is None else description, + description=description, type="general" if mem_type is None else mem_type, scope=scope, scope_id=scope_id, @@ -4864,16 +4872,14 @@ class SQLiteBackend(_KeyedAttachmentSaveWrappers): last_accessed=now, access_count=0, ) - # On conflict, refresh content + timestamps. description/type are - # overwritten only when the caller supplied them; None means "unset" -> - # keep the stored value. created and access_count are left untouched. + # On conflict, refresh content, description, and timestamps. A None type means + # "unset" -> keep the stored value. created/access_count stay untouched. set_: dict[str, Any] = { "content": insert_stmt.excluded.content, "updated": now, "last_accessed": now, } - if description is not None: - set_["description"] = insert_stmt.excluded.description + set_["description"] = insert_stmt.excluded.description if mem_type is not None: set_["type"] = insert_stmt.excluded.type stmt = insert_stmt.on_conflict_do_update( @@ -4881,6 +4887,24 @@ class SQLiteBackend(_KeyedAttachmentSaveWrappers): set_=set_, ).returning(structured_memories) with self._conn() as conn: + if require_active_project: + if scope != "project" or not scope_id: + raise ValueError("active-project guard requires project scope") + # SQLite has no row locks. Taking the writer lock before the + # existence check serializes this transaction with + # ``delete_project`` (which uses the same prologue). + conn.execute(sa.text("BEGIN IMMEDIATE")) + project = conn.execute( + sa.select(projects.c.project_id).where( + sa.and_( + projects.c.project_id == scope_id, + projects.c.state == "active", + ) + ) + ).fetchone() + if project is None: + conn.rollback() + raise ValueError("project is missing, archived, or no longer writable") row = conn.execute(stmt).fetchone() conn.commit() if row is None: # unreachable: ON CONFLICT DO UPDATE returns one row @@ -4913,26 +4937,58 @@ class SQLiteBackend(_KeyedAttachmentSaveWrappers): def delete_structured_memory( self, name: str, scope: str = "global", scope_id: str = "" ) -> bool: - with self._conn() as conn: - result = conn.execute( - sa.delete(structured_memories).where( - sa.and_( - structured_memories.c.name == name, - structured_memories.c.scope == scope, - structured_memories.c.scope_id == scope_id, - ) + return self.delete_structured_memory_returning(name, scope, scope_id) is not None + + def delete_structured_memory_returning( + self, name: str, scope: str = "global", scope_id: str = "" + ) -> dict[str, str] | None: + stmt = ( + sa.delete(structured_memories) + .where( + sa.and_( + structured_memories.c.name == name, + structured_memories.c.scope == scope, + structured_memories.c.scope_id == scope_id, ) ) + .returning(structured_memories) + ) + with self._conn() as conn: + row = conn.execute(stmt).fetchone() conn.commit() - return result.rowcount > 0 + return dict(row._mapping) if row is not None else None def delete_structured_memory_by_id(self, memory_id: str) -> bool: + return self.delete_structured_memory_by_id_returning(memory_id) is not None + + def delete_structured_memory_by_id_returning(self, memory_id: str) -> dict[str, str] | None: with self._conn() as conn: - result = conn.execute( - sa.delete(structured_memories).where(structured_memories.c.memory_id == memory_id) - ) + row = conn.execute( + sa.delete(structured_memories) + .where(structured_memories.c.memory_id == memory_id) + .returning(structured_memories) + ).fetchone() conn.commit() - return result.rowcount > 0 + return dict(row._mapping) if row is not None else None + + def find_structured_memory_scopes( + self, + name: str, + scopes: list[tuple[str, str]], + ) -> list[tuple[str, str]]: + if not scopes: + return [] + with self._conn() as conn: + scope_clauses, params = self._build_scope_or_clause(scopes) + rows = conn.execute( + sa.text( + "SELECT scope, scope_id FROM structured_memories " + f"WHERE name = :name AND ({scope_clauses}) " + "ORDER BY scope, scope_id" + ), + {**params, "name": name}, + ).fetchall() + return [(str(row.scope), str(row.scope_id)) for row in rows] def list_structured_memories( self, @@ -5961,6 +6017,9 @@ class SQLiteBackend(_KeyedAttachmentSaveWrappers): def delete_project(self, project_id: str) -> bool: with self._conn() as conn: + # Serialize with guarded project-memory upserts before inspecting + # or deleting the container. + conn.execute(sa.text("BEGIN IMMEDIATE")) # No FK cascade in the schema family, so purge the project's scoped # memory + member rows explicitly (same transaction) before the # project row — honouring the "destroys the container AND its scoped diff --git a/turnstone/core/tools.py b/turnstone/core/tools.py index 3eb1c3fd..945431ae 100644 --- a/turnstone/core/tools.py +++ b/turnstone/core/tools.py @@ -64,12 +64,11 @@ def _apply_kind_variant(tool: dict[str, Any], kind: str, meta: dict[str, Any]) - """Return a kind-specific copy of ``tool`` with description / params overridden. Each kind sees only the surface it can actually use — for ``memory``, - coord sessions get a description + scope enum that mention only the - ``coordinator`` scope, while interactive sessions get a description - + scope enum that omit ``coordinator`` entirely. This keeps the - LLM contract tight: the model never sees enum values it can't use, - and never reads description sentences explaining why a scope is - forbidden. + coord sessions get ``coordinator`` plus the attach-dependent ``project`` + scope, while interactive sessions get global/workstream/user plus + ``project``. This keeps the LLM contract tight: the model never sees enum + values it can't use, and never reads description sentences explaining why + a scope is forbidden. No-op (returns the input tool unchanged) when the tool has no ``kind_variants`` metadata or no entry for ``kind``. Otherwise diff --git a/turnstone/eval/nudges.py b/turnstone/eval/nudges.py index bff61d69..19bb7316 100644 --- a/turnstone/eval/nudges.py +++ b/turnstone/eval/nudges.py @@ -259,9 +259,8 @@ def _tasks_action_enum() -> frozenset[str]: # # Neither half is a literal here. The action VOCABULARY comes from the # tool's own schema (:func:`_tasks_action_enum`) and the READ half is -# ``ChatSession._TASKS_READ_ACTIONS``, production's own classifier — -# the one its parallel-batch guard and approval path rule on. Mutating -# is the remainder, so an action added to the schema counts as a +# ``_TASKS_READ_ACTIONS``, production's own preparer classifier. +# Mutating is the remainder, so an action added to the schema counts as a # mutation until production classifies it as a read: a new write can # never be silently dropped from the bookkeeping test, and the drift # that IS possible (a new read) is caught statically by @@ -824,7 +823,7 @@ def _seed_world(storage: Any, case: dict[str, Any]) -> None: saved, _was_update = save_structured_memory( row["name"], row["content"], - row.get("description"), + row["description"], row.get("type"), scope=row.get("scope", "global"), scope_id=row.get("scope_id", ""), @@ -1969,7 +1968,7 @@ def _check_world_is_seedable(case: dict[str, Any]) -> str | None: Recognized keys only (``memory`` / ``nodes``) — an unrecognized key is a silent no-op seed, which reads as "seeded" while leaving the hollow world the block exists to fill. Memory rows need non-empty - string ``name`` and ``content`` (the production upsert's own + string ``name``, ``description``, and ``content`` (the production upsert's own requirements, surfaced at authoring time); node rows need a non-empty string ``node_id``. """ @@ -1984,7 +1983,7 @@ def _check_world_is_seedable(case: dict[str, Any]) -> str | None: for i, row in enumerate(world.get("memory", ())): if not isinstance(row, dict): return f"world.memory[{i}] must be a dict" - for field in ("name", "content"): + for field in ("name", "content", "description"): v = row.get(field) if not isinstance(v, str) or not v.strip(): return f"world.memory[{i}].{field} must be a non-empty string" diff --git a/turnstone/eval/scenarios/nudges.py b/turnstone/eval/scenarios/nudges.py index aa7ab541..5489258e 100644 --- a/turnstone/eval/scenarios/nudges.py +++ b/turnstone/eval/scenarios/nudges.py @@ -95,6 +95,7 @@ NUDGE_CELLS: list[dict[str, Any]] = [ "memory": [ { "name": "acme-api-project", + "description": "Acme API repository and deployment context", "type": "reference", "content": ( "acme-api: FastAPI service. Repo layout: " @@ -105,6 +106,7 @@ NUDGE_CELLS: list[dict[str, Any]] = [ }, { "name": "auth-backend-migration-status", + "description": "Current authentication migration status", "content": ( "migrations/007_auth_backend.sql applied on the " "staging replica; auth service suite green " diff --git a/turnstone/sdk/server.py b/turnstone/sdk/server.py index 1d7e7a5c..b4075b78 100644 --- a/turnstone/sdk/server.py +++ b/turnstone/sdk/server.py @@ -525,19 +525,21 @@ class AsyncTurnstoneServer(_BaseClient): name: str, content: str, *, - description: str = "", + description: str, mem_type: str = "general", scope: str = "global", scope_id: str = "", ) -> MemoryInfo: + description = (description or "").strip() + if not description: + raise ValueError("memory description is required and must be non-empty") body: dict[str, Any] = { "name": name, "content": content, + "description": description, "type": mem_type, "scope": scope, } - if description: - body["description"] = description if scope_id: body["scope_id"] = scope_id return await self._request( @@ -868,7 +870,7 @@ class TurnstoneServer: name: str, content: str, *, - description: str = "", + description: str, mem_type: str = "general", scope: str = "global", scope_id: str = "", diff --git a/turnstone/server.py b/turnstone/server.py index cf81fd4d..6a9bd163 100644 --- a/turnstone/server.py +++ b/turnstone/server.py @@ -3495,31 +3495,157 @@ def _resolve_user_scope_id( return uid, None +def _resolve_workstream_memory_scope_id( + request: Request, + scope_id: str, +) -> tuple[str, JSONResponse | None]: + """Bind REST workstream-memory access to the authenticated owner.""" + from turnstone.core.storage._registry import get_storage + + resolved = scope_id.strip() + if not resolved: + return "", JSONResponse( + {"error": "scope_id is required for workstream scope"}, + status_code=400, + ) + owner = get_storage().get_workstream_owner(resolved) + if owner is None: + return "", JSONResponse({"error": "Workstream not found"}, status_code=404) + caller = _auth_user_id(request) + if "service" not in _auth_scopes(request) and (not caller or owner != caller): + return "", JSONResponse( + {"error": "Cannot access another user's workstream memories"}, + status_code=403, + ) + return resolved, None + + +def _resolve_rest_memory_scope( + request: Request, + scope: str, + scope_id: str, + *, + allow_empty: bool, +) -> tuple[str, str, JSONResponse | None]: + """Validate a public memory scope and bind its caller-controlled id.""" + normalized_scope = scope.strip().lower() + normalized_id = scope_id.strip() + if not normalized_scope and allow_empty: + err = _validate_scope_scope_id(normalized_scope, normalized_id) + return normalized_scope, normalized_id, err + if normalized_scope not in _VALID_MEMORY_SCOPES: + return ( + "", + "", + JSONResponse( + { + "error": ( + f"invalid scope: {normalized_scope}; " + f"must be one of {sorted(_VALID_MEMORY_SCOPES)}" + ) + }, + status_code=400, + ), + ) + if normalized_scope == "user": + normalized_id, err = _resolve_user_scope_id(request, normalized_id) + if err: + return "", "", err + elif normalized_scope == "workstream": + normalized_id, err = _resolve_workstream_memory_scope_id(request, normalized_id) + if err: + return "", "", err + err = _validate_scope_scope_id( + normalized_scope, + normalized_id, + require_scope_id=True, + ) + return normalized_scope, normalized_id, err + + +def _rest_visible_memory_scopes(request: Request) -> list[tuple[str, str]]: + """Default public read envelope: global plus the caller's user scope.""" + scopes = [("global", "")] + uid = _auth_user_id(request) + if uid: + scopes.append(("user", uid)) + return scopes + + +def _audit_rest_memory_mutation( + request: Request, + action: str, + row: dict[str, str], +) -> None: + """Record one authenticated REST/SDK memory mutation.""" + from turnstone.core.audit import record_audit + from turnstone.core.storage._registry import get_storage + + uid, ip = _audit_context(request) + record_audit( + get_storage(), + uid, + action, + "memory", + row["memory_id"], + { + "name": row["name"], + "scope": row["scope"], + "scope_id": row["scope_id"], + "type": row["type"], + "surface": "rest", + }, + ip, + ) + + async def list_memories(request: Request) -> JSONResponse: """GET /v1/api/memories — list memories with optional filters.""" - from turnstone.core.memory import list_structured_memories + from turnstone.core.storage._registry import get_storage - mem_type = request.query_params.get("type", "") + mem_type = request.query_params.get("type", "").strip().lower() scope = request.query_params.get("scope", "") scope_id = request.query_params.get("scope_id", "") + if mem_type and mem_type not in _VALID_MEMORY_TYPES: + return JSONResponse({"error": f"invalid type: {mem_type}"}, status_code=400) try: - limit = min(int(request.query_params.get("limit", "100")), 200) + limit = int(request.query_params.get("limit", "100")) except (ValueError, TypeError): return JSONResponse({"error": "limit must be an integer"}, status_code=400) - err = _validate_scope_scope_id(scope, scope_id) + if not 1 <= limit <= 200: + return JSONResponse({"error": "limit must be between 1 and 200"}, status_code=400) + scope, scope_id, err = _resolve_rest_memory_scope( + request, + scope, + scope_id, + allow_empty=True, + ) if err: return err - if scope == "user": - scope_id, err = _resolve_user_scope_id(request, scope_id) - if err: - return err - rows = list_structured_memories(mem_type=mem_type, scope=scope, scope_id=scope_id, limit=limit) + try: + storage = get_storage() + if scope: + rows = storage.list_structured_memories( + mem_type=mem_type, + scope=scope, + scope_id=scope_id, + limit=limit, + ) + else: + rows = storage.list_visible_structured_memories( + _rest_visible_memory_scopes(request), + mem_type=mem_type, + limit=limit, + ) + except Exception: + log.warning("memory.rest_list_failed", exc_info=True) + return JSONResponse({"error": "Memory storage unavailable"}, status_code=500) return JSONResponse({"memories": rows, "total": len(rows)}) async def save_memory(request: Request) -> JSONResponse: """POST /v1/api/memories — save (upsert) a structured memory.""" - from turnstone.core.memory import save_structured_memory + from turnstone.core.memory import save_structured_memory_strict from turnstone.core.web_helpers import read_json_or_400 body = await read_json_or_400(request) @@ -3536,12 +3662,15 @@ async def save_memory(request: Request) -> JSONResponse: {"error": f"content exceeds {_MAX_MEMORY_CONTENT} character limit"}, status_code=400, ) - # None (field omitted) means "leave unset": the upsert keeps the stored - # value on update and defaults on insert; an explicit value overwrites. raw_desc = body.get("description") - description = None if raw_desc is None else str(raw_desc) + description = "" if raw_desc is None else str(raw_desc).strip() + if not description: + return JSONResponse( + {"error": "description is required and must be non-empty"}, + status_code=400, + ) raw_type = body.get("type") - mem_type = None if raw_type is None else str(raw_type) + mem_type = None if raw_type is None else str(raw_type).strip().lower() scope = str(body.get("scope", "global")) scope_id = str(body.get("scope_id", "")) if mem_type is not None and mem_type not in _VALID_MEMORY_TYPES: @@ -3549,24 +3678,31 @@ async def save_memory(request: Request) -> JSONResponse: {"error": f"invalid type: {mem_type}; must be one of {sorted(_VALID_MEMORY_TYPES)}"}, status_code=400, ) - if scope not in _VALID_MEMORY_SCOPES: - return JSONResponse( - {"error": f"invalid scope: {scope}; must be one of {sorted(_VALID_MEMORY_SCOPES)}"}, - status_code=400, - ) - if scope == "user": - scope_id, err = _resolve_user_scope_id(request, scope_id) - if err: - return err - err = _validate_scope_scope_id(scope, scope_id, require_scope_id=True) + scope, scope_id, err = _resolve_rest_memory_scope( + request, + scope, + scope_id, + allow_empty=False, + ) if err: return err - # The upsert RETURNINGs the full saved row, so no follow-up read is needed. - row, was_update = save_structured_memory( - name, content, description=description, mem_type=mem_type, scope=scope, scope_id=scope_id - ) - if not row: + try: + row, was_update = save_structured_memory_strict( + name, + content, + description=description, + mem_type=mem_type, + scope=scope, + scope_id=scope_id, + ) + except Exception: + log.warning("memory.rest_save_failed", name=name, exc_info=True) return JSONResponse({"error": "Failed to save memory"}, status_code=500) + _audit_rest_memory_mutation( + request, + "memory.update" if was_update else "memory.save", + row, + ) return JSONResponse(row, status_code=200 if was_update else 201) @@ -3575,7 +3711,7 @@ async def search_memories(request: Request) -> JSONResponse: Uses POST for the request body but requires only read scope (non-mutating). """ - from turnstone.core.memory import search_structured_memories as search_fn + from turnstone.core.storage._registry import get_storage from turnstone.core.web_helpers import read_json_or_400 body = await read_json_or_400(request) @@ -3584,46 +3720,72 @@ async def search_memories(request: Request) -> JSONResponse: query = str(body.get("query", "")).strip() if not query: return JSONResponse({"error": "query is required"}, status_code=400) - mem_type = str(body.get("type", "")) + mem_type = str(body.get("type", "")).strip().lower() scope = str(body.get("scope", "")) scope_id = str(body.get("scope_id", "")) try: - limit = min(int(body.get("limit", 20)), 50) + limit = int(body.get("limit", 20)) except (ValueError, TypeError): return JSONResponse({"error": "limit must be an integer"}, status_code=400) - err = _validate_scope_scope_id(scope, scope_id) + if mem_type and mem_type not in _VALID_MEMORY_TYPES: + return JSONResponse({"error": f"invalid type: {mem_type}"}, status_code=400) + if not 1 <= limit <= 50: + return JSONResponse({"error": "limit must be between 1 and 50"}, status_code=400) + scope, scope_id, err = _resolve_rest_memory_scope( + request, + scope, + scope_id, + allow_empty=True, + ) if err: return err - if scope == "user": - scope_id, err = _resolve_user_scope_id(request, scope_id) - if err: - return err - rows = search_fn(query, mem_type=mem_type, scope=scope, scope_id=scope_id, limit=limit) + try: + storage = get_storage() + if scope: + rows = storage.search_structured_memories( + query, + mem_type=mem_type, + scope=scope, + scope_id=scope_id, + limit=limit, + ) + else: + rows = storage.search_visible_structured_memories( + query, + _rest_visible_memory_scopes(request), + mem_type=mem_type, + limit=limit, + ) + except Exception: + log.warning("memory.rest_search_failed", exc_info=True) + return JSONResponse({"error": "Memory storage unavailable"}, status_code=500) return JSONResponse({"memories": rows, "total": len(rows)}) async def delete_memory_endpoint(request: Request) -> JSONResponse: """DELETE /v1/api/memories/{name} — delete a memory by name and scope.""" - from turnstone.core.memory import delete_structured_memory, normalize_key + from turnstone.core.memory import delete_structured_memory_returning_strict, normalize_key name = normalize_key(request.path_params["name"]) scope = request.query_params.get("scope", "global") - if scope not in _VALID_MEMORY_SCOPES: - return JSONResponse( - {"error": f"invalid scope: {scope}; must be one of {sorted(_VALID_MEMORY_SCOPES)}"}, - status_code=400, - ) scope_id = request.query_params.get("scope_id", "") - if scope == "user": - scope_id, err = _resolve_user_scope_id(request, scope_id) - if err: - return err - err = _validate_scope_scope_id(scope, scope_id, require_scope_id=True) + scope, scope_id, err = _resolve_rest_memory_scope( + request, + scope, + scope_id, + allow_empty=False, + ) if err: return err - if delete_structured_memory(name, scope, scope_id): - return JSONResponse({"status": "ok", "name": name}) - return JSONResponse({"error": f"Memory '{name}' not found"}, status_code=404) + try: + deleted = delete_structured_memory_returning_strict(name, scope, scope_id) + except Exception: + log.warning("memory.rest_delete_failed", name=name, exc_info=True) + return JSONResponse({"error": "Failed to delete memory"}, status_code=500) + if deleted is None: + return JSONResponse({"error": f"Memory '{name}' not found"}, status_code=404) + _audit_rest_memory_mutation(request, "memory.delete", deleted) + return JSONResponse({"status": "ok", "name": name}) # --------------------------------------------------------------------------- diff --git a/turnstone/tools/memory.json b/turnstone/tools/memory.json index fb96ae5e..b74abfd2 100644 --- a/turnstone/tools/memory.json +++ b/turnstone/tools/memory.json @@ -1,6 +1,6 @@ { "name": "memory", - "description": "Persistent memory across sessions. Actions: 'save' stores a memory, 'get' retrieves full content by name, 'search' finds memories by query, 'delete' removes a memory, 'list' shows all memories. Use 'get' to read full content — search/list truncate previews to 200 chars. Memories have a type (user/general/feedback/reference) and a scope.", + "description": "Persistent memory across sessions. Actions: 'save' stores a memory and always requires a non-empty description, 'get' retrieves full content by name, 'search' finds memories by query, 'delete' removes a memory, 'list' shows all memories. Use 'get' to read full content — search/list truncate previews to 200 chars. Memories have a type (user/general/feedback/reference) and a scope. Save/get/delete inherit one target: an attached active project, otherwise the session-kind default. A read-only project permits get but makes inherited save/delete fail; they never fall back to another scope. Pass the displayed scope explicitly when following a search/list result from another scope.", "parameters": { "type": "object", "properties": { @@ -11,6 +11,7 @@ }, "name": { "type": "string", + "maxLength": 256, "description": "Memory identifier (required for 'save', 'get', and 'delete'). Short snake_case key." }, "content": { @@ -19,7 +20,8 @@ }, "description": { "type": "string", - "description": "Short description for relevance matching (recommended for 'save')." + "minLength": 1, + "description": "Required non-empty description for relevance matching on every 'save' (create or update)." }, "type": { "type": "string", @@ -29,7 +31,7 @@ "scope": { "type": "string", "enum": ["global", "workstream", "user", "coordinator", "project"], - "description": "Memory scope. 'global' = shared across everything; 'workstream' = private to this workstream; 'user' = follows the user across workstreams; 'project' = the shared bucket of the project this session is attached to (available only when attached, writable only with project write access). Default: the attached project when you can write it, otherwise 'global'." + "description": "Memory scope. 'global' = shared across everything; 'workstream' = private to this workstream; 'user' = follows the acting user across workstreams; 'project' = the shared bucket of the active project this session is attached to. Save/get/delete without scope inherit exactly one target: project when attached, otherwise the session-kind default. A read-only project permits get but rejects save/delete without falling back. Search/list without scope use all visible scopes. A valid explicit scope selects exactly that scope." }, "query": { "type": "string", @@ -46,20 +48,20 @@ "interactive": true, "kind_variants": { "interactive": { - "description": "Persistent memory across sessions. Actions: 'save' stores a memory, 'get' retrieves full content by name, 'search' finds memories by query, 'delete' removes a memory, 'list' shows all memories. Use 'get' to read full content — search/list truncate previews to 200 chars. Memories have a type (user/general/feedback/reference) and a scope: 'global' (shared everywhere), 'workstream' (this workstream only), 'user' (follows you across workstreams), and 'project' (the shared bucket of an attached project). When this workstream is attached to a project you can write, new memories default to the project; otherwise to 'global'.", + "description": "Persistent memory across sessions. Actions: 'save' stores a memory and always requires a non-empty description, 'get' retrieves full content by name, 'search' finds memories by query, 'delete' removes a memory, 'list' shows all memories. Use 'get' to read full content — search/list truncate previews to 200 chars. Memories have a type (user/general/feedback/reference) and a scope: 'global' (shared everywhere), 'workstream' (this workstream only), 'user' (follows the acting user across workstreams), and 'project' (the shared bucket of an attached project). Unscoped save/get/delete target project when attached, otherwise global. A read-only project permits get but rejects save/delete without falling back. Pass the displayed scope explicitly when following a search/list result from another scope.", "parameter_overrides": { "scope": { "enum": ["global", "workstream", "user", "project"], - "description": "Memory scope. 'global' = shared across everything; 'workstream' = private to this workstream; 'user' = follows the user across workstreams; 'project' = the shared bucket of the project this workstream is attached to (available only when attached, and writable only with project write access). Default: the attached project when you can write it, otherwise 'global'." + "description": "Memory scope. 'global' = shared across everything; 'workstream' = private to this workstream; 'user' = follows the acting user across workstreams; 'project' = the shared bucket of the active project this workstream is attached to. Save/get/delete without scope target project when attached, otherwise global. A read-only project permits get but rejects save/delete without falling back. Search/list without scope use all visible scopes. A valid explicit scope selects exactly that scope." } } }, "coordinator": { - "description": "Persistent orchestration memory shared by all of your user's coordinator sessions. Actions: 'save' stores a memory, 'get' retrieves full content by name, 'search' finds memories by query, 'delete' removes a memory, 'list' shows all memories. Use 'get' to read full content — search/list truncate previews to 200 chars. Memories have a type (user/general/feedback/reference). Coordinator memories survive across coordinator sessions — save orchestration knowledge worth keeping (recurring procedures, environment facts, lessons from past runs). They are NOT visible to child workstreams.", + "description": "Persistent orchestration memory for the acting user. Actions: 'save' stores a memory and always requires a non-empty description, 'get' retrieves full content by name, 'search' finds memories by query, 'delete' removes a memory, 'list' shows all memories. Use 'get' to read full content — search/list truncate previews to 200 chars. Coordinator memories survive across that user's coordinator sessions and are NOT visible to child workstreams. Unscoped save/get/delete target project when attached, otherwise coordinator. A read-only project permits get but rejects save/delete without falling back. Pass the displayed scope explicitly when following a search/list result from another scope.", "parameter_overrides": { "scope": { "enum": ["coordinator", "project"], - "description": "'coordinator' = the per-user orchestration namespace, durable across coordinator sessions; 'project' = the shared bucket of the project this coordinator is attached to (available only when attached, writable only with project write access). Default: the attached project when you can write it, otherwise 'coordinator'." + "description": "'coordinator' = the acting user's private orchestration namespace, durable across their coordinator sessions; 'project' = the shared bucket of the active project this coordinator is attached to. Save/get/delete without scope target project when attached, otherwise coordinator. A read-only project permits get but rejects save/delete without falling back. Search/list without scope use all visible scopes. A valid explicit scope selects exactly that scope." } } }