diff --git a/interface/web/src/Chatbot.jsx b/interface/web/src/Chatbot.jsx index 16b8b50..3e82028 100644 --- a/interface/web/src/Chatbot.jsx +++ b/interface/web/src/Chatbot.jsx @@ -2,6 +2,7 @@ import { useState, useRef, useEffect } from "react"; import { API_BASE } from "./config"; import { Markdown } from "./Markdown"; +import { diffLines, DIFF_LINE_COLOR, keyValueDiffLines } from "./preview/diff-view.js"; export function Chatbot({ visible = true, conversationId, setConversationId, onConversationChanged }) { const [messages, setMessages] = useState([]); @@ -728,13 +729,16 @@ export function Chatbot({ visible = true, conversationId, setConversationId, onC
⚠️ The assistant wants to run: - {" "} - {pendingApproval.map((a, i) => ( - - {a.name}({a.arguments ? Object.values(a.arguments).join(", ") : ""}) - - ))}
+ {pendingApproval.map((a, i) => + a.preview + ? + : ( + + {a.name}({a.arguments ? Object.values(a.arguments).join(", ") : ""}) + + ) + )}
); +} + +// One self-edit tool's approval preview: a real, server-computed diff instead +// of the flat "name(arg, arg)" one-liner used for every other action tool. +// This is what makes "always ask first" mean actual informed consent for +// edit_source/edit_playbook/edit_settings — the human reviews what will +// actually change, not a description of it. See synapse/self_edit.py's +// preview_* functions, which compute exactly what's rendered here. +function ActionPreview({ action }) { + const { name, preview } = action; + const banner = (text) => ( +
+ ⚠ {text} +
+ ); + const diffBox = (lines) => ( +
+      {lines.length === 0 && (no changes)}
+      {lines.map((l, i) => (
+        
+ {l.text || " "} +
+ ))} +
+ ); + + if (!preview.ok) { + return ( +
+
+ {name} + {" — this will fail:"} +
+
{preview.error}
+
+ ); + } + + if (name === "edit_source") { + return ( +
+
+ edit_source + {" "}{preview.path}{preview.is_new_file ? " (new file)" : ""} +
+ {diffBox(diffLines(preview.diff))} +
+ ); + } + + if (name === "edit_playbook") { + const keys = ["title", "goal", "instructions", "tags", "tools", "model"]; + const lines = keyValueDiffLines(preview.before, preview.after, keys); + return ( +
+
+ edit_playbook + {" "}{preview.is_new ? "(new playbook)" : preview.after?.title} +
+ {preview.becomes_main_playbook && banner("this will become the active system prompt")} + {diffBox(lines)} +
+ ); + } + + if (name === "edit_settings") { + const before = {}, after = {}; + for (const [k, v] of Object.entries(preview.applied || {})) { + before[k] = v.before; + after[k] = v.after; + } + const lines = keyValueDiffLines(before, after, Object.keys(preview.applied || {})); + return ( +
+
+ edit_settings +
+ {preview.policy_change && banner("this changes the tool-approval policy itself")} + {preview.system_prompt_change && banner("this changes the fallback system prompt")} + {diffBox(lines)} + {preview.ignored_unknown && preview.ignored_unknown.length > 0 && ( +
+ ignored (not a real setting): {preview.ignored_unknown.join(", ")} +
+ )} +
+ ); + } + + return null; } \ No newline at end of file diff --git a/interface/web/src/Markdown.jsx b/interface/web/src/Markdown.jsx index e12fb3a..0cf95df 100644 --- a/interface/web/src/Markdown.jsx +++ b/interface/web/src/Markdown.jsx @@ -9,6 +9,11 @@ import { PREVIEW_LANGS, RENDERABLE_LANGS } from "./preview/languages.js"; // The execution track's display half: a ```nexus-run fence is a run result // (source + captured output), not a program to execute here. See run-langs.js. import { RUN_FENCE_LANG, parseRunResult } from "./preview/run-langs.js"; +// Self-modification's display half: a ```nexus-edit fence is the result of an +// already-applied, already-approved edit_source/edit_playbook/edit_settings +// call. See self-edit-langs.js. +import { EDIT_FENCE_LANG, parseEditResult } from "./preview/self-edit-langs.js"; +import { diffLines, DIFF_LINE_COLOR, keyValueDiffLines } from "./preview/diff-view.js"; // Parse content into an array of {type, value, lang, streaming} blocks. // Handles: @@ -72,6 +77,10 @@ export function Markdown({ content }) { const run = block.streaming ? null : parseRunResult(block.value); if (run) return ; } + if (lang === EDIT_FENCE_LANG) { + const edit = block.streaming ? null : parseEditResult(block.value); + if (edit) return ; + } return RENDERABLE_LANGS.has(lang) ? : ; @@ -252,6 +261,114 @@ function RunBlock({ run }) { ); } +// A finished self-edit: the diff/change that was actually applied, plus (for +// edit_source) the git commit it landed as. Like RunBlock, nothing executes +// here — the edit already happened on the backend under approval, and by the +// time this renders it's history. Styled the same way as CodeBlock/RunBlock +// for visual consistency, with diff lines colored via the shared +// diff-view.js vocabulary rather than a syntax-highlighting library. +function EditBlock({ edit }) { + const [copied, setCopied] = useState(false); + + const copyText = + edit.kind === "source" ? edit.diff + : edit.kind === "playbook" ? edit.instructions + : JSON.stringify(edit.applied, null, 2); + + const copy = () => { + navigator.clipboard.writeText((copyText || "").trimEnd()).then(() => { + setCopied(true); + setTimeout(() => setCopied(false), 1500); + }); + }; + + const title = + edit.kind === "source" ? `edited ${edit.path}` + : edit.kind === "playbook" ? `playbook: ${edit.title || edit.id}` + : "settings changed"; + + const lines = + edit.kind === "source" ? diffLines(edit.diff) + : edit.kind === "settings" + ? keyValueDiffLines( + Object.fromEntries(Object.entries(edit.applied).map(([k, v]) => [k, v.before])), + Object.fromEntries(Object.entries(edit.applied).map(([k, v]) => [k, v.after])), + Object.keys(edit.applied), + ) + : []; + + return ( +
+
+ + {title} + +
+ {edit.commit && ( + + commit {edit.commit} + + )} + +
+
+ {edit.kind === "playbook" && edit.isMainPlaybook && ( +
+ ⚠ this is now the active system prompt +
+ )} + {(edit.kind === "settings") && (edit.policyChange || edit.systemPromptChange) && ( +
+ ⚠ {edit.policyChange && "changed the tool-approval policy"} + {edit.policyChange && edit.systemPromptChange && " and "} + {edit.systemPromptChange && "changed the fallback system prompt"} +
+ )} + {edit.kind === "playbook" ? ( +
+          {(edit.instructions || "").trimEnd()}
+        
+ ) : ( +
+          {lines.map((l, i) => (
+            
+ {l.text || " "} +
+ ))} +
+ )} +
+ ); +} + // Content-Security-Policy for the rendered preview. Together with the iframe's // `sandbox` attribute below, this is the entire trust boundary for model- // authored HTML/SVG, so it stays conservative rather than convenient: diff --git a/interface/web/src/preview/diff-view.js b/interface/web/src/preview/diff-view.js new file mode 100644 index 0000000..bf53747 --- /dev/null +++ b/interface/web/src/preview/diff-view.js @@ -0,0 +1,56 @@ +/* + * diff-view.js — shared plain-text diff-line classification, no dependency. + * + * There is no diff/syntax-highlighting library in this project (see + * package.json), so a unified-diff string is colored by hand: split into + * lines, tag each by its leading character. Used by both the self-edit + * approval banner (Chatbot.jsx, reviewing a change BEFORE it's applied) and + * the nexus-edit result block (Markdown.jsx, showing one AFTER) — one small + * shared vocabulary so both read the same way. + */ + +/** + * Split unified-diff text (from difflib.unified_diff on the backend) into + * {text, kind} lines. kind is one of "add" | "remove" | "hunk" | "file" | + * "context". The "+++"/"---" file markers and "@@" hunk headers get their own + * kind so they aren't colored as if they were real added/removed lines (a + * bare "+++" line is not "code that was added"). + */ +export function diffLines(text) { + return (text || "").replace(/\n$/, "").split("\n").map((line) => { + if (line.startsWith("+++") || line.startsWith("---")) return { text: line, kind: "file" }; + if (line.startsWith("@@")) return { text: line, kind: "hunk" }; + if (line.startsWith("+")) return { text: line, kind: "add" }; + if (line.startsWith("-")) return { text: line, kind: "remove" }; + return { text: line, kind: "context" }; + }); +} + +export const DIFF_LINE_COLOR = { + add: "#8aff8a", + remove: "#ff8a80", + hunk: "#7a92a8", + file: "#7a92a8", + context: "#ccc", +}; + +/** + * before/after key-value pairs (playbook fields, settings keys) rendered + * through the same add/remove vocabulary as a real diff, so a settings or + * playbook change reads the same way a source diff does: one line removed, + * one line added, per changed key. Unchanged keys are omitted entirely — + * this is "what's different," not a full dump. + */ +export function keyValueDiffLines(before, after, keys) { + const lines = []; + for (const key of keys) { + const b = before ? before[key] : undefined; + const a = after ? after[key] : undefined; + const bStr = JSON.stringify(b); + const aStr = JSON.stringify(a); + if (bStr === aStr) continue; + if (b !== undefined) lines.push({ text: `- ${key}: ${bStr}`, kind: "remove" }); + if (a !== undefined) lines.push({ text: `+ ${key}: ${aStr}`, kind: "add" }); + } + return lines; +} diff --git a/interface/web/src/preview/diff-view.test.js b/interface/web/src/preview/diff-view.test.js new file mode 100644 index 0000000..e33921e --- /dev/null +++ b/interface/web/src/preview/diff-view.test.js @@ -0,0 +1,45 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; + +import { diffLines, keyValueDiffLines } from "./diff-view.js"; + +test("diffLines tags add/remove/hunk/file/context lines", () => { + const text = "--- a/x.py\n+++ b/x.py\n@@ -1,1 +1,1 @@\n-old\n+new\n unchanged\n"; + const lines = diffLines(text); + assert.equal(lines[0].kind, "file"); + assert.equal(lines[1].kind, "file"); + assert.equal(lines[2].kind, "hunk"); + assert.equal(lines[3].kind, "remove"); + assert.equal(lines[4].kind, "add"); + assert.equal(lines[5].kind, "context"); +}); + +test("diffLines drops exactly one trailing newline, not trailing blank lines", () => { + const lines = diffLines("a\nb\n"); + assert.equal(lines.length, 2); + assert.equal(lines[1].text, "b"); +}); + +test("diffLines on empty text returns one empty context line, not a crash", () => { + const lines = diffLines(""); + assert.equal(lines.length, 1); + assert.equal(lines[0].text, ""); +}); + +test("keyValueDiffLines only emits changed keys", () => { + const lines = keyValueDiffLines({ a: 1, b: 2 }, { a: 1, b: 3 }, ["a", "b"]); + assert.equal(lines.length, 2); + assert.match(lines[0].text, /^- b: 2$/); + assert.match(lines[1].text, /^\+ b: 3$/); +}); + +test("keyValueDiffLines handles a null before (brand-new object)", () => { + const lines = keyValueDiffLines(null, { title: "New" }, ["title"]); + assert.equal(lines.length, 1); + assert.equal(lines[0].kind, "add"); + assert.match(lines[0].text, /title: "New"/); +}); + +test("keyValueDiffLines emits nothing when nothing changed", () => { + assert.deepEqual(keyValueDiffLines({ a: 1 }, { a: 1 }, ["a"]), []); +}); diff --git a/interface/web/src/preview/self-edit-langs.js b/interface/web/src/preview/self-edit-langs.js new file mode 100644 index 0000000..accde87 --- /dev/null +++ b/interface/web/src/preview/self-edit-langs.js @@ -0,0 +1,65 @@ +/* + * self-edit-langs.js — display half of the self-modification tools + * (edit_source / edit_playbook / edit_settings), the counterpart to + * run-langs.js. Nothing executes or applies here: by the time this parses a + * fence, the write (if any) already happened on the backend under the user's + * per-call approval, in synapse/self_edit.py. This side only labels and lays + * out what was returned. + * + * One fence tag, three payload shapes (source / playbook / settings), because + * all three go through the same approval round-trip and the same "carry what + * happened in one block" idea as run_snippet's nexus-run fence. + */ + +export const EDIT_FENCE_LANG = "nexus-edit"; + +/** + * Parse a nexus-edit fence body. Returns null for anything malformed or of an + * unrecognized kind — the caller falls back to showing the block as plain + * code, the same honest-fallback behavior as parseRunResult. + */ +export function parseEditResult(text) { + let data; + try { + data = JSON.parse(text); + } catch { + return null; + } + if (!data || typeof data !== "object") return null; + + if (data.kind === "source") { + if (typeof data.path !== "string" || typeof data.diff !== "string") return null; + return { + kind: "source", + path: data.path, + diff: data.diff, + commit: typeof data.commit === "string" ? data.commit : null, + instruction: typeof data.instruction === "string" ? data.instruction : "", + }; + } + + if (data.kind === "playbook") { + if (typeof data.id !== "string") return null; + return { + kind: "playbook", + id: data.id, + title: typeof data.title === "string" ? data.title : "", + goal: typeof data.goal === "string" ? data.goal : "", + instructions: typeof data.instructions === "string" ? data.instructions : "", + isMainPlaybook: !!data.is_main_playbook, + }; + } + + if (data.kind === "settings") { + if (!data.applied || typeof data.applied !== "object") return null; + return { + kind: "settings", + applied: data.applied, + ignoredUnknown: Array.isArray(data.ignored_unknown) ? data.ignored_unknown : [], + policyChange: !!data.policy_change, + systemPromptChange: !!data.system_prompt_change, + }; + } + + return null; +} diff --git a/interface/web/src/preview/self-edit-langs.test.js b/interface/web/src/preview/self-edit-langs.test.js new file mode 100644 index 0000000..4f3a14c --- /dev/null +++ b/interface/web/src/preview/self-edit-langs.test.js @@ -0,0 +1,80 @@ +/* + * The self-edit result envelope: what parseEditResult will and won't accept. + * + * Same reasoning as run-langs.test.js: this parses text a model chose to + * paste, so the malformed cases matter as much as the well-formed ones. A bad + * envelope must return null so the caller falls back to plain code, not a + * half-built object that renders a change that never happened. + */ +import { test } from "node:test"; +import assert from "node:assert/strict"; + +import { EDIT_FENCE_LANG, parseEditResult } from "./self-edit-langs.js"; + +test("the fence tag is the one the backend emits", () => { + assert.equal(EDIT_FENCE_LANG, "nexus-edit"); +}); + +test("a well-formed source envelope parses into display fields", () => { + const edit = parseEditResult(JSON.stringify({ + kind: "source", ok: true, path: "synapse/tools.py", diff: "--- a\n+++ b\n", + commit: "abc1234", instruction: "restart needed", + })); + assert.equal(edit.kind, "source"); + assert.equal(edit.path, "synapse/tools.py"); + assert.equal(edit.commit, "abc1234"); + assert.equal(edit.instruction, "restart needed"); +}); + +test("a source envelope with no commit reads as null, not undefined", () => { + const edit = parseEditResult(JSON.stringify({ + kind: "source", path: "x.py", diff: "", commit: null, + })); + assert.equal(edit.commit, null); +}); + +test("a source envelope missing path or diff is rejected", () => { + assert.equal(parseEditResult(JSON.stringify({ kind: "source", diff: "x" })), null); + assert.equal(parseEditResult(JSON.stringify({ kind: "source", path: "x.py" })), null); +}); + +test("a well-formed playbook envelope parses into display fields", () => { + const edit = parseEditResult(JSON.stringify({ + kind: "playbook", id: "p1", title: "T", goal: "G", instructions: "I", + is_main_playbook: true, + })); + assert.equal(edit.kind, "playbook"); + assert.equal(edit.id, "p1"); + assert.equal(edit.isMainPlaybook, true); +}); + +test("a playbook envelope missing id is rejected", () => { + assert.equal(parseEditResult(JSON.stringify({ kind: "playbook", title: "T" })), null); +}); + +test("a well-formed settings envelope parses into display fields", () => { + const edit = parseEditResult(JSON.stringify({ + kind: "settings", + applied: { model: { before: "a", after: "b" } }, + ignored_unknown: ["nope"], + policy_change: true, + system_prompt_change: false, + })); + assert.equal(edit.kind, "settings"); + assert.deepEqual(edit.applied, { model: { before: "a", after: "b" } }); + assert.deepEqual(edit.ignoredUnknown, ["nope"]); + assert.equal(edit.policyChange, true); + assert.equal(edit.systemPromptChange, false); +}); + +test("a settings envelope missing applied is rejected", () => { + assert.equal(parseEditResult(JSON.stringify({ kind: "settings" })), null); +}); + +test("malformed or foreign envelopes are rejected", () => { + assert.equal(parseEditResult("not json"), null); + assert.equal(parseEditResult("null"), null); + assert.equal(parseEditResult("[1,2,3]"), null); + assert.equal(parseEditResult(JSON.stringify({ kind: "unknown_kind" })), null); + assert.equal(parseEditResult(JSON.stringify({ path: "x.py", diff: "" })), null); +}); diff --git a/synapse/chat.py b/synapse/chat.py index cc200e0..dccad72 100644 --- a/synapse/chat.py +++ b/synapse/chat.py @@ -10,6 +10,7 @@ from typing import AsyncGenerator, Dict, List, Optional, Any from .nexus_config import settings, DEFAULT_CHAT_MODEL from .ollama_manager import get_ollama_manager from . import tools as _tools +from . import self_edit # Cap on tool-call round-trips before the final answer — stops a confused small # model from looping forever. @@ -349,9 +350,17 @@ async def _run_tool_loop(manager, messages, model, tool_schemas, temperature, nu messages.append(msg) # If any action tool needs per-call approval, pause and wait for the user. + # edit_playbook/edit_settings/edit_source always require it, regardless of + # `policy` — a global "allow" set for convenience on an unrelated tool + # (web_search, say) must never silently also unlock unattended + # self-modification. See self_edit.ALWAYS_ASK_TOOLS. decisions = None action_calls = [c for c in calls if _tools.is_action(c.get("function", {}).get("name", ""))] - if policy == "ask" and action_calls: + needs_approval = policy == "ask" or any( + c.get("function", {}).get("name", "") in self_edit.ALWAYS_ASK_TOOLS + for c in action_calls + ) + if needs_approval and action_calls: event = asyncio.Event() # Single-use capability token, delivered only to the client that owns # this stream. /chat/approve requires it, so knowing the (guessable, @@ -359,13 +368,21 @@ async def _run_tool_loop(manager, messages, model, tool_schemas, temperature, nu # else's pending action. token = secrets.token_urlsafe(32) pending_approvals[conversation_id] = {"event": event, "decisions": {}, "token": token} + + def _action_entry(c): + name = c.get("function", {}).get("name", "") + args = c.get("function", {}).get("arguments") + entry = {"name": name, "arguments": args} + if name in self_edit.PREVIEWABLE: + try: + entry["preview"] = self_edit.preview_for(name, args or {}) + except Exception as e: + entry["preview"] = {"ok": False, "error": f"preview failed: {e}"} + return entry + yield "__approve__" + _json.dumps({ "token": token, - "actions": [ - {"name": c.get("function", {}).get("name", ""), - "arguments": c.get("function", {}).get("arguments")} - for c in action_calls - ], + "actions": [_action_entry(c) for c in action_calls], }) try: await asyncio.wait_for(event.wait(), timeout=_APPROVAL_TIMEOUT) diff --git a/synapse/main.py b/synapse/main.py index 5880d9b..22f1e46 100644 --- a/synapse/main.py +++ b/synapse/main.py @@ -28,6 +28,7 @@ from .chat import generate_chat_response, stream_chat_response, _synapse_trace from . import chat as _chat from .ollama_manager import initialize_ollama, initialize_ollama_async, get_ollama_manager from . import frontend_manager as _frontend_manager +from . import playbook_manager from .playbook_manager import PlaybookManager from . import tools as _tools @@ -205,7 +206,7 @@ async def _generate_conversation_title(first_message: str, model: str) -> Option from .memory.store import store, MemoryItem -from .playbooks.store import playbook_store, PlaybookItem +from .playbooks.store import playbook_store from .search import needs_web_search, web_search MEMORY_SERVICE = settings.memory_url @@ -1031,37 +1032,14 @@ def _find_playbook_by_id(playbook_id: str) -> Tuple[Optional[Any], Optional[str] def _persist_playbook(playbook_dict: Dict[str, Any]) -> Dict[str, Any]: - """ - Persist a playbook dict to the store by converting to PlaybookItem. - """ + """Persist a playbook dict to the store. Thin wrapper over the shared, + mergeable implementation in playbook_manager — this call site always does a + full replace (merge=False), matching the frontend form's behavior of always + submitting a complete object.""" try: - # Preserve existing order on update; use provided order (or tail) on create - existing = playbook_store.get_playbook(str(playbook_dict["id"])) - order = existing.order if existing else playbook_dict.get("order", len(playbook_store.all_playbooks())) - - playbook_item = PlaybookItem( - id=str(playbook_dict["id"]), - title=playbook_dict.get("title", ""), - goal=playbook_dict.get("goal", ""), - instructions=playbook_dict.get("instructions", ""), - tags=playbook_dict.get("tags", []), - tools=playbook_dict.get("tools", []), - model=playbook_dict.get("model", ""), - order=order - ) - - playbook_store.add_playbook(playbook_item) - - # Return as dict - return { - "id": playbook_item.id, - "title": playbook_item.title, - "goal": playbook_item.goal, - "instructions": playbook_item.instructions, - "tags": playbook_item.tags, - "tools": playbook_item.tools, - "model": playbook_item.model, - } + return playbook_manager.persist_playbook(dict(playbook_dict), merge=False) + except (ValueError, RuntimeError) as e: + raise HTTPException(status_code=500, detail=f"Failed to persist playbook: {str(e)}") except Exception as e: raise HTTPException(status_code=500, detail=f"Failed to persist playbook: {str(e)}") diff --git a/synapse/playbook_manager.py b/synapse/playbook_manager.py index bd0aefa..15c0d8e 100644 --- a/synapse/playbook_manager.py +++ b/synapse/playbook_manager.py @@ -1,6 +1,59 @@ from typing import List +from uuid import uuid4 from .playbooks.store import playbook_store, PlaybookItem +# Fields a caller can supply; anything else in a persist dict is ignored. +# `order` is deliberately excluded — see persist_playbook. +_EDITABLE_FIELDS = ("title", "goal", "instructions", "tags", "tools", "model") +_FIELD_DEFAULTS = {"title": "", "goal": "", "instructions": "", "tags": [], "tools": [], "model": ""} + + +def persist_playbook(data: dict, *, merge: bool) -> dict: + """Write a playbook dict to the store, returning it as a plain dict. + + `merge=False` is today's behavior (main.py's HTTP form handlers): a full + replace, with pydantic defaults for anything omitted. `merge=True` (used by + the edit_playbook tool) instead keeps each field's *existing* value when the + caller's dict doesn't supply it — a model calling with a partial argument + set must not silently blank out the fields it didn't mention. + + `order` is never taken from `data` in merge mode: it is preserved from the + existing playbook on update, or appended at the tail on create. Position 0 + is unconditionally the active system prompt (see get_main_playbook) — moving + a playbook there is `make_main`'s job, never an accidental side effect of an + ordinary field edit. + """ + existing_id = str(data.get("id") or "") + existing = playbook_store.get_playbook(existing_id) if existing_id else None + + if merge: + fields = {} + for key in _EDITABLE_FIELDS: + if key in data and data[key] is not None: + fields[key] = data[key] + elif existing is not None: + fields[key] = getattr(existing, key) + else: + fields[key] = _FIELD_DEFAULTS[key] + if existing is None and not (fields["title"] and fields["goal"] and fields["instructions"]): + raise ValueError("title, goal, and instructions are required to create a new playbook") + else: + fields = {key: data.get(key, _FIELD_DEFAULTS[key]) for key in _EDITABLE_FIELDS} + + order = existing.order if existing else data.get("order", len(playbook_store.all_playbooks())) + item = PlaybookItem(id=existing_id or str(uuid4()), order=order, **fields) + playbook_store.add_playbook(item) + return item.model_dump() + + +def make_main(playbook_id: str) -> None: + """Reorder so `playbook_id` is position 0 (the active system prompt).""" + all_ids = [p.id for p in playbook_store.all_playbooks()] + if playbook_id not in all_ids: + raise ValueError(f"no playbook with id {playbook_id!r}") + ordered = [playbook_id] + [pid for pid in all_ids if pid != playbook_id] + playbook_store.reorder_playbooks(ordered) + class PlaybookManager: @classmethod diff --git a/synapse/self_edit.py b/synapse/self_edit.py new file mode 100644 index 0000000..9d3dc3a --- /dev/null +++ b/synapse/self_edit.py @@ -0,0 +1,290 @@ +"""The assistant's ability to change what it is: its own playbooks, its own +runtime settings, and (in a source checkout only) its own source files. + +WHAT THIS IS NOT +---------------- +Not a way to skip human review. Every function here is either read-only +(the `preview_*`/`diff_text` functions, safe to call before anything is +approved) or is only ever reached after the per-call approval round-trip in +chat.py — and `edit_playbook`/`edit_settings`/`edit_source` are *always* +gated that way, regardless of the global `action_tool_policy` setting (see +`ALWAYS_ASK_TOOLS`). This module is the mechanism the approval actually acts +on, layered: + + 1. Consent edit_* tools are ACTION tools with a hardcoded approval + floor — chat.py pauses for Approve/Deny even when the + global policy is "allow", so flipping that setting for an + unrelated tool (web_search, say) can never silently unlock + unattended self-modification too. + 2. Real diff the human reviews a diff computed here, server-side, from + what is actually on disk versus the model's proposed + `new_content` — never a diff or description the model wrote + itself. A model can misdescribe a change; it cannot make + difflib misreport one. + 3. Boundary edit_source is confined to one root (settings.project_root) + via the same realpath + `Path.parents` check that closed a + sibling-directory bypass in main.py's /icons/image, plus a + denylist of dangerous subtrees inside that root (.git, the + venv, node_modules, build output, runtime state). + 4. Audit trail every applied source edit is committed to git (best-effort; + a missing git binary or a non-repo root degrades the result + to `commit: None`, it never blocks the write). This is a + reversibility net, not a substitute for layer 1 — the + approval already happened before anything is written. + 5. Size caps MAX_FILE_CHARS/MAX_DIFF_CHARS bound what a single call can + submit or what the approval UI has to render, mirroring + code_run.py's MAX_SOURCE/MAX_OUTPUT. + +A file edit does not hot-reload the running process. Python does not re-import +a changed module on its own, and the frontend's production build is the static +`dist/` the backend serves — editing interface/web/src/*.jsx only affects a +developer's own `npm run dev` session, if one happens to be running. Every +successful edit_source result says so explicitly, because both the model and +the human reviewing it will otherwise reasonably expect an instant effect that +does not happen. +""" +from __future__ import annotations + +import difflib +import os +import subprocess +from pathlib import Path +from typing import Any + +from .memory.store import store +from .nexus_config import settings +from .playbooks.store import playbook_store +from . import playbook_manager + +MAX_FILE_CHARS = 200_000 # chars of proposed file content accepted +MAX_DIFF_CHARS = 20_000 # chars of diff text shown/returned, clipped like code_run.MAX_OUTPUT + +# Subdirectories of settings.project_root that edit_source must never touch, +# even though they're inside the one allowed root. `data`/`runtime` land here +# in a source checkout (see nexus_config.py DATA_DIR/RUNTIME_DIR) and are owned +# by their own tools (edit_settings, the memory store), not raw file writes. +_DENYLIST_SUBDIRS = frozenset({ + ".git", "Promethean", "node_modules", "dist", "__pycache__", + "runtime", "data", ".venv", "venv", +}) + +# Tools that must always pause for human approval, regardless of the global +# action_tool_policy setting. Shared by tools.py (ACTION_TOOLS membership) and +# chat.py (the approval-gating condition) so there is one definition of the +# floor, not two that could drift apart. +ALWAYS_ASK_TOOLS = frozenset({"edit_playbook", "edit_settings", "edit_source"}) + +# Tool names whose approval payload gets a computed, human-readable preview +# attached before the human ever sees the Approve/Deny prompt. +PREVIEWABLE = frozenset({"edit_source", "edit_playbook", "edit_settings"}) + +WINDOWS = os.name == "nt" +_NO_WINDOW = subprocess.CREATE_NO_WINDOW if WINDOWS else 0 + + +class PathError(ValueError): + pass + + +def _resolve_source_path(rel_path: str) -> Path: + """A path the model gave us, resolved and boundary-checked against + settings.project_root. Raises PathError with a human-readable reason on + any rejection — the same message is shown in the pre-approval preview and + returned as the tool's error, so both audiences see exactly why.""" + raw = (rel_path or "").strip() + if not raw: + raise PathError("path is required") + # Cheap rejection before ever touching the filesystem: an absolute path or + # a Windows drive prefix is never a legitimate "file in this project" path. + if raw.startswith(("/", "\\")) or (len(raw) > 1 and raw[1] == ":"): + raise PathError(f"path must be relative to the project root, not absolute: {raw!r}") + + root = Path(os.path.realpath(str(settings.project_root))) + candidate = root / raw + real = Path(os.path.realpath(str(candidate))) + + # Same real == root or root in real.parents pattern as + # icons/compositor.py::_is_allowed_path — a bare str.startswith() here + # would let a sibling directory that merely shares a prefix pass, exactly + # the bug just fixed in main.py's /icons/image. + if not (real == root or root in real.parents): + raise PathError(f"path escapes the project root: {raw!r}") + + try: + top = real.relative_to(root).parts[0] + except (ValueError, IndexError): + top = "" + if top in _DENYLIST_SUBDIRS: + raise PathError(f"{top}/ is off-limits to edit_source — use its own tool if there is one") + + return real + + +def source_checkout_required() -> None: + """Raise if this install has no live, editable source tree to write into. + + In a wheel install, synapse/ lives inside site-packages with no sibling + repo — settings.project_root would just be the installed package dir, and + there is nothing to commit into. Same precedent as nexusos_cli/ncp.py + refusing to start the Vite dev server in a wheel install.""" + if not settings.source_checkout: + raise RuntimeError( + "edit_source is unavailable — this is not a source checkout, so " + "there is no live project tree to edit or commit into." + ) + + +def diff_text(path_display: str, before: str, after: str) -> str: + lines = difflib.unified_diff( + before.splitlines(keepends=True), + after.splitlines(keepends=True), + fromfile=path_display, + tofile=path_display, + ) + text = "".join(lines) + if len(text) <= MAX_DIFF_CHARS: + return text + return text[:MAX_DIFF_CHARS] + f"\n... [truncated at {MAX_DIFF_CHARS} characters, {len(text)} total]" + + +def preview_source_edit(path: str, new_content: str) -> dict: + """Read-only: never raises. Computes the real diff between what's on disk + and the proposed new_content, so the approval UI shows ground truth before + anything is written. Degrades to {"ok": False, "error": ...} on any + rejection (bad path, wheel install, oversized content) rather than + crashing the approval payload — the human still sees why it would fail.""" + try: + source_checkout_required() + real = _resolve_source_path(path) + new_content = new_content or "" + if len(new_content) > MAX_FILE_CHARS: + return {"ok": False, "error": f"new_content is over {MAX_FILE_CHARS} characters"} + before = real.read_text(encoding="utf-8") if real.is_file() else "" + display = str(real.relative_to(Path(os.path.realpath(str(settings.project_root))))) + return { + "ok": True, + "path": display, + "is_new_file": not real.is_file(), + "diff": diff_text(display, before, new_content), + } + except (PathError, RuntimeError) as e: + return {"ok": False, "error": str(e)} + except Exception as e: + return {"ok": False, "error": f"could not compute preview: {e}"} + + +def _git_commit(real_path: Path, summary: str) -> str | None: + """Best-effort audit-trail commit. Never raises, never undoes the write + that already happened — a missing git binary or a project root that isn't + a repo just means commit stays None.""" + root = str(settings.project_root) + try: + rel = str(real_path.relative_to(Path(os.path.realpath(root)))) + message = f"self-edit: {(summary or rel)[:180]}" + subprocess.run( + ["git", "add", "--", rel], cwd=root, check=True, + capture_output=True, creationflags=_NO_WINDOW, + ) + subprocess.run( + ["git", "commit", "-m", message, "--", rel], cwd=root, check=True, + capture_output=True, creationflags=_NO_WINDOW, + ) + sha = subprocess.run( + ["git", "rev-parse", "--short", "HEAD"], cwd=root, check=True, + capture_output=True, text=True, creationflags=_NO_WINDOW, + ) + return sha.stdout.strip() or None + except Exception: + return None + + +def apply_source_edit(path: str, new_content: str, summary: str = "") -> dict: + """Write an approved edit_source call. Re-validates everything preview did + — never trust that nothing changed between preview and approval — then + writes, diffs against the pre-write content, and commits.""" + try: + source_checkout_required() + real = _resolve_source_path(path) + new_content = new_content or "" + if len(new_content) > MAX_FILE_CHARS: + return {"ok": False, "error": f"new_content is over {MAX_FILE_CHARS} characters"} + except (PathError, RuntimeError) as e: + return {"ok": False, "error": str(e)} + + before = real.read_text(encoding="utf-8") if real.is_file() else "" + display = str(real.relative_to(Path(os.path.realpath(str(settings.project_root))))) + real.parent.mkdir(parents=True, exist_ok=True) + real.write_text(new_content, encoding="utf-8") + return { + "ok": True, + "path": display, + "diff": diff_text(display, before, new_content), + "commit": _git_commit(real, summary), + } + + +def preview_playbook_edit(args: dict) -> dict: + """Read-only before/after preview for edit_playbook. Mirrors the merge + semantics of playbook_manager.persist_playbook(merge=True) without writing + anything, so the approval UI shows exactly what will actually change.""" + try: + pb_id = str(args.get("id") or "") + existing = playbook_store.get_playbook(pb_id) if pb_id else None + make_active = bool(args.get("make_active")) + fields = {} + for key in ("title", "goal", "instructions", "tags", "tools", "model"): + if key in args and args[key] is not None: + fields[key] = args[key] + elif existing is not None: + fields[key] = getattr(existing, key) + else: + fields[key] = [] if key in ("tags", "tools") else "" + if existing is None and not (fields["title"] and fields["goal"] and fields["instructions"]): + return {"ok": False, "error": "title, goal, and instructions are required to create a new playbook"} + is_new = existing is None + becomes_main = make_active or (is_new and not playbook_store.all_playbooks()) + return { + "ok": True, + "is_new": is_new, + "before": existing.model_dump() if existing else None, + "after": fields, + "becomes_main_playbook": becomes_main, + } + except Exception as e: + return {"ok": False, "error": f"could not compute preview: {e}"} + + +def preview_settings_edit(args: dict) -> dict: + """Read-only before/after preview for edit_settings, split into keys that + will actually apply versus ones update_settings would silently ignore — + the approval UI must never imply an unknown key will take effect.""" + try: + changes = args.get("changes") or {} + current = store.get_settings() + applied: dict[str, dict[str, Any]] = {} + ignored_unknown: list[str] = [] + for key, value in changes.items(): + if key in store._SETTINGS_DEFAULTS: + applied[key] = {"before": current.get(key), "after": value} + else: + ignored_unknown.append(key) + return { + "ok": True, + "applied": applied, + "ignored_unknown": ignored_unknown, + "policy_change": "action_tool_policy" in applied, + "system_prompt_change": "system_prompt" in applied, + } + except Exception as e: + return {"ok": False, "error": f"could not compute preview: {e}"} + + +def preview_for(name: str, args: dict) -> dict: + """Single dispatch entry point chat.py calls to enrich an approval payload.""" + if name == "edit_source": + return preview_source_edit(args.get("path", ""), args.get("new_content", "")) + if name == "edit_playbook": + return preview_playbook_edit(args) + if name == "edit_settings": + return preview_settings_edit(args) + return {"ok": False, "error": f"no previewer for {name}"} diff --git a/synapse/tools.py b/synapse/tools.py index 03806c9..61eaaf6 100644 --- a/synapse/tools.py +++ b/synapse/tools.py @@ -3,10 +3,15 @@ Ollama drives the calling: `/api/chat` with a `tools` param returns `message.tool_calls`, and this module is just the registry + dispatch. -Most tools READ local state (memory, history, documents, models). A few act: -`web_search`/`fetch_url` make outbound HTTP requests, and `remember` WRITES a -memory fact. The per-playbook allowlist (`PlaybookItem.tools`) is the security -boundary — an action tool only fires when a playbook explicitly lists it. +Most tools READ local state (memory, history, documents, models). Some act: +`web_search`/`fetch_url` make outbound HTTP requests, `remember` WRITES a +memory fact, and `edit_playbook`/`edit_settings`/`edit_source` change the +assistant's own playbooks, settings, and (source checkout only) source code — +see synapse/self_edit.py for what that last group actually does and does not +protect against. The per-playbook allowlist (`PlaybookItem.tools`) is the +first gate — an action tool only fires when a playbook explicitly lists it — +and the three self-edit tools additionally always pause for per-call approval +regardless of the global action_tool_policy (self_edit.ALWAYS_ASK_TOOLS). """ from __future__ import annotations @@ -14,6 +19,8 @@ import json from typing import Awaitable, Callable from . import code_run +from . import playbook_manager +from . import self_edit from .memory.store import store, MemoryItem from .ollama_manager import get_ollama_manager @@ -888,6 +895,81 @@ async def _run_snippet( }) +# Result fence for the three self-edit tools: same "carry the change and what +# happened to it in one block" idea as _run_fence, so the model can't paste a +# claimed diff that doesn't match what was actually applied. +_EDIT_FENCE_LANG = "nexus-edit" + + +def _edit_fence(payload: dict) -> str: + body = json.dumps(payload, ensure_ascii=False).replace("`", "\\u0060") + return f"```{_EDIT_FENCE_LANG}\n{body}\n```" + + +async def _edit_source(path: str = "", new_content: str = "", summary: str = "", **_) -> str: + """ACTION tool: writes a file in the live project tree and commits it. See + synapse/self_edit.py for the boundary check, the size cap, and exactly what + the git commit does and doesn't guarantee.""" + import asyncio as _a + + result = await _a.to_thread(self_edit.apply_source_edit, path, new_content or "", summary or "") + if result.get("ok"): + result["instruction"] = ( + "This was written and committed for real. Paste the fence unchanged, then " + "tell the user plainly that a restart is needed for it to take effect — " + "this file is not reloaded into the running process." + ) + result["fence"] = _edit_fence({"kind": "source", **result}) + return json.dumps(result) + + +async def _edit_playbook( + id: str = "", title: str = "", goal: str = "", instructions: str = "", + tags: list | None = None, tools: list | None = None, model: str = "", + make_active: bool = False, **_, +) -> str: + """ACTION tool: create or update a playbook. Fields left unset keep their + current value — this merges, it does not replace. make_active is a + separate, explicit flag: without it, an edit can never accidentally become + the active system prompt.""" + try: + result = playbook_manager.persist_playbook( + { + "id": id, "title": title, "goal": goal, "instructions": instructions, + "tags": tags, "tools": tools, "model": model, + }, + merge=True, + ) + except ValueError as e: + return json.dumps({"ok": False, "error": str(e)}) + if make_active: + playbook_manager.make_main(result["id"]) + result["is_main_playbook"] = True + result["ok"] = True + result["fence"] = _edit_fence({"kind": "playbook", **result}) + return json.dumps(result) + + +async def _edit_settings(changes: dict | None = None, **_) -> str: + """ACTION tool: change one or more runtime settings. Unknown keys are + silently ignored, exactly like PUT /settings already does.""" + changes = changes if isinstance(changes, dict) else {} + if not changes: + return json.dumps({"ok": False, "error": "changes must be a non-empty object"}) + preview = self_edit.preview_settings_edit({"changes": changes}) + if not preview.get("ok"): + return json.dumps(preview) + if not preview.get("applied"): + return json.dumps({ + "ok": False, + "error": "no recognized settings keys in changes", + "ignored_unknown": preview.get("ignored_unknown", []), + }) + store.update_settings({k: v["after"] for k, v in preview["applied"].items()}) + preview["ok"] = True + preview["fence"] = _edit_fence({"kind": "settings", **preview}) + return json.dumps(preview) + # name -> (schema, callable). Schema is the OpenAI/Ollama function-tool format. REGISTRY: dict[str, tuple[dict, Callable[..., Awaitable[str]]]] = { @@ -1121,13 +1203,114 @@ REGISTRY: dict[str, tuple[dict, Callable[..., Awaitable[str]]]] = { }, _remember, ), + "edit_source": ( + { + "type": "function", + "function": { + "name": "edit_source", + "description": ( + "Rewrite a file in this project's own source tree and commit the " + "change. Requires human approval every time — the person reviews a " + "real diff before anything is written. Send the COMPLETE new file " + "content, not a patch; the server computes the diff itself. `path` " + "is relative to the project root (e.g. \"synapse/tools.py\"), never " + "absolute. Only available in a source checkout, not a packaged " + "install. Writing the file does not restart the running process — " + "say so plainly once it's applied." + ), + "parameters": { + "type": "object", + "properties": { + "path": { + "type": "string", + "description": "Project-relative path to the file, e.g. synapse/tools.py", + }, + "new_content": { + "type": "string", + "description": "The complete replacement content of the file.", + }, + "summary": { + "type": "string", + "description": "One line describing the change, used as the commit message.", + }, + }, + "required": ["path", "new_content"], + }, + }, + }, + _edit_source, + ), + "edit_playbook": ( + { + "type": "function", + "function": { + "name": "edit_playbook", + "description": ( + "Create or update a playbook — the instructions that shape how the " + "assistant behaves. Requires human approval every time. Fields you " + "omit keep their current value; this merges into the existing " + "playbook, it does not replace it. Set make_active=true only when " + "this playbook should become the active system prompt — never as a " + "side effect of an ordinary edit. Omit `id` to create a new playbook." + ), + "parameters": { + "type": "object", + "properties": { + "id": {"type": "string", "description": "Existing playbook id to update; omit to create new."}, + "title": {"type": "string", "description": "Short name for the playbook."}, + "goal": {"type": "string", "description": "One-line statement of what this playbook is for."}, + "instructions": {"type": "string", "description": "The actual instructions/system prompt text."}, + "tags": {"type": "array", "items": {"type": "string"}, "description": "Routing tags."}, + "tools": {"type": "array", "items": {"type": "string"}, "description": "Tool names this playbook grants."}, + "model": {"type": "string", "description": "Preferred Ollama model for this playbook, or blank for auto."}, + "make_active": { + "type": "boolean", + "description": "Set true to make this the active system prompt. Default false.", + }, + }, + "required": [], + }, + }, + }, + _edit_playbook, + ), + "edit_settings": ( + { + "type": "function", + "function": { + "name": "edit_settings", + "description": ( + "Change one or more runtime settings (e.g. model, temperature, " + "action_tool_policy, memory_model). Requires human approval every " + "time. Send only the keys you actually want to change — unrecognized " + "keys are silently ignored, and existing values are left untouched." + ), + "parameters": { + "type": "object", + "properties": { + "changes": { + "type": "object", + "description": "Partial map of setting name to new value.", + }, + }, + "required": ["changes"], + }, + }, + }, + _edit_settings, + ), } # Tools that act (write local state or reach the network). These require an # explicit consent gate (settings.allow_action_tools) on top of the per-playbook -# allowlist — a playbook granting one isn't enough on its own. -ACTION_TOOLS = frozenset({"web_search", "fetch_url", "remember", "run_snippet"}) +# allowlist — a playbook granting one isn't enough on its own. The three +# self-edit tools additionally always pause for per-call approval regardless +# of that global policy — see self_edit.ALWAYS_ASK_TOOLS and chat.py. +ACTION_TOOLS = frozenset({ + "web_search", "fetch_url", "remember", "run_snippet", + "edit_playbook", "edit_settings", "edit_source", +}) # Action tools offered on a *cue* rather than only via a playbook allowlist — # the run track is a standing UI capability like the render window, but unlike diff --git a/tests/test_self_edit.py b/tests/test_self_edit.py new file mode 100644 index 0000000..49e7046 --- /dev/null +++ b/tests/test_self_edit.py @@ -0,0 +1,308 @@ +"""Self-modification: synapse/self_edit.py, plus the ACTION_TOOLS/approval-floor +wiring in tools.py and chat.py that gates it. + +Path-boundary tests mirror the sibling-directory-bypass idiom already used for +/icons/image (tests/test_smoke.py) and icons/compositor.py — the same bug class, +fixed the same way, tested the same way. +""" +import asyncio +import json +import shutil +import subprocess +from pathlib import Path + +import pytest + +from synapse import self_edit +from synapse import tools +from synapse import playbook_manager +from synapse.nexus_config import settings +from synapse.playbooks.store import PlaybookFileStore, PlaybookItem + + +def _requires_git(): + return pytest.mark.skipif(not shutil.which("git"), reason="git not installed") + + +@pytest.fixture +def fake_project(tmp_path, monkeypatch): + """A throwaway project root with the subdirs edit_source must know about.""" + root = tmp_path / "proj" + (root / "synapse").mkdir(parents=True) + (root / ".git").mkdir() + (root / "runtime").mkdir() + (root / "data").mkdir() + monkeypatch.setattr(settings, "project_root", root) + monkeypatch.setattr(settings, "source_checkout", True) + return root + + +# --------------------------------------------------------------------------- +# Path boundary +# --------------------------------------------------------------------------- + +def test_resolve_source_path_allows_a_file_under_root(fake_project): + f = fake_project / "synapse" / "foo.py" + f.write_text("x", encoding="utf-8") + resolved = self_edit._resolve_source_path("synapse/foo.py") + assert resolved == f.resolve() + + +def test_resolve_source_path_denies_sibling_directory_bypass(fake_project, tmp_path): + # A sibling directory that merely shares a string prefix with the allowed + # root ("proj-evil" vs "proj") must not pass — the exact bug class fixed + # today in main.py's /icons/image. + sibling = tmp_path / "proj-evil" + sibling.mkdir() + (sibling / "x.py").write_text("evil", encoding="utf-8") + with pytest.raises(self_edit.PathError): + self_edit._resolve_source_path("../proj-evil/x.py") + + +@pytest.mark.parametrize("subdir", sorted(self_edit._DENYLIST_SUBDIRS)) +def test_resolve_source_path_denies_each_denylisted_subdir(fake_project, subdir): + with pytest.raises(self_edit.PathError): + self_edit._resolve_source_path(f"{subdir}/whatever.txt") + + +def test_resolve_source_path_denies_absolute_paths(fake_project): + with pytest.raises(self_edit.PathError): + self_edit._resolve_source_path("C:\\Windows\\System32\\evil.py") + with pytest.raises(self_edit.PathError): + self_edit._resolve_source_path("/etc/passwd") + + +# --------------------------------------------------------------------------- +# source_checkout gating +# --------------------------------------------------------------------------- + +def test_source_checkout_gating(fake_project, monkeypatch): + monkeypatch.setattr(settings, "source_checkout", False) + with pytest.raises(RuntimeError): + self_edit.source_checkout_required() + preview = self_edit.preview_source_edit("synapse/foo.py", "x") + assert preview["ok"] is False + assert "not a source checkout" in preview["error"] + + +# --------------------------------------------------------------------------- +# Diff computed from ground truth +# --------------------------------------------------------------------------- + +def test_preview_computes_a_real_diff_not_the_models_claim(fake_project): + f = fake_project / "synapse" / "foo.py" + f.write_text("print('old')\n", encoding="utf-8") + preview = self_edit.preview_source_edit("synapse/foo.py", "print('new')\n") + assert preview["ok"] is True + assert "-print('old')" in preview["diff"] + assert "+print('new')" in preview["diff"] + + +def test_preview_rejects_oversized_content(fake_project): + huge = "x" * (self_edit.MAX_FILE_CHARS + 1) + preview = self_edit.preview_source_edit("synapse/foo.py", huge) + assert preview["ok"] is False + + +# --------------------------------------------------------------------------- +# Apply + git commit +# --------------------------------------------------------------------------- + +@_requires_git() +def test_apply_source_edit_writes_and_commits(fake_project): + subprocess.run(["git", "init"], cwd=fake_project, check=True, capture_output=True) + subprocess.run(["git", "config", "user.email", "test@test"], cwd=fake_project, check=True, capture_output=True) + subprocess.run(["git", "config", "user.name", "test"], cwd=fake_project, check=True, capture_output=True) + + result = self_edit.apply_source_edit("synapse/foo.py", "print('applied')\n", "add foo") + assert result["ok"] is True + assert (fake_project / "synapse" / "foo.py").read_text(encoding="utf-8") == "print('applied')\n" + assert result["commit"] is not None + + log = subprocess.run(["git", "log", "--oneline"], cwd=fake_project, check=True, + capture_output=True, text=True) + assert "self-edit: add foo" in log.stdout + + +def test_apply_source_edit_degrades_to_no_commit_when_git_fails(fake_project, monkeypatch): + # No `git init` here — fake_project/.git exists as a plain directory, not a + # real repo, so git commands fail. The write must still succeed. + result = self_edit.apply_source_edit("synapse/foo.py", "print('ok')\n", "x") + assert result["ok"] is True + assert result["commit"] is None + assert (fake_project / "synapse" / "foo.py").read_text(encoding="utf-8") == "print('ok')\n" + + +# --------------------------------------------------------------------------- +# Playbook merge semantics +# --------------------------------------------------------------------------- + +@pytest.fixture +def fake_playbooks(tmp_path, monkeypatch): + store = PlaybookFileStore(tmp_path / "playbooks") + monkeypatch.setattr(playbook_manager, "playbook_store", store) + monkeypatch.setattr(self_edit, "playbook_store", store) + return store + + +def test_edit_playbook_merge_preserves_omitted_fields(fake_playbooks): + fake_playbooks.add_playbook(PlaybookItem( + id="p1", title="Title", goal="Goal", instructions="Do X", + tags=["a"], tools=["remember"], model="m1", order=0, + )) + result = playbook_manager.persist_playbook({"id": "p1", "instructions": "Do Y"}, merge=True) + assert result["instructions"] == "Do Y" + assert result["title"] == "Title" + assert result["goal"] == "Goal" + assert result["tags"] == ["a"] + assert result["tools"] == ["remember"] + assert result["model"] == "m1" + + +def test_edit_playbook_requires_full_fields_for_new(fake_playbooks): + with pytest.raises(ValueError): + playbook_manager.persist_playbook({"instructions": "only this"}, merge=True) + + +def test_edit_playbook_create_appends_at_tail_not_main(fake_playbooks): + fake_playbooks.add_playbook(PlaybookItem(id="p1", title="A", goal="g", instructions="i", order=0)) + result = playbook_manager.persist_playbook( + {"title": "B", "goal": "g2", "instructions": "i2"}, merge=True, + ) + assert result["order"] != 0 + main = fake_playbooks.all_playbooks()[0] + assert main.id == "p1" + + +def test_make_main_reassigns_order_zero_without_corrupting_the_rest(fake_playbooks): + fake_playbooks.add_playbook(PlaybookItem(id="p1", title="A", goal="g", instructions="i", order=0)) + fake_playbooks.add_playbook(PlaybookItem(id="p2", title="B", goal="g", instructions="i", order=1)) + playbook_manager.make_main("p2") + all_pb = fake_playbooks.all_playbooks() + assert all_pb[0].id == "p2" + assert {p.id for p in all_pb} == {"p1", "p2"} + + +def test_preview_playbook_edit_flags_becomes_main(fake_playbooks): + fake_playbooks.add_playbook(PlaybookItem(id="p1", title="A", goal="g", instructions="i", order=0)) + fake_playbooks.add_playbook(PlaybookItem(id="p2", title="B", goal="g", instructions="i", order=1)) + preview = self_edit.preview_playbook_edit({"id": "p2", "make_active": True}) + assert preview["ok"] is True + assert preview["becomes_main_playbook"] is True + + +# --------------------------------------------------------------------------- +# Settings edit +# --------------------------------------------------------------------------- + +def test_edit_settings_ignores_unknown_keys(): + preview = self_edit.preview_settings_edit({"changes": {"model": "llama3.1:8b", "not_a_real_key": 1}}) + assert preview["ok"] is True + assert "model" in preview["applied"] + assert "not_a_real_key" in preview["ignored_unknown"] + + +def test_edit_settings_flags_policy_and_system_prompt_changes(): + preview = self_edit.preview_settings_edit({"changes": {"action_tool_policy": "allow"}}) + assert preview["policy_change"] is True + preview2 = self_edit.preview_settings_edit({"changes": {"system_prompt": "be nice"}}) + assert preview2["system_prompt_change"] is True + + +# --------------------------------------------------------------------------- +# Approval floor + payload enrichment (chat.py wiring) +# --------------------------------------------------------------------------- + +class _EditManager: + """Returns one edit_settings tool_call, then plain content.""" + def __init__(self, args): + self.n = 0 + self.args = args + + async def chat(self, **_): + self.n += 1 + if self.n == 1: + return {"role": "assistant", + "tool_calls": [{"function": {"name": "edit_settings", "arguments": self.args}}]} + return {"role": "assistant", "content": "done"} + + +def _drive(policy, decision, args, monkeypatch, preview_stub=None): + from synapse import chat as chatmod + + async def fake_dispatch(name, call_args): + return json.dumps({"ok": True}) + monkeypatch.setattr(tools, "dispatch", fake_dispatch) + + if preview_stub is not None: + monkeypatch.setattr(self_edit, "preview_for", lambda name, a: preview_stub) + + async def run(): + messages = [{"role": "user", "content": "change a setting"}] + schemas = tools.schemas_for(["edit_settings"]) + gen = chatmod._run_tool_loop(_EditManager(args), messages, "m", schemas, None, None, + conversation_id="conv2", policy=policy) + statuses = [] + approve_payload = None + async for s in gen: + statuses.append(s) + if s.startswith("__approve__"): + approve_payload = json.loads(s[len("__approve__"):]) + w = chatmod.pending_approvals["conv2"] + w["decisions"] = {"edit_settings": decision} + w["event"].set() + return statuses, approve_payload + + return asyncio.run(run()) + + +def test_edit_settings_requires_approval_even_when_policy_is_allow(monkeypatch): + statuses, payload = _drive("allow", True, {"changes": {"model": "x"}}, monkeypatch) + assert any(s.startswith("__approve__") for s in statuses) + assert payload is not None + + +def test_approve_payload_carries_the_computed_preview(monkeypatch): + stub = {"ok": True, "applied": {"model": {"before": "a", "after": "x"}}} + statuses, payload = _drive("ask", True, {"changes": {"model": "x"}}, monkeypatch, preview_stub=stub) + assert payload["actions"][0]["preview"] == stub + + +def test_approve_payload_degrades_gracefully_when_preview_raises(monkeypatch): + def boom(name, args): + raise RuntimeError("preview exploded") + monkeypatch.setattr(self_edit, "preview_for", boom) + statuses, payload = _drive("ask", True, {"changes": {"model": "x"}}, monkeypatch) + assert payload is not None + assert payload["actions"][0]["preview"]["ok"] is False + + +def test_action_tools_include_self_edit_and_are_gated_by_consent(): + allow = ["edit_playbook", "edit_settings", "edit_source"] + on = [s["function"]["name"] for s in tools.schemas_for(allow, allow_actions=True)] + off = [s["function"]["name"] for s in tools.schemas_for(allow, allow_actions=False)] + assert set(on) == set(allow) + assert off == [] + for name in allow: + assert tools.is_action(name) + assert name in self_edit.ALWAYS_ASK_TOOLS + + +# --------------------------------------------------------------------------- +# End-to-end: dispatch("edit_source", ...) through the real preview/apply path +# --------------------------------------------------------------------------- + +@_requires_git() +def test_dispatch_edit_source_end_to_end(fake_project): + subprocess.run(["git", "init"], cwd=fake_project, check=True, capture_output=True) + subprocess.run(["git", "config", "user.email", "test@test"], cwd=fake_project, check=True, capture_output=True) + subprocess.run(["git", "config", "user.name", "test"], cwd=fake_project, check=True, capture_output=True) + + out = asyncio.run(tools.dispatch("edit_source", { + "path": "synapse/foo.py", "new_content": "print('e2e')\n", "summary": "e2e test", + })) + payload = json.loads(out) + assert payload["ok"] is True + assert payload["commit"] is not None + assert "nexus-edit" in payload["fence"] + assert (fake_project / "synapse" / "foo.py").read_text(encoding="utf-8") == "print('e2e')\n"