diff --git a/docs/core/mcp.md b/docs/core/mcp.md index e9300a5..6c5ff39 100644 --- a/docs/core/mcp.md +++ b/docs/core/mcp.md @@ -95,6 +95,17 @@ Each role includes the tools of the roles above it in this list. `set_alert_action` (the playbook a rule runs when it fires), `delete_monitor`, `delete_alert_rule`, `prune_docker`. +`save_playbook` and `delete_playbook` write this instance's own playbooks into +`steward-local`, the same as Steward's editor (#5748). That is automation for +these hosts that isn't part of Steward itself; Steward's built-in playbooks are +changed in its repository, and a git source by its repo, so both are refused. +A save must parse as a list of plays or nothing is written; it won't overwrite +an existing playbook without `replace=true`; and it then runs +`ansible-playbook --syntax-check` (no host contacted) and says what that found. +The loop for building one is save → `run_playbook` in check mode on one target +→ `get_ansible_run` → fix and save again → apply with `check=false`. The audit +row records the size, never the content. + The client sees each tool's full contract: arguments, what an empty value means, and what comes back. That contract is the tool's docstring, so read the docstrings in `steward/mcp/tools/` for the detail. diff --git a/steward/ansible/executor.py b/steward/ansible/executor.py index 23034db..315feb1 100644 --- a/steward/ansible/executor.py +++ b/steward/ansible/executor.py @@ -329,6 +329,36 @@ def ansible_env(creds: dict | None, base_env) -> dict: return env +# Long enough for Ansible to load its collections on a cold start; a syntax +# check reaches no host. +SYNTAX_CHECK_SECONDS = 60 +SYNTAX_OUTPUT_CHARS = 2000 + + +async def syntax_check(source_path: str, relative_path: str) -> tuple[bool | None, str]: + """`ansible-playbook --syntax-check` on a saved playbook (#5748): catches + what a YAML parse doesn't, such as a module name Ansible can't resolve or + a missing role. Runs in the source's folder against a throwaway localhost + inventory, so no host is contacted. Returns (True, ""), (False, output), + or (None, why) when the check couldn't run.""" + try: + proc = await asyncio.create_subprocess_exec( + "ansible-playbook", "--syntax-check", "-i", "localhost,", relative_path, + stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.STDOUT, + cwd=source_path, env=ansible_env({}, os.environ), start_new_session=True) + except FileNotFoundError: + return None, "ansible-playbook isn't installed here, so only the YAML was checked." + try: + out, _ = await asyncio.wait_for(proc.communicate(), SYNTAX_CHECK_SECONDS) + except asyncio.TimeoutError: + await _terminate_then_kill(proc, grace=2.0) + return None, f"The syntax check took over {SYNTAX_CHECK_SECONDS} s and was stopped." + text = out.decode(errors="replace").strip() + if proc.returncode == 0: + return True, "" + return False, text[-SYNTAX_OUTPUT_CHARS:] + + # ── Structured results (parsed from the streamed output) ────────────────────── _RECAP_RE = re.compile( diff --git a/steward/ansible/sources.py b/steward/ansible/sources.py index cd6e910..75c3253 100644 --- a/steward/ansible/sources.py +++ b/steward/ansible/sources.py @@ -212,7 +212,7 @@ def read_playbook(source_path: str, relative_path: str) -> str | None: return target.read_text(errors="replace") -def _resolve_writable(source_path: str, relative_path: str) -> tuple[Path | None, str | None]: +def resolve_writable(source_path: str, relative_path: str) -> tuple[Path | None, str | None]: """Resolve relative_path under source_path, guarding traversal + extension. Returns (target_path, None) on success or (None, error). The root need not @@ -233,7 +233,7 @@ def _resolve_writable(source_path: str, relative_path: str) -> tuple[Path | None def write_playbook(source_path: str, relative_path: str, content: str) -> tuple[bool, str | None]: """Write a playbook into a source, creating parent dirs. Traversal-guarded.""" - target, err = _resolve_writable(source_path, relative_path) + target, err = resolve_writable(source_path, relative_path) if err: return False, err try: @@ -246,7 +246,7 @@ def write_playbook(source_path: str, relative_path: str, content: str) -> tuple[ def delete_playbook(source_path: str, relative_path: str) -> tuple[bool, str | None]: """Delete a playbook from a source. Traversal-guarded; no-op if absent.""" - target, err = _resolve_writable(source_path, relative_path) + target, err = resolve_writable(source_path, relative_path) if err: return False, err try: diff --git a/steward/mcp/tools/automation.py b/steward/mcp/tools/automation.py index 0ca8fbb..452da19 100644 --- a/steward/mcp/tools/automation.py +++ b/steward/mcp/tools/automation.py @@ -229,6 +229,88 @@ async def get_playbook(source: str, playbook: str) -> dict: return out +def _editable(name: str) -> dict: + s = _source(name) + if not src_module.is_editable_source(s): + why = ("it holds Steward's own playbooks, which are changed in Steward's repository" + if s["name"] == src_module.BUILTIN_SOURCE_NAME else + "it is read from a git repository, which would overwrite a change on its next pull" + if s["type"] == "git" else "it is not editable") + raise ValueError(f"source {s['name']!r} can't be written here: {why}. Write this " + f"instance's own playbooks into {src_module.LOCAL_SOURCE_NAME!r}.") + return s + + +async def save_playbook(playbook: str, content: str, source: str = src_module.LOCAL_SOURCE_NAME, + replace: bool = False) -> dict: + """Write a playbook of this instance's own: automation for these hosts that + isn't part of Steward itself. The same as saving in Steward's editor. + + playbook: a path in the source ending .yml or .yaml, e.g. "upkeep/prune-runner.yml". + content: the whole playbook. It must parse as YAML and be a list of plays, + or nothing is saved and the error (with its line) comes back. + source: "steward-local" (default), or another editable folder source. + steward-builtin is refused: Steward's own playbooks are changed in + Steward's repository. Git sources are refused: their repo owns them. + replace: an existing playbook is overwritten only with replace=true, so + a typo in a name never clobbers another one. + + After saving, `ansible-playbook --syntax-check` runs on it (no host is + contacted); `syntax` says whether it passed and, if not, why, e.g. a + module name Ansible can't resolve. The file is saved either way. + + To test it: run_playbook(source, playbook, scope="target:") runs + it in check mode, get_ansible_run shows what would change; fix, save with + replace=true, run again; apply with check=false once it is right. + + Admin only. Recorded in the audit log (its size, never its content). + """ + s = _editable(source) + path = (playbook or "").strip().lstrip("/") + target, err = src_module.resolve_writable(s["path"], path) + if err: + raise ValueError(f"{err}. Nothing was saved.") + ok, err = src_module.validate_playbook_yaml(content or "") + if not ok: + raise ValueError(f"{err}\nNothing was saved.") + existed = target.exists() + if existed and not replace: + raise ValueError(f"{s['name']} already has {path!r}; pass replace=true to overwrite it, " + "or choose another name. Nothing was saved.") + ok, err = src_module.write_playbook(s["path"], path, content) + if not ok: + raise ValueError(f"{err}. Nothing was saved.") + await audit_mcp("ansible.playbook.updated" if existed else "ansible.playbook.created", + "ansible_playbook", f"{s['name']}:{path}", + {"source": s["name"], "playbook": path, "bytes": len(content)}) + passed, output = await executor.syntax_check(s["path"], path) + syntax = {"ok": passed} + if output: + syntax["output" if passed is False else "note"] = output + return {"source": s["name"], "playbook": path, "replaced": existed, "bytes": len(content), + "syntax": syntax, + "next": f'run_playbook(source="{s["name"]}", playbook="{path}", ' + 'scope="target:") to see what it would change.'} + + +async def delete_playbook(playbook: str, source: str = src_module.LOCAL_SOURCE_NAME) -> dict: + """Delete a playbook written into steward-local (or another editable folder + source). Its past runs stay. A schedule or alert action that names it + will fail until it is pointed elsewhere. + + Admin only. Recorded in the audit log. + """ + s = _editable(source) + path = (playbook or "").strip().lstrip("/") + _playbook(s, path) + ok, err = src_module.delete_playbook(s["path"], path) + if not ok: + raise ValueError(err) + await audit_mcp("ansible.playbook.deleted", "ansible_playbook", f"{s['name']}:{path}", + {"source": s["name"], "playbook": path}) + return {"deleted": path, "source": s["name"]} + + async def list_inventory() -> dict: """Steward's Ansible inventory: targets, groups, and how to scope a run to them. @@ -787,4 +869,5 @@ def register(register_tool) -> None: for fn in (run_playbook, cancel_ansible_run, create_schedule, update_schedule, delete_schedule, update_group_members): register_tool(fn, role=UserRole.operator, read_only=False) - register_tool(set_alert_action, role=UserRole.admin, read_only=False) + for fn in (set_alert_action, save_playbook, delete_playbook): + register_tool(fn, role=UserRole.admin, read_only=False) diff --git a/tests/integration/test_mcp_automation.py b/tests/integration/test_mcp_automation.py index 845c041..2247828 100644 --- a/tests/integration/test_mcp_automation.py +++ b/tests/integration/test_mcp_automation.py @@ -197,3 +197,72 @@ def test_runs_schedules_and_alert_actions_through_mcp(app, tmp_path): assert len(await _audit("alert_rule.action_cleared", rule_name)) == 1 asyncio.run(_go()) + + +@_NEEDS_DB +def test_an_instance_playbook_is_written_checked_run_and_removed_through_mcp(app, tmp_path, monkeypatch): + """#5748: save_playbook into steward-local, the real syntax check, a + check-mode run of what was saved, replace, delete; audit rows carry the + size and never the content.""" + from sqlalchemy import select + from steward.ansible import sources as src_module + from steward.core.api_tokens import Caller + from steward.mcp import _context + from steward.mcp.tools import automation as a + from steward.models.ansible_inventory import AnsibleTarget + from steward.models.audit import AuditEvent + from steward.models.users import User, UserRole + + monkeypatch.setattr(src_module, "USER_PLAYBOOK_DIR", str(tmp_path / "local")) + tag = uuid.uuid4().hex[:8] + uid, username, target_name = str(uuid.uuid4()), f"mcp-pb-{tag}", f"mcp-pb-target-{tag}" + marker = "MARKER-" + tag + good = textwrap.dedent(f"""\ + - hosts: all + connection: local + gather_facts: false + tasks: + - ansible.builtin.debug: + msg: "{marker}" + """) + typo = good.replace("ansible.builtin.debug", "ansible.builtin.debugg") + + async def _go(): + async with app.db_sessionmaker() as s: + async with s.begin(): + s.add(User(id=uid, username=username, email=f"{tag}@e.test", + password_hash="x", role=UserRole.admin, is_active=True)) + s.add(AnsibleTarget(name=target_name, address="127.0.0.1", + ansible_vars={"ansible_connection": "local"})) + _context._app_ctx.set(app) + _context._caller_ctx.set(Caller(user_id=uid, username=username, + role=UserRole.admin, token_id=7)) + + bad = await a.save_playbook("upkeep/check.yml", typo) + assert bad["syntax"]["ok"] is False and "debugg" in bad["syntax"]["output"] + fixed = await a.save_playbook("upkeep/check.yml", good, replace=True) + assert fixed["syntax"] == {"ok": True} and fixed["replaced"] is True + + started = await a.run_playbook("steward-local", "upkeep/check.yml", + scope=f"target:{target_name}") + assert started["check"] is True + done = await a.wait_for_run(started["run_id"], timeout_s=120) + assert done["status"] == "success", done["tail"] + assert marker in "\n".join(done["tail"]) + + assert await a.delete_playbook("upkeep/check.yml") == { + "deleted": "upkeep/check.yml", "source": "steward-local"} + assert "upkeep/check.yml" not in [ + b["playbook"] for b in (await a.list_playbooks(source="steward-local"))["playbooks"]] + + async with app.db_sessionmaker() as s: + rows = list((await s.execute(select(AuditEvent).where( + AuditEvent.user_id == uid, AuditEvent.action.like("ansible.playbook.%")) + .order_by(AuditEvent.timestamp))).scalars()) + assert [r.action for r in rows] == ["ansible.playbook.created", "ansible.playbook.updated", + "ansible.playbook.deleted"] + assert all(r.username == f"{username} via MCP" for r in rows) + assert json.loads(rows[1].detail_json)["bytes"] == len(good) + assert not any(marker in (r.detail_json or "") for r in rows) + + asyncio.run(_go()) diff --git a/tests/mcp/test_automation.py b/tests/mcp/test_automation.py index 8ce64e1..60a1447 100644 --- a/tests/mcp/test_automation.py +++ b/tests/mcp/test_automation.py @@ -25,6 +25,7 @@ VIEWER_TOOLS = {"list_playbooks", "get_playbook", "list_inventory", "list_ansibl "get_ansible_run", "list_schedules", "wait_for_run"} OPERATOR_TOOLS = {"run_playbook", "cancel_ansible_run", "create_schedule", "update_schedule", "delete_schedule", "update_group_members"} +ADMIN_TOOLS = {"set_alert_action", "save_playbook", "delete_playbook"} def test_split_vars_keeps_secrets_apart_from_what_is_stored(): @@ -96,8 +97,10 @@ def test_automation_tools_are_registered_with_their_roles(app): for name in OPERATOR_TOOLS: assert mcp.tool_roles[name] is UserRole.operator, name assert mcp._tool_manager.get_tool(name).annotations.read_only_hint is False, name - assert mcp.tool_roles["set_alert_action"] is UserRole.admin - for name in VIEWER_TOOLS | OPERATOR_TOOLS | {"set_alert_action"}: + for name in ADMIN_TOOLS: + assert mcp.tool_roles[name] is UserRole.admin, name + assert mcp._tool_manager.get_tool(name).annotations.read_only_hint is False, name + for name in VIEWER_TOOLS | OPERATOR_TOOLS | ADMIN_TOOLS: assert (mcp._tool_manager.get_tool(name).description or "").strip(), name @@ -107,6 +110,9 @@ def test_a_viewer_token_cannot_start_a_run(app): assert "needs the operator role" in app.mcp_server.refusal("run_playbook", viewer) assert app.mcp_server.refusal("run_playbook", operator) is None assert "needs the admin role" in app.mcp_server.refusal("set_alert_action", operator) + for name in ("save_playbook", "delete_playbook"): + assert "needs the admin role" in app.mcp_server.refusal(name, operator) + assert "needs the admin role" in app.mcp_server.refusal(name, viewer) def test_builtin_playbooks_get_steward_url_from_steward(): @@ -168,3 +174,97 @@ def test_group_members_are_named_by_target_or_by_linked_host(): assert [t.name for t in got] == ["web01", "db01"] with pytest.raises(ValueError, match="no target or linked host named 'nas'.*Nothing was changed"): automation.member_targets(["web01", "nas"], [web, db], hosts) + + +# ── writing this instance's own playbooks (#5748) ─────────────────────────── + +PLAY = "- hosts: all\n gather_facts: false\n tasks:\n - ansible.builtin.ping:\n" + + +@pytest.fixture +def writable(monkeypatch, tmp_path): + """steward-local in a temp folder beside the read-only sources, with the + audit rows and the syntax check captured.""" + from steward.ansible import sources as src_module + local = tmp_path / "local" + srcs = [{"name": src_module.BUILTIN_SOURCE_NAME, "type": "local", "path": str(tmp_path / "builtin")}, + {"name": src_module.LOCAL_SOURCE_NAME, "type": "local", "path": str(local)}, + {"name": "infra", "type": "git", "path": str(tmp_path / "git")}] + monkeypatch.setattr(automation, "_sources", lambda: srcs) + seen = {"audits": [], "checked": []} + + async def audit(action, entity_type=None, entity_id=None, detail=None): + seen["audits"].append((action, entity_id, detail)) + + async def syntax(path, rel): + seen["checked"].append(rel) + return seen.get("syntax", (True, "")) + monkeypatch.setattr(automation, "audit_mcp", audit) + monkeypatch.setattr(automation.executor, "syntax_check", syntax) + seen["dir"] = local + return seen + + +async def test_a_new_playbook_is_written_checked_and_audited_without_its_content(writable): + out = await automation.save_playbook("upkeep/ping.yml", PLAY) + assert (writable["dir"] / "upkeep/ping.yml").read_text() == PLAY + assert out["source"] == "steward-local" and out["replaced"] is False + assert out["syntax"] == {"ok": True} and writable["checked"] == ["upkeep/ping.yml"] + [(action, entity, detail)] = writable["audits"] + assert action == "ansible.playbook.created" and entity == "steward-local:upkeep/ping.yml" + assert detail == {"source": "steward-local", "playbook": "upkeep/ping.yml", "bytes": len(PLAY)} + assert PLAY not in str(writable["audits"]) + + +async def test_an_existing_playbook_is_replaced_only_when_asked(writable): + await automation.save_playbook("ping.yml", PLAY) + with pytest.raises(ValueError, match="already has 'ping.yml'.*replace=true.*Nothing was saved"): + await automation.save_playbook("ping.yml", "- hosts: web\n tasks: []\n") + assert (writable["dir"] / "ping.yml").read_text() == PLAY + out = await automation.save_playbook("ping.yml", "- hosts: web\n tasks: []\n", replace=True) + assert out["replaced"] is True + assert [a for a, _, _ in writable["audits"]] == ["ansible.playbook.created", "ansible.playbook.updated"] + + +@pytest.mark.parametrize("content, why", [ + ("- hosts: all\n tasks: [unclosed\n", "(?s)YAML error.*line \\d"), + ("hosts: all\n", "list of plays"), + ("", "empty"), +]) +async def test_a_playbook_that_isnt_one_says_why_and_writes_nothing(writable, content, why): + with pytest.raises(ValueError, match=why): + await automation.save_playbook("bad.yml", content) + assert not (writable["dir"] / "bad.yml").exists() + assert writable["audits"] == [] and writable["checked"] == [] + + +async def test_stewards_own_and_git_sources_are_refused(writable): + with pytest.raises(ValueError, match="Steward's own playbooks.*repository.*steward-local"): + await automation.save_playbook("x.yml", PLAY, source="steward-builtin") + with pytest.raises(ValueError, match="git repository"): + await automation.save_playbook("x.yml", PLAY, source="infra") + with pytest.raises(ValueError, match="Steward's own playbooks"): + await automation.delete_playbook("x.yml", source="steward-builtin") + for bad in ("../escape.yml", "notes.txt"): + with pytest.raises(ValueError, match="Nothing was saved"): + await automation.save_playbook(bad, PLAY) + assert writable["audits"] == [] + + +async def test_a_failed_syntax_check_is_reported_and_the_file_kept(writable): + writable["syntax"] = (False, "ERROR! couldn't resolve module/action 'ansible.builtin.pingg'") + out = await automation.save_playbook("ping.yml", PLAY) + assert out["syntax"] == {"ok": False, "output": writable["syntax"][1]} + assert (writable["dir"] / "ping.yml").exists() + writable["syntax"] = (None, "ansible-playbook isn't installed here, so only the YAML was checked.") + out = await automation.save_playbook("ping.yml", PLAY, replace=True) + assert out["syntax"] == {"ok": None, "note": writable["syntax"][1]} + + +async def test_a_playbook_is_deleted_and_audited(writable): + await automation.save_playbook("ping.yml", PLAY) + assert await automation.delete_playbook("ping.yml") == {"deleted": "ping.yml", "source": "steward-local"} + assert not (writable["dir"] / "ping.yml").exists() + assert writable["audits"][-1][:2] == ("ansible.playbook.deleted", "steward-local:ping.yml") + with pytest.raises(ValueError, match="has no playbook 'ping.yml'"): + await automation.delete_playbook("ping.yml")