diff --git a/src/benchflow/agents/registry.py b/src/benchflow/agents/registry.py index 253f602b3..6ea34856c 100644 --- a/src/benchflow/agents/registry.py +++ b/src/benchflow/agents/registry.py @@ -294,16 +294,39 @@ def _js_agent_launch(binary: str, args: str = "") -> str: def _json_settings_merge(path: str, mutator: str) -> str: - """Idempotent JSON-settings merge as a one-line bash snippet.""" - py = ( - "import json,os,pathlib;" - f"p=pathlib.Path(os.path.expandvars(os.path.expanduser({path!r})));" - "p.parent.mkdir(parents=True, exist_ok=True);" - "d=json.loads(p.read_text()) if p.exists() and p.read_text().strip() else {};" + """Idempotent JSON-settings merge as a one-line bash snippet. + + Runs on the Node runtime ``_NODE_INSTALL`` provisions, not on ``python3``: + policy setup executes inside the *task* image, which owes BenchFlow no + interpreter, so a Python-free image used to abort a rollout with exit 127 + before ACP launch (#1047). Callers must therefore be agents whose install + guarantees ``_BENCHFLOW_NODE_PREFIX`` — every JS agent does, via + ``_js_agent_install`` or the mimo manifest. + + ``mutator`` is a JavaScript statement operating on ``d``. The escape pass + reproduces ``json.dumps``' ``ensure_ascii`` so upgrading BenchFlow leaves + an existing settings file byte-identical: agent homes can arrive + pre-populated from the host (gemini copies ``~/.gemini/settings.json`` in + via ``subscription_auth``), and rewriting a user's non-ASCII values on + first run would break the idempotence this merge promises. It starts at + U+007F because ``JSON.stringify`` has already escaped in-string control + characters, while the newlines and indent it emits must stay literal. + """ + js = ( + "const fs=require('fs'),pa=require('path');" + "const p=process.argv[1];" + "fs.mkdirSync(pa.dirname(p),{recursive:true});" + "let d={};" + "try{const t=fs.readFileSync(p,'utf8');if(t.trim())d=JSON.parse(t);}" + "catch(e){if(e.code!=='ENOENT')throw e;}" f"{mutator};" - "p.write_text(json.dumps(d, indent=2) + '\\n')" + "fs.writeFileSync(p,JSON.stringify(d,null,2)" + ".replace(/[\\u007f-\\uffff]/g," + "c=>'\\\\u'+c.charCodeAt(0).toString(16).padStart(4,'0'))+'\\n');" ) - return f"python3 -c {shlex.quote(py)}" + # The path stays a shell word so the surrounding bash expands + # $BENCHFLOW_AGENT_HOME, matching what os.path.expandvars did before. + return f'{_BENCHFLOW_NODE_PREFIX}/bin/node -e {shlex.quote(js)} "{path}"' # OpenCode-family proxy fix: OpenCode and its MiMo fork validate provider/model @@ -545,9 +568,10 @@ class AgentConfig: ), disallow_web_tools_setup_cmd=_json_settings_merge( "$BENCHFLOW_AGENT_HOME/.claude/settings.json", - 'd.setdefault("permissions",{}).setdefault("deny",[]);' - '[d["permissions"]["deny"].append(t) for t in ["WebSearch","WebFetch"] ' - 'if t not in d["permissions"]["deny"]]', + "if(!('permissions' in d))d.permissions={};" + "if(!('deny' in d.permissions))d.permissions.deny=[];" + 'for(const t of ["WebSearch","WebFetch"])' + "if(!d.permissions.deny.includes(t))d.permissions.deny.push(t)", ), disallow_web_tools_owned_paths=["$HOME/.claude"], supports_acp_set_model=False, @@ -690,10 +714,10 @@ class AgentConfig: ), disallow_web_tools_setup_cmd=_json_settings_merge( "$BENCHFLOW_AGENT_HOME/.gemini/settings.json", - 'd.setdefault("tools",{}).setdefault("exclude",[]);' - '[d["tools"]["exclude"].append(t) for t in ' - '["google_web_search","web_fetch"] ' - 'if t not in d["tools"]["exclude"]]', + "if(!('tools' in d))d.tools={};" + "if(!('exclude' in d.tools))d.tools.exclude=[];" + 'for(const t of ["google_web_search","web_fetch"])' + "if(!d.tools.exclude.includes(t))d.tools.exclude.push(t)", ), disallow_web_tools_owned_paths=["$HOME/.gemini"], ), @@ -721,7 +745,7 @@ class AgentConfig: }, disallow_web_tools_setup_cmd=_json_settings_merge( "$BENCHFLOW_AGENT_HOME/.config/opencode/opencode.json", - 'd.setdefault("tools",{})["webfetch"]=False', + "if(!('tools' in d))d.tools={};d.tools.webfetch=false", ), disallow_web_tools_owned_paths=["$HOME/.config/opencode"], ), @@ -770,7 +794,7 @@ class AgentConfig: }, disallow_web_tools_setup_cmd=_json_settings_merge( "$BENCHFLOW_AGENT_HOME/.config/mimocode/mimocode.json", - 'd.setdefault("tools",{})["webfetch"]=False', + "if(!('tools' in d))d.tools={};d.tools.webfetch=false", ), disallow_web_tools_owned_paths=["$HOME/.config/mimocode"], ), diff --git a/tests/test_internet_policy.py b/tests/test_internet_policy.py index f34e2daaf..6e0ef3a5a 100644 --- a/tests/test_internet_policy.py +++ b/tests/test_internet_policy.py @@ -1,4 +1,5 @@ import json +import shutil import subprocess from types import SimpleNamespace from unittest.mock import AsyncMock, MagicMock, patch @@ -266,13 +267,25 @@ def test_apply_web_policy_noop_when_not_disallowed(): def _run_setup_cmd(agent_name: str, tmp_path) -> dict: """Execute an agent's disallow_web_tools_setup_cmd and return the JSON it wrote.""" - from benchflow.agents.registry import AGENTS + from benchflow.agents.registry import _BENCHFLOW_NODE_PREFIX, AGENTS cfg = AGENTS[agent_name] assert cfg.disallow_web_tools_setup_cmd, f"{agent_name} has no setup_cmd" + # JSON-merge policies target the Node runtime BenchFlow provisions inside the + # sandbox (#1047); a dev host has no such prefix, so retarget the local one. + # This is what kept the old python3 dependency invisible to these tests, so + # coverage on a real task image lives in the container test below. + cmd = cfg.disallow_web_tools_setup_cmd + sandbox_node = f"{_BENCHFLOW_NODE_PREFIX}/bin/node" + if sandbox_node in cmd: + host_node = shutil.which("node") + if host_node is None: + pytest.skip("no node runtime on PATH to exercise the JSON-merge policy") + cmd = cmd.replace(sandbox_node, host_node) + result = subprocess.run( - ["bash", "-c", cfg.disallow_web_tools_setup_cmd], + ["bash", "-c", cmd], env={"BENCHFLOW_AGENT_HOME": str(tmp_path), "PATH": "/usr/bin:/bin"}, capture_output=True, text=True, @@ -424,3 +437,103 @@ def test_task_toml_missing_allow_internet_defaults_to_allowed(tmp_path): task = Task(task_dir) assert _task_disallows_internet(task) is False + + +def test_json_merge_policies_do_not_borrow_task_image_interpreters(): + """Policy setup runs in the task image, which owes BenchFlow no runtime. + + The JSON merge used to shell out to ``python3``; a Python-free task image + with ``allow_internet = false`` then aborted at exit 127 before ACP launch + (#1047). + """ + from benchflow.agents.registry import _BENCHFLOW_NODE_PREFIX, AGENTS + + for agent in ("claude-agent-acp", "gemini", "opencode", "mimo"): + cmd = AGENTS[agent].disallow_web_tools_setup_cmd + assert "python3" not in cmd, agent + assert cmd.startswith(f"{_BENCHFLOW_NODE_PREFIX}/bin/node "), agent + + +def test_node_backed_policies_are_guaranteed_by_their_own_install(): + """Whoever depends on the Node runtime must also be the one installing it. + + Pins the ownership boundary #1047 is about, rather than today's four + agents: a policy may only reach for the Node prefix if that agent's own + install_cmd provisions it. + """ + from benchflow.agents.registry import _BENCHFLOW_NODE_PREFIX, AGENTS + + for name, cfg in AGENTS.items(): + cmd = cfg.disallow_web_tools_setup_cmd or "" + if f"{_BENCHFLOW_NODE_PREFIX}/bin/node" not in cmd: + continue + assert _BENCHFLOW_NODE_PREFIX in (cfg.install_cmd or ""), ( + f"{name} runs its no-web policy on the BenchFlow Node runtime but " + "its install does not provision one" + ) + + +@pytest.mark.skipif( + not shutil.which("docker"), reason="needs a Docker daemon to build the task image" +) +async def test_no_web_policies_apply_in_a_python_free_task_image(): + """End-to-end guard for #1047 on an image that ships no Python at all. + + Provisions the agent runtime first — exactly what the JS installers do + before policy application — then applies each policy twice, so the run + covers the merge content and the idempotence it promises. + """ + from benchflow.agents.install import apply_web_tool_policy + from benchflow.agents.registry import _NODE_INSTALL, AGENTS + + expected = { + "claude-agent-acp": ( + "/root/.claude/settings.json", + {"permissions": {"deny": ["WebSearch", "WebFetch"]}}, + ), + "gemini": ( + "/root/.gemini/settings.json", + {"tools": {"exclude": ["google_web_search", "web_fetch"]}}, + ), + "opencode": ( + "/root/.config/opencode/opencode.json", + {"tools": {"webfetch": False}}, + ), + "mimo": ( + "/root/.config/mimocode/mimocode.json", + {"tools": {"webfetch": False}}, + ), + } + + def sh(*args: str) -> subprocess.CompletedProcess: + return subprocess.run(args, capture_output=True, text=True, timeout=900) + + container = subprocess.check_output( + ["docker", "run", "--detach", "--rm", "ubuntu:24.04", "sleep", "infinity"], + text=True, + ).strip() + + class ContainerEnv: + async def exec(self, command: str, *, timeout_sec=None, **_kwargs): + done = sh("docker", "exec", container, "bash", "-lc", command) + return SimpleNamespace( + return_code=done.returncode, stdout=done.stdout, stderr=done.stderr + ) + + try: + probe = sh("docker", "exec", container, "bash", "-lc", "command -v python3") + assert probe.returncode != 0, "base image unexpectedly ships python3" + + provision = sh("docker", "exec", container, "bash", "-lc", _NODE_INSTALL) + assert provision.returncode == 0, provision.stderr[-400:] + + for agent, (path, want) in expected.items(): + for _ in range(2): + await apply_web_tool_policy( + ContainerEnv(), agent, AGENTS[agent], "/root", disallow=True + ) + written = sh("docker", "exec", container, "cat", path) + assert written.returncode == 0, f"{agent}: {path} not written" + assert json.loads(written.stdout) == want, agent + finally: + sh("docker", "rm", "--force", container)