fix(ansible): git sources work in the image and survive a redeploy; pulls time out; alert vars go in a JSON file (#5157)

- git ships in the image; checkouts live under /data/ansible/sources
  (STEWARD_ANSIBLE_SOURCE_DIR), on the persistent volume.
- One task, ansible_git_pulls, reads the sources every 30 s and pulls each
  as its interval passes: a source added in Settings is pulled without a
  restart, a removed one is forgotten.
- git_pull: a 120 s timeout ending the process group, GIT_TERMINAL_PROMPT=0,
  pull --ff-only, a failed clone removed; it raises GitPullError with git's
  message, so the Sync button stops saying Synced after a failure (and the
  message is now escaped).
- An alert action's vars and the firing's steward_alert_* vars reach
  ansible-playbook as one JSON extra-vars file, not -e key=value, which
  Ansible split on whitespace (a rule named 'High CPU on web').

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Dockerfile

@@ -10,6 +10,8 @@ RUN DEBIAN_FRONTEND=noninteractive apt-get update \
         # sshpass — Ansible shells out to it for first-contact password SSH
         # (provisioning a fresh host before the managed key is installed)
         sshpass \
+        # git — clones and pulls Ansible git sources (Settings > Ansible)
+        git \
     && rm -rf /var/lib/apt/lists/*
 
 # Upgrade pip to suppress the version notice
@@ -37,7 +39,7 @@ RUN chmod +x /entrypoint.sh
 # so the entrypoint can fix up /data permissions if needed, then drops to
 # 'app' (uid 1000) via gosu before launching steward.
 RUN useradd -m -u 1000 app \
-    && mkdir -p /data/playbook_cache /data/plugins \
+    && mkdir -p /data/ansible/sources /data/ansible/runs /data/ansible/playbooks /data/plugins \
     && chown -R app:app /data /app
 
 EXPOSE 5000

docs/core/ansible.md

@@ -31,10 +31,9 @@ ansible.sources:
     url: "https://github.com/example/infra.git"
     branch: "main"
     pull_interval_seconds: 300
-    cache_path: "/data/playbook_cache/infra-repo"
 ```
 
-Git sources register a `ScheduledTask` named `ansible_git_pull_<name>` that runs `git pull` on the configured interval.
+Each git source is checked out under `/data/ansible/sources/<name>` (override with `STEWARD_ANSIBLE_SOURCE_DIR`), on the persistent volume, so a redeploy keeps it. One scheduled task, `ansible_git_pulls`, ticks every 30 s and pulls each source once its `pull_interval_seconds` has passed; a source added in Settings is pulled within a tick, with no restart. A clone or pull that runs past 120 s is cut (its process group terminated), a clone that fails is removed so the next attempt starts clean, and git never prompts for credentials.
 
 ---
 

steward/ansible/sources.py

@@ -3,8 +3,10 @@ import asyncio
 import logging
 import os
 import re
+import shutil
 import stat
 import tempfile
+import time
 from pathlib import Path
 
 logger = logging.getLogger(__name__)
@@ -76,6 +78,9 @@ BUILTIN_SOURCE_NAME = "steward-builtin"
 # persistent /data volume; created lazily on first save.
 LOCAL_SOURCE_NAME = "steward-local"
 USER_PLAYBOOK_DIR = os.environ.get("STEWARD_PLAYBOOK_DIR", "/data/ansible/playbooks")
+# Git sources are checked out here, on the persistent /data volume, so a
+# redeploy keeps them instead of re-cloning into the container's layer (#5157).
+SOURCE_CACHE_DIR = os.environ.get("STEWARD_ANSIBLE_SOURCE_DIR", "/data/ansible/sources")
 
 
 def _builtin_source() -> dict:
@@ -124,7 +129,7 @@ def get_sources(ansible_cfg: dict) -> list[dict]:
     Config structure in config.yaml::
 
         ansible:
-          cache_dir: /var/cache/steward/ansible
+          cache_dir: /data/ansible/sources
           sources:
             - name: my-playbooks
               type: local
@@ -136,7 +141,7 @@ def get_sources(ansible_cfg: dict) -> list[dict]:
               pull_interval_seconds: 3600
     """
     sources = ansible_cfg.get("sources", [])
-    cache_dir = ansible_cfg.get("cache_dir", "/var/cache/steward/ansible")
+    cache_dir = ansible_cfg.get("cache_dir") or SOURCE_CACHE_DIR
     # Always expose the bundled (read-only) + local (writable) first-party sources.
     result = [_builtin_source(), _local_source()]
     for src in sources:
@@ -343,8 +348,43 @@ ASKPASS_SCRIPT = (
 )
 
 
-async def git_pull(source: dict) -> None:
-    """Clone the git repo if absent; pull if already present.
+# A clone or pull past this is cut, so a hung remote can't hold a source's
+# pull (or the Sync button) open forever (#5157).
+GIT_TIMEOUT_SECONDS = 120.0
+
+
+class GitPullError(RuntimeError):
+    """A clone or pull that failed or ran past its timeout; says which and why."""
+
+
+async def _git(argv: list[str], env: dict, timeout: float, what: str) -> None:
+    from steward.ansible.executor import _terminate_then_kill
+
+    proc = await asyncio.create_subprocess_exec(
+        *argv,
+        stdout=asyncio.subprocess.PIPE,
+        stderr=asyncio.subprocess.PIPE,
+        env=env,
+        start_new_session=True,   # its own group, so a timeout ends ssh/helpers too
+    )
+    try:
+        _, stderr = await asyncio.wait_for(proc.communicate(), timeout)
+    except asyncio.TimeoutError:
+        raise GitPullError(f"{what} did not finish within {timeout:g} s") from None
+    finally:
+        if proc.returncode is None:
+            await asyncio.shield(_terminate_then_kill(proc, grace=2.0))
+    if proc.returncode != 0:
+        raise GitPullError(f"{what} failed: {stderr.decode(errors='replace').strip()[-500:]}")
+
+
+async def git_pull(source: dict, timeout: float = GIT_TIMEOUT_SECONDS) -> None:
+    """Clone the git repo if absent; fast-forward it if already present.
+
+    Raises GitPullError when git fails or runs past `timeout`; a clone that
+    did not finish is removed, so the next attempt clones afresh rather than
+    pulling into a broken checkout. git never prompts (GIT_TERMINAL_PROMPT=0):
+    a missing credential fails at once instead of waiting on input.
 
     When source['http_token'] is set, git asks a temporary GIT_ASKPASS script,
     which prints the token from the child's environment, so it never touches
@@ -353,7 +393,7 @@ async def git_pull(source: dict) -> None:
     path = Path(source["path"])
     token = source.get("http_token", "")
 
-    env = None
+    env = {**os.environ, "GIT_TERMINAL_PROMPT": "0"}
     askpass_path = None
     if token:
         fd, askpass_path = tempfile.mkstemp(prefix="steward-askpass-", suffix=".sh")
@@ -362,41 +402,50 @@ async def git_pull(source: dict) -> None:
         finally:
             os.close(fd)
         os.chmod(askpass_path, stat.S_IRWXU)
-        env = {**os.environ, "GIT_ASKPASS": askpass_path, ASKPASS_TOKEN_ENV: token}
+        env.update({"GIT_ASKPASS": askpass_path, ASKPASS_TOKEN_ENV: token})
 
     try:
         if not (path / ".git").exists():
             path.mkdir(parents=True, exist_ok=True)
-            proc = await asyncio.create_subprocess_exec(
-                "git", "clone",
-                "--branch", source["branch"],
-                "--single-branch",
-                source["url"], str(path),
-                stdout=asyncio.subprocess.PIPE,
-                stderr=asyncio.subprocess.PIPE,
-                env=env,
-            )
-            _, stderr = await proc.communicate()
-            if proc.returncode != 0:
-                logger.error(
-                    "git clone failed for %r: %s",
-                    source["name"],
-                    stderr.decode(errors="replace"),
-                )
+            try:
+                await _git(["git", "clone", "--branch", source["branch"], "--single-branch",
+                            source["url"], str(path)],
+                           env, timeout, f"git clone of {source['name']!r}")
+            except BaseException:
+                shutil.rmtree(path, ignore_errors=True)
+                raise
         else:
-            proc = await asyncio.create_subprocess_exec(
-                "git", "-C", str(path), "pull",
-                stdout=asyncio.subprocess.PIPE,
-                stderr=asyncio.subprocess.PIPE,
-                env=env,
-            )
-            _, stderr = await proc.communicate()
-            if proc.returncode != 0:
-                logger.error(
-                    "git pull failed for %r: %s",
-                    source["name"],
-                    stderr.decode(errors="replace"),
-                )
+            await _git(["git", "-C", str(path), "pull", "--ff-only"],
+                       env, timeout, f"git pull of {source['name']!r}")
     finally:
         if askpass_path and os.path.exists(askpass_path):
             os.unlink(askpass_path)
+
+
+# When each git source was last pulled (monotonic seconds), by name.
+_last_pull: dict[str, float] = {}
+
+
+async def pull_due_sources(ansible_cfg: dict, now: float | None = None) -> list[str]:
+    """Pull every git source whose pull_interval_seconds has passed.
+
+    Run by one scheduled task on a short tick, reading the sources as they are
+    now, so a source added in Settings is pulled within a tick and an edited
+    interval applies without a restart (#5157). A failed pull is logged and
+    waits out its interval like a good one. Returns the names pulled.
+    """
+    now = time.monotonic() if now is None else now
+    sources = get_sources(ansible_cfg)
+    due = [src for src in sources
+           if src["type"] == "git"
+           and now - _last_pull.get(src["name"], float("-inf")) >= src["pull_interval_seconds"]]
+    names = {src["name"] for src in sources}
+    for gone in [n for n in _last_pull if n not in names]:
+        del _last_pull[gone]
+    for src in due:
+        _last_pull[src["name"]] = now
+        try:
+            await git_pull(src)
+        except Exception:
+            logger.exception("Ansible source %r: pull failed", src["name"])
+    return [src["name"] for src in due]

steward/app.py

@@ -434,26 +434,21 @@ def _register_core_tasks(app: Quart) -> None:
         ),
     ])
 
-    # Register git-pull tasks for each ansible git source
-    ansible_cfg = app.config.get("ANSIBLE", {})
-    from .ansible.sources import get_sources
-    for source in get_sources(ansible_cfg):
-        if source["type"] != "git":
-            continue
-        _source = source
+    # Pull Ansible git sources as each falls due. One task reads the sources
+    # as they are on every tick, so one added in Settings is pulled within
+    # half a minute, no restart (#5157).
+    async def run_git_pulls():
+        from .ansible.sources import pull_due_sources
+        await pull_due_sources(app.config.get("ANSIBLE", {}))
 
-        async def run_git_pull(src=_source):
-            from .ansible.sources import git_pull
-            await git_pull(src)
-
-        app._task_registry.append(
-            ScheduledTask(
-                name=f"ansible_git_pull_{_source['name']}",
-                coro_factory=run_git_pull,
-                interval_seconds=_source["pull_interval_seconds"],
-                run_on_startup=True,
-            )
+    app._task_registry.append(
+        ScheduledTask(
+            name="ansible_git_pulls",
+            coro_factory=run_git_pulls,
+            interval_seconds=30,
+            run_on_startup=True,
         )
+    )
 
     # Fire due Ansible schedules (recurring playbook runs). Checks every minute;
     # each schedule's own interval gates whether it actually runs.

steward/core/alerts.py

@@ -34,21 +34,35 @@ def init_alerts(app: "Quart") -> None:
     _app = app
 
 
-def alert_extra_vars(rule: AlertRule, value: float) -> list[str]:
-    """Ansible extra-vars describing the alert that fired, as `key=value` strings.
+def alert_extra_vars(rule: AlertRule, value: float) -> dict:
+    """Ansible extra-vars describing the alert that fired.
 
     Injected into every alert-triggered playbook run so the playbook can target
-    the exact resource/metric that fired without templating. Pure function.
+    the exact resource/metric that fired without templating. They reach
+    ansible-playbook in its JSON extra-vars file, not as `-e key=value`, which
+    Ansible splits on whitespace: a rule named "High CPU on web" stays one
+    value (#5157). Pure function.
     """
-    return [
-        f"steward_alert_rule={rule.name}",
-        f"steward_alert_source={rule.source_module}",
-        f"steward_alert_resource={rule.resource_name}",
-        f"steward_alert_metric={rule.metric_name}",
-        f"steward_alert_value={value}",
-        f"steward_alert_threshold={rule.threshold}",
-        f"steward_alert_operator={rule.operator.value}",
-    ]
+    return {
+        "steward_alert_rule": rule.name,
+        "steward_alert_source": rule.source_module,
+        "steward_alert_resource": rule.resource_name,
+        "steward_alert_metric": rule.metric_name,
+        "steward_alert_value": value,
+        "steward_alert_threshold": rule.threshold,
+        "steward_alert_operator": rule.operator.value,
+    }
+
+
+def action_vars(lines) -> dict:
+    """An alert action's stored `key=value` lines as a map, split at the first
+    `=` so a value keeps its spaces, quotes and further `=`."""
+    out: dict = {}
+    for line in lines or ():
+        key, sep, val = str(line).partition("=")
+        if sep and key.strip():
+            out[key.strip()] = val
+    return out
 
 
 async def record_metric(
@@ -489,8 +503,10 @@ async def _run_ansible_action(
     playbook = action.get("playbook") or ""
     inventory = action.get("inventory") or ""
     source_name = action.get("source") or ""
+    # One JSON extra-vars file; the firing's own vars win over the action's.
     params: dict = {
-        "extra_vars": list(action.get("extra_vars") or []) + alert_extra_vars(rule, value),
+        "extra_vars_map": {**action_vars(action.get("extra_vars")),
+                           **alert_extra_vars(rule, value)},
     }
     if action.get("limit"):
         params["limit"] = action["limit"]

steward/settings/routes.py

@@ -1,5 +1,6 @@
 # steward/settings/routes.py
 from __future__ import annotations
+import html
 import logging
 from pathlib import Path
 from quart import Blueprint, current_app, render_template, request, redirect, url_for, session
@@ -505,7 +506,8 @@ async def ansible_sync_source(idx: int):
         return '<span style="color:var(--green);font-size:0.8rem;">Synced</span>'
     except Exception as exc:
         logger.exception("git pull failed for source %r", source["name"])
-        return f'<span style="color:var(--red);font-size:0.8rem;">Sync failed: {exc}</span>'
+        return ('<span style="color:var(--red);font-size:0.8rem;">Sync failed: '
+                f'{html.escape(str(exc))}</span>')
 
 
 # ── Auth (OIDC / LDAP) ────────────────────────────────────────────────────────

tests/core/test_git_sources.py

@@ -0,0 +1,102 @@
+"""Git sources: the checkout location, a pull cut at its timeout, a failure that
+says why, and one pull task that picks up sources as they change (#5157)."""
+import os
+import time
+from unittest.mock import AsyncMock, MagicMock, patch
+
+import pytest
+
+from steward.ansible import sources
+
+
+def _fake_git(tmp_path, monkeypatch, body: str) -> None:
+    """Put a `git` on PATH that runs `body` instead of the real one."""
+    bindir = tmp_path / "bin"
+    bindir.mkdir()
+    git = bindir / "git"
+    git.write_text("#!/bin/sh\n" + body + "\n")
+    git.chmod(0o755)
+    monkeypatch.setenv("PATH", f"{bindir}{os.pathsep}{os.environ.get('PATH', '')}")
+
+
+def _source(path, **kw):
+    return {"name": "infra", "path": str(path), "url": "https://git.example/infra.git",
+            "branch": "main", "http_token": "", **kw}
+
+
+def test_a_git_source_is_checked_out_under_data():
+    src = next(s for s in sources.get_sources(
+        {"sources": [{"name": "infra", "type": "git", "url": "https://x/infra.git"}]})
+        if s["name"] == "infra")
+    assert src["path"] == os.path.join(sources.SOURCE_CACHE_DIR, "infra")
+    # On the persistent volume, not the container's layer.
+    assert sources.SOURCE_CACHE_DIR == os.environ.get(
+        "STEWARD_ANSIBLE_SOURCE_DIR", "/data/ansible/sources")
+
+
+@pytest.mark.asyncio
+async def test_a_hung_clone_is_cut_at_its_timeout_and_removed(tmp_path, monkeypatch):
+    _fake_git(tmp_path, monkeypatch, "sleep 30")
+    checkout = tmp_path / "cache" / "infra"
+    started = time.monotonic()
+    with pytest.raises(sources.GitPullError, match="did not finish within 1 s"):
+        await sources.git_pull(_source(checkout), timeout=1)
+    assert time.monotonic() - started < 10
+    assert not checkout.exists()           # the next attempt clones afresh
+
+
+@pytest.mark.asyncio
+async def test_a_failed_pull_says_why_and_keeps_the_checkout(tmp_path, monkeypatch):
+    _fake_git(tmp_path, monkeypatch, "echo 'fatal: Not possible to fast-forward' >&2; exit 128")
+    checkout = tmp_path / "infra"
+    (checkout / ".git").mkdir(parents=True)
+    with pytest.raises(sources.GitPullError, match="Not possible to fast-forward"):
+        await sources.git_pull(_source(checkout))
+    assert (checkout / ".git").exists()
+
+
+@pytest.mark.asyncio
+async def test_git_never_prompts_and_only_fast_forwards(tmp_path):
+    checkout = tmp_path / "infra"
+    (checkout / ".git").mkdir(parents=True)
+    seen = {}
+
+    async def fake_exec(*args, **kwargs):
+        seen["argv"], seen["env"] = args, kwargs.get("env") or {}
+        proc = MagicMock()
+        proc.returncode = 0
+        proc.communicate = AsyncMock(return_value=(b"", b""))
+        return proc
+
+    with patch("asyncio.create_subprocess_exec", side_effect=fake_exec):
+        await sources.git_pull(_source(checkout))
+    assert seen["env"]["GIT_TERMINAL_PROMPT"] == "0"
+    assert seen["argv"][-2:] == ("pull", "--ff-only")
+
+
+@pytest.mark.asyncio
+async def test_one_task_pulls_sources_as_they_fall_due(monkeypatch):
+    pulled: list[str] = []
+
+    async def fake_pull(src):
+        pulled.append(src["name"])
+        if src["name"] == "broken":
+            raise sources.GitPullError("git pull of 'broken' failed: nope")
+
+    monkeypatch.setattr(sources, "git_pull", fake_pull)
+    monkeypatch.setattr(sources, "_last_pull", {})
+
+    def cfg(*names):
+        intervals = {"fast": 60, "slow": 600, "broken": 60, "new": 60}
+        return {"cache_dir": "/tmp/x", "sources": [
+            {"name": n, "type": "git", "url": f"https://x/{n}.git",
+             "pull_interval_seconds": intervals[n]} for n in names]}
+
+    assert await sources.pull_due_sources(cfg("fast", "slow", "broken"), now=1000) == [
+        "fast", "slow", "broken"]                        # all due at start; a failure is logged
+    assert await sources.pull_due_sources(cfg("fast", "slow", "broken"), now=1030) == []
+    assert await sources.pull_due_sources(cfg("fast", "slow", "broken"), now=1061) == [
+        "fast", "broken"]                                # a failed one waits its interval too
+    # A source added in Settings is pulled on the next tick, no restart.
+    assert await sources.pull_due_sources(cfg("fast", "slow", "new"), now=1062) == ["new"]
+    assert "broken" not in sources._last_pull            # a removed source is forgotten

tests/test_alert_ansible.py

@@ -1,5 +1,5 @@
-"""Unit tests for alert -> Ansible action context vars (task 250)."""
-from steward.core.alerts import alert_extra_vars
+"""Unit tests for alert -> Ansible action context vars (task 250, #5157)."""
+from steward.core.alerts import action_vars, alert_extra_vars
 from steward.models.alerts import AlertOperator, AlertRule
 
 
@@ -9,22 +9,33 @@ def test_alert_extra_vars_describes_the_firing():
         metric_name="battery_status", operator=AlertOperator.lt, threshold=3.0,
         created_by="u",
     )
-    ev = alert_extra_vars(rule, 2.0)
-    assert "steward_alert_rule=UPS low battery" in ev
-    assert "steward_alert_source=snmp" in ev
-    assert "steward_alert_resource=ups" in ev
-    assert "steward_alert_metric=battery_status" in ev
-    assert "steward_alert_value=2.0" in ev
-    assert "steward_alert_threshold=3.0" in ev
-    assert "steward_alert_operator=<" in ev
+    assert alert_extra_vars(rule, 2.0) == {
+        "steward_alert_rule": "UPS low battery",   # one value, spaces and all
+        "steward_alert_source": "snmp",
+        "steward_alert_resource": "ups",
+        "steward_alert_metric": "battery_status",
+        "steward_alert_value": 2.0,
+        "steward_alert_threshold": 3.0,
+        "steward_alert_operator": "<",
+    }
 
 
-def test_alert_extra_vars_are_key_value_strings():
-    rule = AlertRule(
-        name="r", source_module="host_agent", resource_name="srv1",
-        metric_name="cpu_pct", operator=AlertOperator.gt, threshold=90.0,
-        created_by="u",
-    )
-    ev = alert_extra_vars(rule, 95.5)
-    assert all("=" in item for item in ev)
-    assert all(item.startswith("steward_alert_") for item in ev)
+def test_action_vars_split_at_the_first_equals():
+    assert action_vars(["greeting=hello world", "q=a=b", " k = v", "bare", "=x"]) == {
+        "greeting": "hello world", "q": "a=b", "k": " v"}
+    assert action_vars(None) == {}
+
+
+def test_an_alert_run_passes_its_vars_as_a_json_file_not_on_the_command_line():
+    import json
+    from steward.ansible.executor import build_ansible_command, build_extra_vars_file
+    rule = AlertRule(name="High CPU on web", source_module="host_agent", resource_name="web",
+                     metric_name="cpu_pct", operator=AlertOperator.gt, threshold=90.0,
+                     created_by="u")
+    merged = {**action_vars(["note=from the action"]), **alert_extra_vars(rule, 95.5)}
+    cmd = build_ansible_command("p.yml", "inv", {"extra_vars_map": merged})
+    assert "-e" not in cmd                       # nothing interpolated into argv
+    argv, files = build_extra_vars_file(merged, "/tmp/x")
+    assert argv == ["-e", "@/tmp/x/extravars.json"]
+    assert json.loads(files[0][1])["steward_alert_rule"] == "High CPU on web"
+    assert json.loads(files[0][1])["note"] == "from the action"