fix(preview): hung-frame watchdog + require approval for guessed action calls
A runaway preview script (sync infinite loop, or a re-render loop outpacing the bootstrap's own coalescing) had nothing detecting it - the frame just spun. The bootstrap now heartbeats every second, and the parent tears the iframe down if it goes _WATCHDOG_MS silent, whatever the cause. _coerce_tool_calls recovers a tool call guessed from `content` for models with no native tool_calls field. That guess is weaker evidence than the API's own structured field - a model can land on JSON shaped like a call while only meaning to describe one - so an action tool recovered this way now always requires approval, even under the "allow" policy that lets a native tool_calls field run unattended.
This commit is contained in:
@@ -243,6 +243,9 @@ const _PREVIEW_BOOTSTRAP = `<script>
|
|||||||
observers[observers.length - 1].observe(document.body);
|
observers[observers.length - 1].observe(document.body);
|
||||||
}
|
}
|
||||||
setTimeout(post, 300); // late paints: fonts, async draws, first rAF frame
|
setTimeout(post, 300); // late paints: fonts, async draws, first rAF frame
|
||||||
|
// Heartbeat: the parent's watchdog needs a message even when nothing is
|
||||||
|
// changing, or an idle-but-alive frame reads the same as a hung one.
|
||||||
|
setInterval(post, 1000);
|
||||||
});
|
});
|
||||||
})();
|
})();
|
||||||
</script>`;
|
</script>`;
|
||||||
@@ -304,6 +307,14 @@ const _MIN_PREVIEW_H = 160;
|
|||||||
const _MAX_PREVIEW_H = 720;
|
const _MAX_PREVIEW_H = 720;
|
||||||
const _MAX_H_STEPS = 60;
|
const _MAX_H_STEPS = 60;
|
||||||
|
|
||||||
|
// A frame that never posts again — a synchronous `while(true)` in the user's
|
||||||
|
// own script, or a runaway re-render loop the bootstrap's own coalescing
|
||||||
|
// can't outpace — has nothing else to signal it. Silence past this long since
|
||||||
|
// mount (or since the last message) is treated as hung and the frame is torn
|
||||||
|
// down; the bootstrap's 1s heartbeat means a merely-idle-but-alive frame never
|
||||||
|
// gets close to this.
|
||||||
|
const _WATCHDOG_MS = 6000;
|
||||||
|
|
||||||
// Live preview for a renderable fenced block: a Preview/Code toggle rendered
|
// Live preview for a renderable fenced block: a Preview/Code toggle rendered
|
||||||
// via a sandboxed iframe whose document is an encoded data: URL.
|
// via a sandboxed iframe whose document is an encoded data: URL.
|
||||||
//
|
//
|
||||||
@@ -407,9 +418,11 @@ function PreviewFrame({ lang, value, expanded }) {
|
|||||||
const [doc, setDoc] = useState("");
|
const [doc, setDoc] = useState("");
|
||||||
const [buildError, setBuildError] = useState("");
|
const [buildError, setBuildError] = useState("");
|
||||||
const [height, setHeight] = useState(240);
|
const [height, setHeight] = useState(240);
|
||||||
|
const [hung, setHung] = useState(false);
|
||||||
const frameRef = useRef(null);
|
const frameRef = useRef(null);
|
||||||
const heightRef = useRef(240); // mirrors `height` so the listener needn't re-subscribe
|
const heightRef = useRef(240); // mirrors `height` so the listener needn't re-subscribe
|
||||||
const stepsRef = useRef(0);
|
const stepsRef = useRef(0);
|
||||||
|
const lastMsgRef = useRef(0); // set for real by the watchdog effect below
|
||||||
|
|
||||||
// Receive the bootstrap's reports. The frame is on an opaque origin, so
|
// Receive the bootstrap's reports. The frame is on an opaque origin, so
|
||||||
// e.origin is the string "null" and proves nothing - identify the sender by
|
// e.origin is the string "null" and proves nothing - identify the sender by
|
||||||
@@ -419,6 +432,7 @@ function PreviewFrame({ lang, value, expanded }) {
|
|||||||
if (!frameRef.current || e.source !== frameRef.current.contentWindow) return;
|
if (!frameRef.current || e.source !== frameRef.current.contentWindow) return;
|
||||||
const data = e.data;
|
const data = e.data;
|
||||||
if (!data || data.__nexusPreview !== 1) return;
|
if (!data || data.__nexusPreview !== 1) return;
|
||||||
|
lastMsgRef.current = Date.now();
|
||||||
|
|
||||||
if (typeof data.err === "string" && data.err) setError(data.err);
|
if (typeof data.err === "string" && data.err) setError(data.err);
|
||||||
|
|
||||||
@@ -435,6 +449,23 @@ function PreviewFrame({ lang, value, expanded }) {
|
|||||||
return () => window.removeEventListener("message", onMessage);
|
return () => window.removeEventListener("message", onMessage);
|
||||||
}, []);
|
}, []);
|
||||||
|
|
||||||
|
// Watchdog: a frame that goes silent past _WATCHDOG_MS — most likely a
|
||||||
|
// synchronous infinite loop in the model's own script, which blocks even
|
||||||
|
// the bootstrap's heartbeat from ever running — gets torn down rather than
|
||||||
|
// left spinning. Checked on an interval rather than a single timeout so a
|
||||||
|
// message arriving late (slow compile, heavy first paint) keeps resetting
|
||||||
|
// the clock instead of tripping early.
|
||||||
|
useEffect(() => {
|
||||||
|
lastMsgRef.current = Date.now();
|
||||||
|
const id = setInterval(() => {
|
||||||
|
if (Date.now() - lastMsgRef.current > _WATCHDOG_MS) {
|
||||||
|
setHung(true);
|
||||||
|
clearInterval(id);
|
||||||
|
}
|
||||||
|
}, 1000);
|
||||||
|
return () => clearInterval(id);
|
||||||
|
}, [lang, value]);
|
||||||
|
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
let current = true;
|
let current = true;
|
||||||
setDoc("");
|
setDoc("");
|
||||||
@@ -448,9 +479,13 @@ function PreviewFrame({ lang, value, expanded }) {
|
|||||||
}, [lang, value]);
|
}, [lang, value]);
|
||||||
|
|
||||||
// A build failure (JSX that doesn't parse) has no document to show at all, so
|
// A build failure (JSX that doesn't parse) has no document to show at all, so
|
||||||
// the message stands in for the frame rather than sitting under it.
|
// the message stands in for the frame rather than sitting under it. A hung
|
||||||
const frameUrl = doc ? `data:text/html;charset=utf-8,${encodeURIComponent(doc)}` : "";
|
// frame tears down the same way: dropping frameUrl unmounts the iframe,
|
||||||
const shown = buildError || error;
|
// which is what actually stops a runaway script from holding the tab.
|
||||||
|
const frameUrl = doc && !hung ? `data:text/html;charset=utf-8,${encodeURIComponent(doc)}` : "";
|
||||||
|
const shown = hung
|
||||||
|
? "Preview stopped responding (likely an infinite loop) and was stopped."
|
||||||
|
: buildError || error;
|
||||||
|
|
||||||
return (
|
return (
|
||||||
<>
|
<>
|
||||||
|
|||||||
+7
-2
@@ -276,18 +276,23 @@ async def _run_tool_loop(manager, messages, model, tool_schemas, temperature, nu
|
|||||||
)
|
)
|
||||||
if not isinstance(msg, dict):
|
if not isinstance(msg, dict):
|
||||||
break # None/error or no tool support -> fall back to plain stream
|
break # None/error or no tool support -> fall back to plain stream
|
||||||
|
native = bool(msg.get("tool_calls"))
|
||||||
calls = _coerce_tool_calls(msg, allowed_names)
|
calls = _coerce_tool_calls(msg, allowed_names)
|
||||||
if not calls:
|
if not calls:
|
||||||
break
|
break
|
||||||
# Normalize content-JSON tool calls into the shape later turns expect.
|
# Normalize content-JSON tool calls into the shape later turns expect.
|
||||||
if not msg.get("tool_calls"):
|
if not native:
|
||||||
msg = {"role": "assistant", "content": "", "tool_calls": calls}
|
msg = {"role": "assistant", "content": "", "tool_calls": calls}
|
||||||
messages.append(msg)
|
messages.append(msg)
|
||||||
|
|
||||||
# If any action tool needs per-call approval, pause and wait for the user.
|
# If any action tool needs per-call approval, pause and wait for the user.
|
||||||
|
# A call recovered by guessing at `content` (no native tool_calls field)
|
||||||
|
# is a weaker signal than the API's own structured field — a model can
|
||||||
|
# land on JSON shaped like a call while only meaning to describe one, so
|
||||||
|
# it always goes through approval regardless of policy, even "allow".
|
||||||
decisions = None
|
decisions = None
|
||||||
action_calls = [c for c in calls if _tools.is_action(c.get("function", {}).get("name", ""))]
|
action_calls = [c for c in calls if _tools.is_action(c.get("function", {}).get("name", ""))]
|
||||||
if policy == "ask" and action_calls:
|
if (policy == "ask" or not native) and action_calls:
|
||||||
event = asyncio.Event()
|
event = asyncio.Event()
|
||||||
# Single-use capability token, delivered only to the client that owns
|
# Single-use capability token, delivered only to the client that owns
|
||||||
# this stream. /chat/approve requires it, so knowing the (guessable,
|
# this stream. /chat/approve requires it, so knowing the (guessable,
|
||||||
|
|||||||
@@ -93,6 +93,52 @@ def test_ask_policy_skips_on_deny(monkeypatch):
|
|||||||
assert any(m["role"] == "tool" and "declined" in m["content"] for m in messages)
|
assert any(m["role"] == "tool" and "declined" in m["content"] for m in messages)
|
||||||
|
|
||||||
|
|
||||||
|
class _ContentJsonActionManager:
|
||||||
|
"""Small-model shape: dumps the action call into `content`, no native
|
||||||
|
`tool_calls` field — the lower-confidence path the "allow" bypass must
|
||||||
|
not trust."""
|
||||||
|
def __init__(self):
|
||||||
|
self.n = 0
|
||||||
|
|
||||||
|
async def chat(self, **_):
|
||||||
|
self.n += 1
|
||||||
|
if self.n == 1:
|
||||||
|
return {"role": "assistant",
|
||||||
|
"content": json.dumps({"name": "remember", "arguments": {"text": "x"}})}
|
||||||
|
return {"role": "assistant", "content": "done"}
|
||||||
|
|
||||||
|
|
||||||
|
def test_content_json_action_call_asks_even_under_allow_policy(monkeypatch):
|
||||||
|
"""A call recovered by guessing at `content` is weaker evidence than the
|
||||||
|
API's own structured tool_calls field — a model can land on JSON shaped
|
||||||
|
like a call while only meaning to describe one. It must still go through
|
||||||
|
approval even when action_tool_policy is "allow", the default that lets a
|
||||||
|
*native* tool_calls field run unattended."""
|
||||||
|
from synapse import chat as chatmod
|
||||||
|
|
||||||
|
async def fake_dispatch(name, args):
|
||||||
|
return "saved-ok"
|
||||||
|
monkeypatch.setattr(tools, "dispatch", fake_dispatch)
|
||||||
|
|
||||||
|
async def run():
|
||||||
|
messages = [{"role": "user", "content": "remember x"}]
|
||||||
|
schemas = tools.schemas_for(["remember"])
|
||||||
|
gen = chatmod._run_tool_loop(_ContentJsonActionManager(), messages, "m", schemas, None, None,
|
||||||
|
conversation_id="conv", policy="allow")
|
||||||
|
statuses = []
|
||||||
|
async for s in gen:
|
||||||
|
statuses.append(s)
|
||||||
|
if s.startswith("__approve__"):
|
||||||
|
w = chatmod.pending_approvals["conv"]
|
||||||
|
w["decisions"] = {"remember": True}
|
||||||
|
w["event"].set()
|
||||||
|
return statuses
|
||||||
|
|
||||||
|
statuses = asyncio.run(run())
|
||||||
|
assert any(s.startswith("__approve__") for s in statuses)
|
||||||
|
assert "__status__remember" in statuses
|
||||||
|
|
||||||
|
|
||||||
def test_action_tools_gated_by_consent():
|
def test_action_tools_gated_by_consent():
|
||||||
allow = ["search_memory", "web_search", "remember", "fetch_url"]
|
allow = ["search_memory", "web_search", "remember", "fetch_url"]
|
||||||
on = [s["function"]["name"] for s in tools.schemas_for(allow, allow_actions=True)]
|
on = [s["function"]["name"] for s in tools.schemas_for(allow, allow_actions=True)]
|
||||||
|
|||||||
Reference in New Issue
Block a user