From 3a316551be867c29b400d93a8de0c02b3da61425 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 22 May 2026 22:37:55 -0400 Subject: [PATCH] =?UTF-8?q?feat(curator):=20authority=20routing=20?= =?UTF-8?q?=E2=80=94=20mutating=20tools=20queue=20for=20review=20(C3/5)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The interceptor that closes the loop on the curator review queue. With this commit, the curator can call update_note / update_milestone / update_project / update_profile / delete_note — those calls are caught by execute_tool's authority='curator' path, snapshotted, and written to pending_curator_actions for the user to approve or reject later. Additive tools still run immediately. services/tools/_registry.py: - New _CURATOR_MUTATING_TOOLS frozenset: {update_note, update_milestone, update_project, update_profile, delete_note}. update_event / delete_event intentionally excluded — calendar events should always be explicit user intent. - execute_tool gains a keyword-only parameter, defaulting to 'user'. Default behaviour is unchanged; existing callers keep working without changes. - When authority='curator' AND tool is in _CURATOR_MUTATING_TOOLS, _queue_for_review captures a snapshot of the target via a per-tool helper and writes a pending action. Returns {success:true, pending:true, action_id:N, message:...} so the curator sees the call as 'completed' for its bookkeeping. - Per-tool snapshot helpers: _snapshot_note (covers update_note + delete_note — uses the same fuzzy match update_note_tool uses, so the snapshot reflects what'd actually be mutated), _snapshot_milestone, _snapshot_project, _snapshot_profile. Snapshot capture is best-effort — failure logs but still queues with empty snapshot so a curator proposal never silently drops. services/curator.py: - Allowlist now includes the five mutating tools. They're safe to expose because execute_tool intercepts them; the curator can propose without being able to actually mutate. - The execute_tool call now passes authority='curator'. - System prompt explicitly authorizes the proposal pattern: 'update_note', 'update_milestone', 'update_project', 'update_profile', 'delete_note' are described as proposing tools that wait for user approval. 'Don't try to update or delete anything' line removed. services/pending_actions.py: - approve() now passes authority='user' on the replay so the curator interceptor doesn't re-route the replay back into pending and create an infinite loop. What's left in the queue: - C4: API routes (list/approve/reject endpoints). - C5: Frontend Needs Review panel. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/fabledassistant/services/curator.py | 36 +++- .../services/pending_actions.py | 10 +- .../services/tools/_registry.py | 203 +++++++++++++++++- 3 files changed, 236 insertions(+), 13 deletions(-) diff --git a/src/fabledassistant/services/curator.py b/src/fabledassistant/services/curator.py index d68e940..f6b4a14 100644 --- a/src/fabledassistant/services/curator.py +++ b/src/fabledassistant/services/curator.py @@ -53,7 +53,7 @@ _MAX_TOOL_ROUNDS = 4 # resolution before it can safely link names → ids on the additive # calls. _CURATOR_ALLOWED_TOOLS: frozenset[str] = frozenset({ - # Additive — primary curator work + # Additive — primary curator work; run directly. "record_moment", "create_note", # also creates tasks (status='todo') "log_work", # appends to a task's work-log timeline @@ -61,7 +61,15 @@ _CURATOR_ALLOWED_TOOLS: frozenset[str] = frozenset({ "save_place", "create_project", "create_milestone", - # Read-only — supporting lookups for entity resolution + # Mutating — intercepted by execute_tool's authority="curator" path + # and routed to the pending-actions queue for user review. + # See services/tools/_registry.py:_CURATOR_MUTATING_TOOLS. + "update_note", # covers tasks + notes; routed to pending + "update_milestone", # routed to pending + "update_project", # routed to pending + "update_profile", # routed to pending + "delete_note", # routed to pending + # Read-only — supporting lookups for entity resolution and cross-ref. "search_notes", "search_projects", "search_journal", @@ -88,15 +96,24 @@ WHAT TO CAPTURE (from `User:` lines only): - New tasks or todos the user wants to track ("I need to call mom tomorrow", "remind me to renew the domain") - Knowledge worth saving as a note ("the dhcp issue at Bedford was caused by stale leases") -TOOL USE — additive only. -You have these tools available; nothing else exists. The architecture intentionally excludes update and delete operations from your reach — a confidently-wrong update is worse than a duplicate create that the user can prune. +TOOL USE — two categories. +**Additive (run immediately).** These tools persist to the user's data on the spot. Use them freely when warranted: - `record_moment` — for journal beats. One tool call per beat; do not collapse multiple beats. Write content in the user's voice (first-person or imperative); NEVER "the user mentioned…" framing. - `create_note` — create a task (set status='todo' plus due_date/priority if mentioned) OR a knowledge note (omit status). Use this when the user states a new commitment with a clear scope, or shares a chunk of reusable knowledge. - `log_work` — append a progress entry to an existing task. Use this when the user describes work they did on a task that already exists. ALWAYS call search_notes first to confirm the task exists by title or keyword; if no match, prefer create_note with status='todo' instead, or skip. - `save_person` / `save_place` — create a new entry only when the user mentions someone or somewhere you have no prior reference for. Otherwise skip — better to omit a link on a moment than to invent the wrong entity. - `create_project` / `create_milestone` — only when the user is clearly starting a NEW project or milestone they intend to track. Don't infer projects from passing mentions. +**Proposing (queued for user review).** These tools mutate existing data; calls are intercepted and shown to the user for approval in the Needs Review panel. Use them when the user's intent is clear, but expect each one to wait for explicit user OK: +- `update_note` — mark a task done ("I finished the network restage" → update_note query='network restage', status='done'), change priority, edit a note's body, etc. ALWAYS search_notes first to verify the target exists. +- `update_milestone` — close a milestone the user explicitly says is finished, or rename one. +- `update_project` — adjust a project's status/description when the user explicitly states a change. +- `update_profile` — record a profile fact the user explicitly stated about themselves ("I'm a backend engineer now"). Don't infer profile updates from passing context. +- `delete_note` — propose removing a note or task the user explicitly says they no longer need. + +For proposing calls, the user sees a "before / proposed" diff and approves or rejects. You don't need to confirm with the user yourself — proposing IS the asking. If the proposal returns `{success: true, pending: true}`, that means it's queued; carry on with other captures. + ENTITY LINKING (on record_moment): - Use the *_names parameters (person_names, place_names, task_titles, note_titles). The server resolves names → ids. - Before passing task_titles or note_titles, call search_notes to confirm the title exists. Don't invent titles. @@ -115,8 +132,8 @@ If nothing relevant turns up, don't force a reference. The cross-ref is helpful WHAT NOT TO DO: - Don't capture content from `Assistant:` lines. -- Don't try to update or delete anything (you have no tools for it). - Don't fire tool calls for purely meta-conversational fragments ("ok", "thanks", "got it"). +- Don't try to call create_event, delete_event, or anything calendar-related (curator scope excludes calendar entirely). After the tool calls, you may emit one short summary sentence (≤ 20 words) describing what you captured. The summary is shown back to the chat model in subsequent turns so it stays aware of recent topics; it is NOT shown to the user directly. Examples: - "Captured network restage progress and a coffee mention with Sarah." @@ -353,8 +370,15 @@ async def run_curator_for_conversation( args = {} call = CuratorToolCall(name=name, arguments=args) try: + # authority="curator" routes mutating tools + # (_CURATOR_MUTATING_TOOLS in tools/_registry.py) to + # the pending-actions queue instead of executing them. + # Additive tools run normally; the curator can never + # silently mutate user data. tool_result = await execute_tool( - user_id, name, args, conv_id=conv_id, + user_id, name, args, + conv_id=conv_id, + authority="curator", ) call.result = tool_result call.status = ( diff --git a/src/fabledassistant/services/pending_actions.py b/src/fabledassistant/services/pending_actions.py index 04da735..98f1349 100644 --- a/src/fabledassistant/services/pending_actions.py +++ b/src/fabledassistant/services/pending_actions.py @@ -131,17 +131,15 @@ async def approve(action_id: int, user_id: int) -> dict[str, Any]: from fabledassistant.services.tools import execute_tool try: - # NOTE (C2 → C3 hand-off): execute_tool gains an `authority` kwarg - # in commit C3 that the curator interceptor checks. Approval replay - # MUST pass authority="user" once that lands, otherwise the - # interceptor re-routes the replay back into pending and loops. - # Until C3 lands, execute_tool is identity (no interceptor exists - # yet), so this call runs cleanly. + # authority="user" bypasses the curator interceptor — required so + # the replay actually executes the mutating tool instead of + # creating another pending row (which would infinite-loop). result = await execute_tool( user_id, row.action_type, row.payload, conv_id=row.conv_id, + authority="user", ) except Exception as e: logger.exception( diff --git a/src/fabledassistant/services/tools/_registry.py b/src/fabledassistant/services/tools/_registry.py index 1c4531e..c17e63d 100644 --- a/src/fabledassistant/services/tools/_registry.py +++ b/src/fabledassistant/services/tools/_registry.py @@ -115,17 +115,57 @@ async def get_tools_for_user( return tools + +# Mutating tools that the curator must NOT execute directly. When +# `execute_tool` is called with `authority="curator"`, calls to these +# tools are intercepted and routed to the pending-actions queue for +# user review (services/pending_actions.create_pending). The user +# approves or rejects each from the journal's Needs Review panel. +# +# Why this set in particular: confidently-wrong updates / deletes +# corrupt user data in ways that creates don't. Bad creates are +# deletable; bad updates aren't. The curator stays additive by +# default; mutating ops only land via explicit user approval. +_CURATOR_MUTATING_TOOLS: frozenset[str] = frozenset({ + "update_note", # covers tasks AND notes (same Note model) + "update_milestone", + "update_project", + "update_profile", + "delete_note", + # update_event / delete_event intentionally excluded — calendar + # events should always be explicit user intent, never curator + # inference. The curator simply doesn't get to touch the calendar. +}) + + async def execute_tool( user_id: int, tool_name: str, arguments: dict, conv_id: int | None = None, workspace_project_id: int | None = None, + *, + authority: str = "user", ) -> dict: - """Execute a tool call and return the result.""" + """Execute a tool call and return the result. + + `authority` controls the curator-interceptor behavior: + - "user" (default): normal execution. Used by the chat route, journal + curator-trigger route, MCP, and the pending-actions replay path. + - "curator": mutating tools listed in `_CURATOR_MUTATING_TOOLS` are + intercepted and queued via services.pending_actions.create_pending. + Non-mutating tools (create_*, search_*, list_*, record_moment, + log_work, etc.) execute as normal. + + The default "user" preserves all existing callers without changes. + """ td = _REGISTRY.get(tool_name) if td is None: return {"success": False, "error": f"Unknown tool: {tool_name}"} + + if authority == "curator" and tool_name in _CURATOR_MUTATING_TOOLS: + return await _queue_for_review(user_id, conv_id, tool_name, arguments) + try: return await td.handler( user_id=user_id, @@ -136,3 +176,164 @@ async def execute_tool( except Exception as e: logger.exception("Tool execution failed: %s", tool_name) return {"success": False, "error": str(e)} + + +async def _queue_for_review( + user_id: int, conv_id: int | None, tool_name: str, arguments: dict, +) -> dict: + """Capture a snapshot of the target entity and write a pending action. + + The interceptor never raises — if snapshot capture fails for any + reason, we still queue the action with an empty snapshot rather than + silently dropping the curator's proposal. The user can still see and + approve/reject it; the diff display just won't show a real "before." + """ + from fabledassistant.services.pending_actions import create_pending + + target_type: str | None = None + target_id: int | None = None + target_label: str | None = None + snapshot: dict = {} + try: + helper = _SNAPSHOT_HELPERS.get(tool_name) + if helper is not None: + target_type, target_id, target_label, snapshot = await helper( + user_id, arguments, + ) + except Exception: + logger.exception( + "Snapshot capture failed for curator-proposed %s (user=%d); " + "queuing with empty snapshot", + tool_name, user_id, + ) + + row = await create_pending( + user_id=user_id, + conv_id=conv_id, + action_type=tool_name, + target_type=target_type, + target_id=target_id, + target_label=target_label, + payload=dict(arguments or {}), + current_snapshot=snapshot or {}, + ) + return { + "success": True, + "pending": True, + "action_id": row.id, + "message": ( + f"Proposed {tool_name} queued for review (action #{row.id}). " + "User will approve or reject from the journal." + ), + } + + +# Snapshot helpers — each fetches the current state of the target entity +# for the diff display in the Needs Review panel. Lazy-imported because +# the registry module is otherwise dep-free of model/service code. +async def _snapshot_note( + user_id: int, arguments: dict, +) -> tuple[str | None, int | None, str | None, dict]: + """Resolve update_note / delete_note's target — same fuzzy match + update_note_tool uses, so the snapshot reflects what'd actually be + mutated on approval. + """ + from fabledassistant.services.notes import get_note_by_title, list_notes + + query = str(arguments.get("query") or "").strip() + if not query: + return None, None, None, {} + + note = await get_note_by_title(user_id, query) + if note is None: + notes, _ = await list_notes(user_id=user_id, q=query, limit=5) + note = notes[0] if notes else None + if note is None: + return "note", None, query, {} + + snapshot = { + "id": note.id, + "title": note.title, + "is_task": bool(note.is_task), + "status": note.status, + "priority": note.priority, + "due_date": str(note.due_date) if note.due_date else None, + "tags": list(note.tags or []), + "body": (note.body or "")[:500], # cap — diffs only show short context + "description": (note.description or "")[:500] if hasattr(note, "description") else None, + "project_id": note.project_id, + "milestone_id": note.milestone_id, + } + target_type = "task" if note.is_task else "note" + return target_type, note.id, note.title, snapshot + + +async def _snapshot_milestone( + user_id: int, arguments: dict, +) -> tuple[str | None, int | None, str | None, dict]: + from fabledassistant.services.milestones import find_milestone_by_title + + name = str(arguments.get("milestone") or arguments.get("title") or "").strip() + if not name: + return None, None, None, {} + ms = await find_milestone_by_title(user_id, name) + if ms is None: + return "milestone", None, name, {} + snapshot = { + "id": ms.id, + "title": ms.title, + "description": (ms.description or "")[:500], + "status": ms.status, + "project_id": ms.project_id, + } + return "milestone", ms.id, ms.title, snapshot + + +async def _snapshot_project( + user_id: int, arguments: dict, +) -> tuple[str | None, int | None, str | None, dict]: + from fabledassistant.services.tools._helpers import resolve_project + + name = str(arguments.get("project") or arguments.get("title") or "").strip() + if not name: + return None, None, None, {} + proj = await resolve_project(user_id, name) + if proj is None: + return "project", None, name, {} + snapshot = { + "id": proj.id, + "title": proj.title, + "description": (proj.description or "")[:500], + "goal": (proj.goal or "")[:500], + "status": proj.status, + } + return "project", proj.id, proj.title, snapshot + + +async def _snapshot_profile( + user_id: int, _arguments: dict, +) -> tuple[str | None, int | None, str | None, dict]: + from fabledassistant.services.user_profile import get_profile + + profile = await get_profile(user_id) + if profile is None: + return "profile", user_id, None, {} + snapshot = { + "display_name": profile.display_name, + "job_title": profile.job_title, + "industry": profile.industry, + "expertise_level": profile.expertise_level, + "response_style": profile.response_style, + "tone": profile.tone, + "interests": list(profile.interests or []), + } + return "profile", user_id, profile.display_name or "your profile", snapshot + + +_SNAPSHOT_HELPERS = { + "update_note": _snapshot_note, + "delete_note": _snapshot_note, + "update_milestone": _snapshot_milestone, + "update_project": _snapshot_project, + "update_profile": _snapshot_profile, +}