From 4d04c652f2a03886f013577e0f7b538c1d6c4c0b Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Thu, 25 Jun 2026 21:27:11 -0700 Subject: [PATCH] fix(curator): make external-skill write guard actually fire during curation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The salvaged #51875 added a background-review write guard in skill_manage that refuses mutations to skills.external_dirs skills — but it only fires when is_background_review() is true. The curator's LLM review fork ran with the default _memory_write_origin='assistant_tool', so the guard never triggered during the exact curation pass it exists to protect against (GH-47688). - Set _memory_write_origin='background_review' on the curator review fork so turn_context binds it onto the write-origin ContextVar and the guard fires. - Add a regression test asserting the fork runs under the background_review origin (the invariant linking the fork to the guard). - AUTHOR_MAP: map yu-xin-c for the salvaged commit. --- agent/curator.py | 8 ++++++ scripts/release.py | 2 +- tests/agent/test_curator.py | 51 +++++++++++++++++++++++++++++++++++++ 3 files changed, 60 insertions(+), 1 deletion(-) diff --git a/agent/curator.py b/agent/curator.py index be8ee0b13e8..6843205c684 100644 --- a/agent/curator.py +++ b/agent/curator.py @@ -1846,6 +1846,14 @@ def _run_llm_review(prompt: str) -> Dict[str, Any]: # Disable recursive nudges — the curator must never spawn its own review. review_agent._memory_nudge_interval = 0 review_agent._skill_nudge_interval = 0 + # Tag this fork as autonomous background curation so skill_manage's + # background-review write guard fires. Without this the fork inherits + # the default "assistant_tool" origin, is_background_review() is False, + # and the external/bundled/hub-installed skill_manage guards never + # trigger during the curation pass they exist to protect against. + # turn_context.py binds this onto the write-origin ContextVar at turn + # start (see agent/turn_context.py). + review_agent._memory_write_origin = "background_review" # Redirect the forked agent's stdout/stderr to /dev/null while it # runs so its tool-call chatter doesn't pollute the foreground diff --git a/scripts/release.py b/scripts/release.py index 42e4091742f..b6f1d8cc7d0 100755 --- a/scripts/release.py +++ b/scripts/release.py @@ -1645,7 +1645,7 @@ AUTHOR_MAP = { "mordred@inaugust.com": "emonty", "rodrigoeq@hotmail.com": "rodrigoeqnit", "soliva.johnpaul@icloud.com": "jonpol01", - "2182712990@qq.com": "yu-xin-c", # PR #32122 (Docker audio bridge notes) + "2182712990@qq.com": "yu-xin-c", # PR #51875 salvage (protect external_dirs skills from curator) "baxter@bitreserve.ai": "BaxBit", # PR #30200 (Svix webhook signature validation) "chris.eth@qq.com": "duyua9", # PR #10949 (render object config values structurally) "ethie@nous": "ethernet8023", # PR #29342 (TUI clipboard copy on linux/wayland) diff --git a/tests/agent/test_curator.py b/tests/agent/test_curator.py index 26a13edf922..7fab220b963 100644 --- a/tests/agent/test_curator.py +++ b/tests/agent/test_curator.py @@ -1119,3 +1119,54 @@ def test_curator_slot_is_canonical_aux_task(): # 4. web/src/pages/ModelsPage.tsx is checked at build time; the tsx # array and this tuple share a ``Must match _AUX_TASK_SLOTS`` comment. + + +def test_review_fork_runs_under_background_review_origin(curator_env, monkeypatch): + """The curator LLM fork must tag itself as background_review. + + This is the keystone that makes skill_manager_tool's + ``_background_review_write_guard`` fire during a curation pass. Without + ``_memory_write_origin = "background_review"`` on the fork, the agent + inherits the default ``assistant_tool`` origin, ``is_background_review()`` + stays False, and the external/bundled/hub-installed skill_manage guards + never trigger — leaving the LLM agent free to mutate skills.external_dirs + skills (GH-47688). turn_context.py binds this attribute onto the + write-origin ContextVar at turn start, so asserting it is set is the + enforceable invariant linking the fork to the guard. + """ + curator = curator_env["curator"] + + # The curator_env fixture stubs out _run_llm_review wholesale; this test + # exercises the real implementation, so reload the module to restore it. + import importlib + importlib.reload(curator) + monkeypatch.setattr(curator, "_load_config", lambda: {}) + + captured = {} + + class _StubAgent: + def __init__(self, *args, **kwargs): + # AIAgent.__init__ normally sets this default; mirror it so the + # production assignment in _run_llm_review is what flips it. + self._memory_write_origin = "assistant_tool" + self._memory_nudge_interval = 10 + self._skill_nudge_interval = 10 + self.platform = kwargs.get("platform") + self._session_messages = [] + + def run_conversation(self, user_message=None, **kwargs): + # Capture the origin AT RUN TIME — i.e. after _run_llm_review has + # finished configuring the fork, which is exactly when it matters. + captured["write_origin"] = self._memory_write_origin + return {"final_response": "no change"} + + monkeypatch.setattr("run_agent.AIAgent", _StubAgent) + + meta = curator._run_llm_review("review prompt") + + assert meta.get("error") is None, meta.get("error") + assert captured.get("write_origin") == "background_review", ( + "curator review fork did not set _memory_write_origin to " + "'background_review' — the skill_manage background-review write " + "guard would not fire (GH-47688 regression)" + )