Remove 'enroll diff --enforce' option. Tighten yaml data re: handlers - use listen: instead of notify:

CHANGELOG.md

@@ -1,5 +1,6 @@
 # 0.7.0 (unreleased)
 
+ * BREAKING CHANGE: Remove the `enroll diff --enforce` option. Enroll no longer applies the old harvest state locally to repair drift; this avoids the risk of enforcing a potentially malicious or tampered harvest. To restore baseline state, regenerate a manifest from the trusted harvest and apply it yourself, or compare two `enroll diff` runs and act on the result.
  * BREAKING CHANGE: Group all package and systemd-unit roles into Debian Section/RPM Group roles by default, including managed config files and unit state. This mode is not used if `--fqdn` or `--no-common-roles` is set, in which case, the traditional behaviour of preserving one role per package/unit is used instead.
  * BREAKING CHANGE: Only capture user-specific .bashrc style files when using `--dangerous` mode, in case they contain sensitive env vars.
  * BREAKING CHANGE: Don't allow reading `.enroll.ini` in the CWD. Use only the ENROLL_CONFIG env var, an explicit `--config` path or else the XDG default location (or `~/.config/enroll/enroll.ini` if `XDG_CONFIG_HOME` is not set).

DEVELOPMENT.md

@@ -29,14 +29,12 @@ Generated configuration-management output
 
 The harvest bundle is deliberately target-neutral. Ansible renderer consumes the same `state.json` shape and the same harvested artifacts. Renderer code should translate harvest state into the target's idioms; it should not invent source facts that belong in the harvest.
 
-`enroll diff` is also built around harvest bundles. It compares two harvests and, when `--enforce` is requested, can generate a temporary manifest from the old harvest and apply it locally with the selected target:
+`enroll diff` is also built around harvest bundles. It compares two harvests and reports drift between them:
 
 ```bash
-enroll diff --old ./baseline --new ./current --enforce
+enroll diff --old ./baseline --new ./current
 ```
 
-For enforcement, the user is responsible for having the chosen local apply tool on `PATH`: `ansible-playbook`.
-
 ---
 
 ## 2. Repository layout
@@ -82,7 +80,7 @@ enroll/
   yamlutil.py                 YAML helpers used by renderers/JinjaTurtle
   jinjaturtle.py              optional config-file templating integration
 
-  diff.py                     harvest comparison, notifications, and target-selected enforcement
+  diff.py                     harvest comparison and notifications
   explain.py                  human/JSON explanation of harvest contents
   validate.py                 schema and artifact consistency validation
   remote.py                   Paramiko remote harvest implementation
@@ -127,7 +125,7 @@ The supported subcommands are:
 harvest       collect a harvest bundle from a local or remote host
 manifest      generate Ansible output from a harvest bundle
 single-shot   run harvest and manifest in one command
-diff          compare two harvest bundles and optionally enforce old state
+diff          compare two harvest bundles and report drift
 explain       produce a human/JSON explanation of a harvest
 validate      validate state.json and referenced artifacts
 ```
@@ -148,10 +146,6 @@ flowchart TD
   D --> E
   B -->|diff| F[diff.compare_harvests]
   F --> G[diff.format_report]
-  F --> H{--enforce?}
-  H -->|yes| I[diff.enforce_old_harvest]
-  I --> J[manifest.manifest]
-  J --> K[ansible-playbook]
   B -->|explain| L[explain.explain_state]
   B -->|validate| M[validate.validate_harvest]
 ```
@@ -953,7 +947,7 @@ Ansible playbook roles are ordered intentionally:
 
 ### 13.4 Role tags
 
-Generated playbooks tag roles with `role_<safe_role_name>`. `diff --enforce --target ansible` uses these tags to narrow enforcement to roles relevant to the drift report when it can.
+Generated playbooks tag roles with `role_<safe_role_name>`, so operators can narrow a manual `ansible-playbook` run to specific roles with `--tags`.
 
 ### 13.5 Ansible and JinjaTurtle
 
@@ -1026,7 +1020,7 @@ When checks fail, Enroll deletes obsolete generated templates when appropriate a
 
 ---
 
-## 15. Diff, notifications, and enforcement
+## 15. Diff and notifications
 
 File: `diff.py`
 
@@ -1062,28 +1056,7 @@ Reports are formatted by:
 format_report(report, fmt="text" | "markdown" | "json")
 ```
 
-### 15.3 Enforcement decision
+### 15.3 Notifications
-
-`has_enforceable_drift()` is intentionally conservative.
-
-Enforceable drift includes:
-
-- packages that were removed from the current host but existed in the baseline,
-- baseline services that were removed or changed in meaningful non-package fields,
-- baseline users that were removed or changed,
-- baseline files that were removed or changed.
-
-Not enforceable:
-
-- newly installed packages,
-- package version changes alone,
-- newly enabled services,
-- newly added users,
-- newly added managed files.
-
-This keeps `--enforce` focused on restoring baseline state rather than deleting unknown current state or downgrading packages.
-
-### 15.4 Notifications
 
 `diff.py` also supports webhooks and email notifications:
 
@@ -1132,8 +1105,6 @@ This is intended to answer “what did Enroll collect and why?”
 - `manifest.manifest()` validates before rendering Ansible output.
 - `diff.compare_harvests()` validates both input bundles before comparing them, using `no_schema=True` so older harvests can still be inspected while artifact safety checks remain active.
 
-`diff --enforce` renders the old harvest through `manifest.manifest()`, so enforcement also passes through manifest-time validation before a local apply tool is invoked.
-
 It returns a `ValidationResult` with `errors`, `warnings`, `ok()`, `to_dict()`, and `to_text()`.
 
 The CLI supports local schema override with `--schema`, warning failure with `--fail-on-warnings`, JSON/text output, and `--out`.

README.md

@@ -172,27 +172,11 @@ Compare two harvest bundles and report what changed.
 - `--sops` when comparing SOPS-encrypted harvest bundles
 - `--exclude-path <PATTERN>` (repeatable) to ignore file/dir drift under matching paths (same pattern syntax as harvest)
 - `--ignore-package-versions` to ignore package version-only drift (upgrades/downgrades)
-- `--enforce` to apply the **old** harvest state locally (requires the relevant config manager tool on `PATH` - defaults to `ansible-playbook`)
-- `--enforce` runs `ansible-playbook` against the regenerated manifest)
 
 **Noise suppression**
 - `--exclude-path` is useful for things that change often but you still want in the harvest baseline (e.g. `/var/anacron`).
 - `--ignore-package-versions` keeps routine upgrades from alerting; package add/remove drift is still reported.
 
-**Enforcement (`--enforce`)**
-If a diff exists and `ansible-playbook` is on the PATH, Enroll will:
-1) generate a manifest from the **old** harvest into a temporary directory
-2) run the config manager tool against that manifest
-3) record in the diff report that the old harvest was enforced
-
-Enforcement is intentionally “safe”:
-- reinstalls packages that were removed (`state: present`), but does **not** attempt downgrades/pinning
-- restores users, files (contents + permissions/ownership), and service enable/start state
-
-If the config manager tool is not on `PATH`, Enroll returns an error and does not enforce.
-
-**IMPORTANT**: Only enforce harvest bundles that you trust. Validation checks bundle structure and artifact safety; it does not prove that the described system state is safe to apply, e.g. hasn't been tampered with by another user with sufficient permission to do so!
-
 
 **Output formats**
 - `--format json` (default for webhooks)
@@ -508,11 +492,6 @@ enroll diff --old /path/to/harvestA --new /path/to/harvestB --exclude-path /var/
 enroll diff --old /path/to/harvestA --new /path/to/harvestB --ignore-package-versions
 ```
 
-### Enforce the old harvest state when drift is detected
-```bash
-enroll diff --old /path/to/harvestA --new /path/to/harvestB --enforce --ignore-package-versions --exclude-path /var/anacron
-```
-
 ---
 
 ## Explain
@@ -648,7 +627,6 @@ sops = 54A91143AE0AB4F7743B01FE888ED1B423A3BC99
 # ignore noisy drift
 exclude_path = /var/anacron
 ignore_package_versions = true
-# enforce = true  # requires ansible-playbook on PATH
 
 [single-shot]
 # if you use single-shot, put its defaults here.

SECURITY.md

@@ -12,8 +12,8 @@ In particular:
 
 * If Enroll is run as root, the root user is assumed to control and understand the command line, environment, configuration file, and output location being used.
 * If an `enroll.ini` configuration file is loaded, its location and contents are assumed to be owned, selected, and understood by the operator.
-* The operator is expected to understand the implications of options such as `--dangerous`, `--assume-safe-path`, `--sops`, `--enforce`, `--remote-host`, and `--remote-ssh-config`.
+* The operator is expected to understand the implications of options such as `--dangerous`, `--assume-safe-path`, `--sops`, `--remote-host`, and `--remote-ssh-config`.
-* Harvest bundles used for `manifest`, `diff`, or `diff --enforce` are assumed to come from a trusted source unless the operator is deliberately inspecting untrusted input without applying it.
+* Harvest bundles used for `manifest` or `diff` are assumed to come from a trusted source unless the operator is deliberately inspecting untrusted input without applying it.
 * Configuration-management tools invoked by Enroll, such as Ansible, SOPS, SSH, `sudo`, Docker, Podman, Flatpak, Snap, package managers, and system utilities, are assumed to be the trusted tools the operator intended to use.
 
 ## What is in scope
@@ -44,22 +44,21 @@ The following are generally out of scope and should not be reported as Enroll vu
 * A root user loading an `enroll.ini` file whose contents intentionally request dangerous behavior.
 * A root user passing `--dangerous` and then observing that Enroll may collect sensitive information.
 * A root user passing `--assume-safe-path` and then observing that Enroll does not prompt about `PATH` safety.
-* A root user enforcing a malicious or manually edited harvest bundle with `diff --enforce`.
 * A user applying generated Ansible manifests from an untrusted harvest.
 * A user configuring a webhook, email target, SSH proxy command, SOPS binary, package manager, or configuration-management tool that they do not trust.
 * A compromised system where an attacker already controls root-owned files, root’s shell, root’s configuration, or the privileged tools Enroll invokes.
 * Reports that amount to “if root runs this tool with malicious options, root can make the system do dangerous things.”
-* Enroll harvesting a file that has a *commented out* secret even with `--dangerous` disabled (it ignores comments so as to not be totally useless when it comes to harvesting config files). It is still the responsibility of the user to use `--sops` or appropriate at-rest encryption if in the slightest doubt about what might get harvested.
+* Enroll harvesting a file that merely *mentions* a credential-related word in a comment with no assigned value (for example a commented-out `# token` hint in a stock config). Enroll tolerates value-less keyword mentions in comments so it is not useless for harvesting ordinary configuration files. However, a commented-out credential *value* — a populated `key = value` assignment, a URI with embedded credentials, an `Authorization` header, or private-key material — is treated as sensitive even inside a comment, because a "commented out" secret is very often a real secret that was merely disabled. Such a file is refused in default safe mode and requires `--dangerous` (ideally with `--sops`) to collect. It remains the responsibility of the user to use `--sops` or appropriate at-rest encryption if in the slightest doubt about what might get harvested.
 
 Enroll is a tool for administrators, not a sandbox for hostile local users. It cannot make unsafe local trust decisions safe if the operator’s own execution environment is already attacker-controlled.
 
-## Trusted harvests and enforcement
+## Trusted harvests
 
 Harvest bundles should be treated as sensitive and trusted administrative artifacts.
 
 A harvest may contain hostnames, usernames, package lists, service state, filesystem metadata, configuration files, firewall snapshots, container image references, Flatpak/Snap state, and other operational details. In `--dangerous` mode it may contain substantially more sensitive material.
 
-Before running `manifest`, `diff`, or especially `diff --enforce`, the operator should be confident that the harvest bundle came from a trusted source and has not been tampered with.
+Before running `manifest` or `diff`, or applying a generated manifest, the operator should be confident that the harvest bundle came from a trusted source and has not been tampered with.
 
 Enroll validates harvest structure and artifact safety. Validation can detect many unsafe filesystem constructs, such as path traversal, missing artifacts, symlinks, hardlinks, and schema mismatches. Validation does not and cannot prove that the desired state represented by a harvest is safe to apply.
 
@@ -91,7 +90,6 @@ Less useful reports, and normally out of scope, include:
 * “Root can pass `--dangerous` and collect dangerous data.”
 * “Root can pass `--assume-safe-path` and bypass the root `PATH` warning.”
 * “Root can point Enroll at a malicious config file.”
-* “Root can enforce a malicious harvest bundle.”
 * “A malicious local user can compromise Enroll after already controlling root’s environment or binaries.”
 
 Reports about concrete bypasses of Enroll's hardening are welcomed (see https://enroll.sh/security.html), but the project does not treat intentional administrator-controlled execution as a vulnerability.

enroll/ansible.py

@@ -18,7 +18,11 @@ from .manifest_safety import (
     iter_safe_artifact_files,
     prepare_manifest_output_dir,
 )
-from .render_safety import ansible_unsafe_data
+from .render_safety import (
+    ansible_unsafe_data,
+    assert_generated_yaml_safe,
+    scaffold_token,
+)
 from .role_names import avoid_reserved_role_name
 from .state import inventory_packages_from_state, roles_from_state
 from .yamlutil import yaml_dump_mapping, yaml_load_mapping
@@ -236,7 +240,7 @@ class AnsibleRole(CMModule):
         self.add_snapshot_notes(snap)
 
     def render_firewall_runtime_tasks(self) -> str:
-        var_prefix = self.role_name
+        var_prefix = scaffold_token(self.role_name, field="role var_prefix")
         return f"""- name: Ensure firewall runtime snapshot directory exists
   ansible.builtin.file:
     path: {self.firewall_runtime_dir}
@@ -292,7 +296,7 @@ class AnsibleRole(CMModule):
 """
 
     def render_firewall_runtime_handlers(self) -> str:
-        var_prefix = self.role_name
+        var_prefix = scaffold_token(self.role_name, field="role var_prefix")
         return f"""---
 - name: Flush captured ipsets before restoring members
   ansible.builtin.command:
@@ -512,7 +516,7 @@ def _write_role_scaffold(role_dir: str) -> None:
 def _role_tag(role: str) -> str:
     """Return a stable Ansible tag name for a role.
 
-    Used by `enroll diff --enforce` to run only the roles needed to repair drift.
+    Lets operators run only selected roles via `--tags` when applying a manifest.
     """
     r = str(role or "").strip()
     # Ansible tag charset is fairly permissive, but keep it portable and consistent.
@@ -532,13 +536,17 @@ def _write_playbook_all(path: str, roles: List[str]) -> None:
         "  roles:",
     ]
     for r in roles:
-        pb_lines.append(f"    - role: {r}")
+        safe = scaffold_token(r, field="role name")
-        pb_lines.append(f"      tags: [{_role_tag(r)}]")
+        pb_lines.append(f"    - role: {safe}")
+        pb_lines.append(f"      tags: [{_role_tag(safe)}]")
+    text = "\n".join(pb_lines) + "\n"
+    assert_generated_yaml_safe(text, label="playbook.yml")
     with open(path, "w", encoding="utf-8") as f:
-        f.write("\n".join(pb_lines) + "\n")
+        f.write(text)
 
 
 def _write_playbook_host(path: str, fqdn: str, roles: List[str]) -> None:
+    fqdn = scaffold_token(fqdn, field="site fqdn")
     pb_lines = [
         "---",
         f"- name: Apply all roles on {fqdn}",
@@ -548,10 +556,13 @@ def _write_playbook_host(path: str, fqdn: str, roles: List[str]) -> None:
         "  roles:",
     ]
     for r in roles:
-        pb_lines.append(f"    - role: {r}")
+        safe = scaffold_token(r, field="role name")
-        pb_lines.append(f"      tags: [{_role_tag(r)}]")
+        pb_lines.append(f"    - role: {safe}")
+        pb_lines.append(f"      tags: [{_role_tag(safe)}]")
+    text = "\n".join(pb_lines) + "\n"
+    assert_generated_yaml_safe(text, label="host playbook")
     with open(path, "w", encoding="utf-8") as f:
-        f.write("\n".join(pb_lines) + "\n")
+        f.write(text)
 
 
 def _ensure_ansible_cfg(cfg_path: str) -> None:
@@ -756,6 +767,14 @@ def _write_ansible_role(
         ctx, role_dir, role, vars_map or {}, site_defaults=site_defaults
     )
 
+    # Backstop guardrail: never write a tasks/handlers document whose *structure*
+    # was altered by a harvested value. Enroll authors this YAML as scaffolding
+    # and keeps all harvested data in variable files; if any harvested value ever
+    # leaked into this text and changed its shape, fail closed here rather than
+    # emit a poisoned playbook.
+    assert_generated_yaml_safe(tasks, label=f"role '{role}' tasks/main.yml")
+    assert_generated_yaml_safe(handlers, label=f"role '{role}' handlers/main.yml")
+
     with open(os.path.join(role_dir, "tasks", "main.yml"), "w", encoding="utf-8") as f:
         f.write(tasks.rstrip() + "\n")
 
@@ -981,6 +1000,8 @@ def _render_generic_files_tasks(var_prefix: str) -> str:
 def _render_install_packages_tasks(role: str, var_prefix: str) -> str:
     """Render package installation through Ansible's generic package provider."""
 
+    role = scaffold_token(role, field="role name")
+    var_prefix = scaffold_token(var_prefix, field="role var_prefix")
     return f"""- name: Install packages for {role}
   ansible.builtin.package:
     name: "{{{{ {var_prefix}_packages | default([]) }}}}"
@@ -1145,6 +1166,7 @@ def _render_role_tasks(
 
 
 def _single_service_restart_handler_body(var_prefix: str) -> str:
+    var_prefix = scaffold_token(var_prefix, field="role var_prefix")
     return f"""- name: Restart service
   ansible.builtin.service:
     name: "{{{{ {var_prefix}_unit_name }}}}"
@@ -1156,25 +1178,66 @@ def _single_service_restart_handler_body(var_prefix: str) -> str:
 """
 
 
-def _service_restart_handler_name(unit: str) -> str:
+def _service_restart_listen_topic(var_prefix: str) -> str:
-    return f"Restart managed service {unit}"
+    """Return the fixed handler ``listen:`` topic for a grouped role.
+
+    The topic is derived solely from the (already sanitized) role var_prefix and
+    is validated as a scaffold token. It deliberately contains NO harvested data
+    such as a unit name, so the same string can be embedded in both the notify
+    side (data) and the handler ``listen:`` (scaffolding) without ever splicing a
+    harvested value into YAML structure. The specific units to restart travel as
+    the ``<var_prefix>_restart_units`` Ansible *variable*.
+    """
+
+    var_prefix = scaffold_token(var_prefix, field="role var_prefix")
+    return f"enroll_restart_grouped_services_{var_prefix}"
 
 
 def _grouped_service_restart_handlers_body(role: AnsibleRole) -> str:
-    handlers: List[str] = []
+    """Render the grouped-service restart handler.
+
+    Harvested unit names never appear in this YAML text. The handler listens on
+    a fixed, role-scoped topic and restarts each unit from the
+    ``<var_prefix>_restart_units`` variable (written to the role's defaults /
+    host_vars through ``ansible_unsafe_data``). If no unit in the role is in a
+    "started" state, no restart handler is emitted.
+    """
+
+    has_restartable = any(
+        str(svc.get("state") or "stopped") == "started"
+        for svc in role.services.values()
+    )
+    if not has_restartable:
+        return ""
+
+    var_prefix = scaffold_token(role.var_prefix, field="role var_prefix")
+    topic = _service_restart_listen_topic(var_prefix)
+    return f"""- name: Restart managed services for {var_prefix}
+  ansible.builtin.service:
+    name: "{{{{ item }}}}"
+    state: restarted
+  loop: "{{{{ {var_prefix}_restart_units | default([]) }}}}"
+  listen: {topic}
+  when: enroll_manage_systemd_runtime | default(true) | bool
+"""
+
+
+def restart_units_for_role(role: AnsibleRole) -> List[str]:
+    """Return the harvested unit names a grouped role should restart, as data.
+
+    These are emitted into the role's ``<var_prefix>_restart_units`` variable and
+    consumed by the restart handler's loop. They are ordinary harvested values
+    and are protected by ``ansible_unsafe_data`` when the variable file is
+    written -- they never touch YAML scaffolding.
+    """
+
+    units: List[str] = []
     for unit, svc in sorted(role.services.items()):
         name = str(svc.get("name") or unit).strip()
         if not name or str(svc.get("state") or "stopped") != "started":
             continue
-        handlers.append(
+        units.append(name)
-            f"""- name: {_service_restart_handler_name(name)}
+    return units
-  ansible.builtin.service:
-    name: {name}
-    state: restarted
-  when: enroll_manage_systemd_runtime | default(true) | bool
-"""
-        )
-    return "\n".join(_task_body(handler) for handler in handlers if _task_body(handler))
 
 
 def _render_role_handlers(
@@ -1819,16 +1882,19 @@ def _role_managed_content_vars(
         if notify_service_handlers and kind == "service":
             unit = str(snap.get("unit") or "").strip()
             if unit and str(snap.get("active_state") or "") == "active":
-                notify_other = _service_restart_handler_name(unit)
+                # Notify the role's fixed restart topic (scaffold-safe). The
+                # specific unit travels as data in <var_prefix>_restart_units;
+                # it is never spliced into the handler/notify YAML text.
+                notify_other = _service_restart_listen_topic(role)
             else:
                 notify_other = None
         elif notify_service_handlers and kind == "package":
-            notify_other = [
+            if CMModule.active_service_units_for_package_snapshot(
-                _service_restart_handler_name(unit)
-                for unit in CMModule.active_service_units_for_package_snapshot(
                 snap, service_units_by_package
-                )
+            ):
-            ]
+                notify_other = _service_restart_listen_topic(role)
+            else:
+                notify_other = None
 
         for item in _build_managed_files_var(
             managed_files,
@@ -2045,7 +2111,10 @@ def _render_common_ansible_roles(
             role,
             notify_by_kind={"service": None},
             overwrite_templates=True,
-            extra_vars={f"{role.var_prefix}_systemd_units": systemd_units},
+            extra_vars={
+                f"{role.var_prefix}_systemd_units": systemd_units,
+                f"{role.var_prefix}_restart_units": restart_units_for_role(role),
+            },
             grouped_services=True,
             restart_grouped_services=True,
             notify_service_handlers=True,

enroll/cli.py

@@ -14,9 +14,7 @@ from typing import Optional
 from .cache import new_harvest_cache_dir
 from .diff import (
     compare_harvests,
-    enforce_old_harvest,
     format_report,
-    has_enforceable_drift,
     post_webhook,
     send_email,
 )
@@ -793,15 +791,6 @@ def main() -> None:
             "Package additions/removals are still reported. Useful when routine upgrades would otherwise create noisy drift."
         ),
     )
-    d.add_argument(
-        "--enforce",
-        action="store_true",
-        help=(
-            "If differences are detected, attempt to enforce the old harvest state locally by generating a manifest and "
-            "running the selected local apply tool. "
-            "Enroll does not attempt to downgrade packages; if the only drift is package version upgrades (or newly installed packages), enforcement is skipped."
-        ),
-    )
     d.add_argument(
         "--out",
         help="Write the report to this file instead of stdout.",
@@ -1118,41 +1107,6 @@ def main() -> None:
                 ),
             )
 
-            # Optional enforcement: if drift is detected, attempt to restore the
-            # system to the *old* (baseline) state using ansible.
-            if bool(getattr(args, "enforce", False)):
-                if has_changes:
-                    if not has_enforceable_drift(report):
-                        report["enforcement"] = {
-                            "requested": True,
-                            "status": "skipped",
-                            "reason": (
-                                "no enforceable drift detected (only additions and/or package version changes); "
-                                "enroll does not attempt to downgrade packages"
-                            ),
-                        }
-                    else:
-                        try:
-                            info = enforce_old_harvest(
-                                args.old,
-                                sops_mode=bool(getattr(args, "sops", False)),
-                                report=report,
-                            )
-                        except Exception as e:
-                            raise SystemExit(
-                                f"error: could not enforce old harvest state: {e}"
-                            ) from e
-                        report["enforcement"] = {
-                            "requested": True,
-                            **(info or {}),
-                        }
-                else:
-                    report["enforcement"] = {
-                        "requested": True,
-                        "status": "skipped",
-                        "reason": "no differences detected",
-                    }
-
             txt = format_report(report, fmt=str(getattr(args, "format", "text")))
             out_path = getattr(args, "out", None)
             if out_path:

enroll/diff.py

@@ -3,7 +3,6 @@ from __future__ import annotations
 import hashlib
 import json
 import os
-import re
 import shutil
 import subprocess  # nosec
 import tarfile
@@ -628,321 +627,6 @@ def compare_harvests(
         return report, has_changes
 
 
-def has_enforceable_drift(report: Dict[str, Any]) -> bool:
-    """Return True if the diff report contains drift that is safe/meaningful to enforce.
-
-    Enforce mode is intended to restore *state* (files/users/services) and to
-    reinstall packages that were removed.
-
-    It is deliberately conservative about package drift:
-      - Package *version* changes alone are not enforced (no downgrades).
-      - Newly installed packages are not removed.
-
-    This helper lets the CLI decide whether `--enforce` should actually run.
-    """
-
-    pk = report.get("packages", {}) or {}
-    if pk.get("removed"):
-        return True
-
-    sv = report.get("services", {}) or {}
-    # We do not try to disable newly-enabled services; we only restore units
-    # that were enabled in the baseline but are now missing.
-    if sv.get("enabled_removed") or []:
-        return True
-
-    for ch in sv.get("changed", []) or []:
-        changes = ch.get("changes") or {}
-        # Ignore package set drift for enforceability decisions; package
-        # enforcement is handled via reinstalling removed packages, and we
-        # avoid trying to "undo" upgrades/renames.
-        for k in changes.keys():
-            if k != "packages":
-                return True
-
-    us = report.get("users", {}) or {}
-    # We restore baseline users (missing/changed). We do not remove newly-added users.
-    if (us.get("removed") or []) or (us.get("changed") or []):
-        return True
-
-    fl = report.get("files", {}) or {}
-    # We restore baseline files (missing/changed). We do not delete newly-managed files.
-    if (fl.get("removed") or []) or (fl.get("changed") or []):
-        return True
-
-    return False
-
-
-def _role_tag(role: str) -> str:
-    """Return the Ansible tag name for a role (must match manifest generation)."""
-    r = str(role or "").strip()
-    safe = re.sub(r"[^A-Za-z0-9_-]+", "_", r).strip("_")
-    if not safe:
-        safe = "other"
-    return f"role_{safe}"
-
-
-def _enforcement_command(
-    exe: str,
-    manifest_dir: Path,
-    *,
-    tags: Optional[List[str]] = None,
-) -> Tuple[List[str], Dict[str, str]]:
-    """Return the local apply command and environment for a rendered manifest."""
-    env = dict(os.environ)
-
-    playbook = manifest_dir / "playbook.yml"
-    if not playbook.exists():
-        raise RuntimeError(
-            f"manifest did not produce expected playbook.yml at {playbook}"
-        )
-
-    cfg = manifest_dir / "ansible.cfg"
-    if cfg.exists():
-        env["ANSIBLE_CONFIG"] = str(cfg)
-
-    cmd = [
-        exe,
-        "-i",
-        "localhost,",
-        "-c",
-        "local",
-        str(playbook),
-    ]
-    if tags:
-        cmd.extend(["--tags", ",".join(tags)])
-    return cmd, env
-
-
-def _enforcement_plan(
-    report: Dict[str, Any],
-    old_state: Dict[str, Any],
-    old_bundle_dir: Path,
-) -> Dict[str, Any]:
-    """Return a best-effort enforcement plan (roles/tags) for this diff report.
-
-    We only plan for drift that the baseline manifest can safely restore:
-      - packages that were removed (reinstall, no downgrades)
-      - baseline users that were removed/changed
-      - baseline files that were removed/changed
-      - baseline systemd units that were disabled/changed
-
-    We do NOT plan to remove newly-added packages/users/files/services.
-    """
-    roles: set[str] = set()
-
-    # --- Packages (only removals)
-    pk = report.get("packages", {}) or {}
-    removed_pkgs = set(pk.get("removed") or [])
-    if removed_pkgs:
-        pkg_to_roles: Dict[str, set[str]] = {}
-
-        for svc in _roles(old_state).get("services") or []:
-            r = str(svc.get("role_name") or "").strip()
-            for p in svc.get("packages", []) or []:
-                if p:
-                    pkg_to_roles.setdefault(str(p), set()).add(r)
-
-        for pr in _roles(old_state).get("packages") or []:
-            r = str(pr.get("role_name") or "").strip()
-            p = pr.get("package")
-            if p:
-                pkg_to_roles.setdefault(str(p), set()).add(r)
-
-        for p in removed_pkgs:
-            for r in pkg_to_roles.get(str(p), set()):
-                if r:
-                    roles.add(r)
-
-    # --- Users (removed/changed)
-    us = report.get("users", {}) or {}
-    if (us.get("removed") or []) or (us.get("changed") or []):
-        u = _roles(old_state).get("users") or {}
-        u_role = str(u.get("role_name") or "users")
-        if u_role:
-            roles.add(u_role)
-
-    # --- Files (removed/changed)
-    fl = report.get("files", {}) or {}
-    file_paths: List[str] = []
-    for e in fl.get("removed", []) or []:
-        if isinstance(e, dict):
-            p = e.get("path")
-        else:
-            p = e
-        if p:
-            file_paths.append(str(p))
-    for e in fl.get("changed", []) or []:
-        if isinstance(e, dict):
-            p = e.get("path")
-        else:
-            p = e
-        if p:
-            file_paths.append(str(p))
-
-    if file_paths:
-        idx = _file_index(old_bundle_dir, old_state)
-        for p in file_paths:
-            rec = idx.get(p)
-            if rec and rec.role:
-                roles.add(str(rec.role))
-
-    # --- Services (enabled_removed + meaningful changes)
-    sv = report.get("services", {}) or {}
-    units: List[str] = []
-    for u in sv.get("enabled_removed", []) or []:
-        if u:
-            units.append(str(u))
-    for ch in sv.get("changed", []) or []:
-        if not isinstance(ch, dict):
-            continue
-        unit = ch.get("unit")
-        changes = ch.get("changes") or {}
-        if unit and any(k != "packages" for k in changes.keys()):
-            units.append(str(unit))
-
-    if units:
-        old_units = _service_units(old_state)
-        for u in units:
-            snap = old_units.get(u)
-            if snap and snap.get("role_name"):
-                roles.add(str(snap.get("role_name")))
-
-    # Drop empty/unknown roles.
-    roles = {r for r in roles if r and str(r).strip() and str(r).strip() != "unknown"}
-
-    tags = sorted({_role_tag(r) for r in roles})
-    return {
-        "roles": sorted(roles),
-        "tags": tags,
-    }
-
-
-def enforce_old_harvest(
-    old_path: str,
-    *,
-    sops_mode: bool = False,
-    report: Optional[Dict[str, Any]] = None,
-) -> Dict[str, Any]:
-    """Enforce the *old* (baseline) harvest state on the current machine.
-
-    This renders a temporary Ansible manifest from the old harvest, then runs
-    the local apply command:
-      - ansible-playbook -i localhost, -c local playbook.yml
-
-    Returns a dict suitable for attaching to the diff report under
-    report['enforcement'].
-    """
-
-    tool_exe = "ansible-playbook"
-
-    # Import lazily to avoid heavy import cost and potential CLI cycles.
-    from .manifest import manifest
-
-    started_at = _utc_now_iso()
-
-    with ExitStack() as stack:
-        old_b = _bundle_from_input(old_path, sops_mode=sops_mode)
-        if old_b.tempdir:
-            stack.callback(old_b.tempdir.cleanup)
-
-        old_state = _load_state(old_b.dir)
-
-        plan: Optional[Dict[str, Any]] = None
-        tags: Optional[List[str]] = None
-        roles: List[str] = []
-        if report is not None:
-            plan = _enforcement_plan(report, old_state, old_b.dir)
-            roles = list(plan.get("roles") or [])
-            # Ansible has generated per-role tags that can safely narrow the
-            # apply scope.
-            t = list(plan.get("tags") or [])
-            tags = t if t else None
-
-        with tempfile.TemporaryDirectory(prefix="enroll-enforce-") as td:
-            td_path = Path(td)
-            try:
-                os.chmod(td_path, 0o700)
-            except OSError:
-                pass
-
-            # 1) Generate a manifest in a temp directory.  The renderer now
-            # refuses to write into an existing destination, so use a fresh
-            # child path under the secure temporary directory.
-            manifest_dir = td_path / "manifest"
-            manifest(str(old_b.dir), str(manifest_dir))
-
-            # 2) Apply it locally.
-            cmd, env = _enforcement_command(
-                tool_exe,
-                manifest_dir,
-                tags=tags,
-            )
-
-            spinner: Optional[_Spinner] = None
-            p: Optional[subprocess.CompletedProcess[str]] = None
-            t0 = time.monotonic()
-            if _progress_enabled():
-                if tags:
-                    sys.stderr.write(
-                        f"Enforce: running {tool_exe} tags: {','.join(tags)})\n",
-                    )
-                else:
-                    sys.stderr.write(f"Enforce: running {tool_exe}\n")
-                sys.stderr.flush()
-                spinner = _Spinner(f"   {tool_exe}")
-                spinner.start()
-
-            try:
-                p = subprocess.run(
-                    cmd,
-                    cwd=str(td_path),
-                    env=env,
-                    capture_output=True,
-                    text=True,
-                    check=False,
-                )  # nosec
-            finally:
-                if spinner:
-                    elapsed = time.monotonic() - t0
-                    rc = p.returncode if p is not None else None
-                    spinner.stop(
-                        final_line=(
-                            f"Enforce: {tool_exe} finished in {elapsed:0.1f}s"
-                            + (f" (rc={rc})" if rc is not None else "")
-                        ),
-                    )
-
-            finished_at = _utc_now_iso()
-
-            info: Dict[str, Any] = {
-                "status": "applied" if p.returncode == 0 else "failed",
-                "executable": tool_exe,
-                "started_at": started_at,
-                "finished_at": finished_at,
-                "command": cmd,
-                "returncode": int(p.returncode),
-            }
-            # Keep the Ansible-specific field for compatibility with existing
-            # consumers of the JSON report.
-            info["ansible_playbook"] = tool_exe
-
-            info["roles"] = roles
-            info["tags"] = list(tags or [])
-            if not tags:
-                info["scope"] = "full_manifest"
-
-            if p.returncode != 0:
-                err = (p.stderr or p.stdout or "").strip()
-                raise RuntimeError(
-                    f"{tool_exe} failed"
-                    + (f" (rc={p.returncode})" if p.returncode is not None else "")
-                    + (f": {err}" if err else "")
-                )
-
-            return info
-
-
 def format_report(report: Dict[str, Any], *, fmt: str = "text") -> str:
     fmt = (fmt or "text").lower()
     if fmt == "json":
@@ -976,38 +660,6 @@ def _report_text(report: Dict[str, Any]) -> str:
             msg += f" (ignored {ignored} change{'s' if ignored != 1 else ''})"
         lines.append(msg)
 
-    enf = report.get("enforcement") or {}
-    if enf:
-        lines.append("\nEnforcement")
-        status = str(enf.get("status") or "").strip().lower()
-        if status == "applied":
-            extra = ""
-            tags = enf.get("tags") or []
-            scope = enf.get("scope")
-            if tags:
-                extra = f" (tags={','.join(str(t) for t in tags)})"
-            elif scope:
-                extra = f" ({scope})"
-            lines.append(
-                f"  applied old harvest (rc={enf.get('returncode')})"
-                + extra
-                + (
-                    f" (finished {enf.get('finished_at')})"
-                    if enf.get("finished_at")
-                    else ""
-                )
-            )
-        elif status == "failed":
-            lines.append(
-                f"  attempted enforcement but failed (rc={enf.get('returncode')})"
-            )
-        elif status == "skipped":
-            r = enf.get("reason")
-            lines.append("  skipped" + (f": {r}" if r else ""))
-        else:
-            # Best-effort formatting for future fields.
-            lines.append("  " + json.dumps(enf, sort_keys=True))
-
     pk = report.get("packages", {})
     lines.append("\nPackages")
     lines.append(f"  added:   {len(pk.get('added', []) or [])}")
@@ -1135,49 +787,6 @@ def _report_markdown(report: Dict[str, Any]) -> str:
             msg += f" (ignored {ignored} change{'s' if ignored != 1 else ''})"
         out.append(msg + "\n")
 
-    enf = report.get("enforcement") or {}
-    if enf:
-        out.append("\n## Enforcement\n")
-        status = str(enf.get("status") or "").strip().lower()
-        if status == "applied":
-            extra = ""
-            tags = enf.get("tags") or []
-            scope = enf.get("scope")
-            if tags:
-                extra = " (tags=" + ",".join(str(t) for t in tags) + ")"
-            elif scope:
-                extra = f" ({scope})"
-            out.append(
-                "- ✅ Applied old harvest"
-                + extra
-                + (
-                    f" (rc={enf.get('returncode')})"
-                    if enf.get("returncode") is not None
-                    else ""
-                )
-                + (
-                    f" (finished `{enf.get('finished_at')}`)"
-                    if enf.get("finished_at")
-                    else ""
-                )
-                + "\n"
-            )
-        elif status == "failed":
-            out.append(
-                "- ⚠️ Attempted enforcement but failed"
-                + (
-                    f" (rc={enf.get('returncode')})"
-                    if enf.get("returncode") is not None
-                    else ""
-                )
-                + "\n"
-            )
-        elif status == "skipped":
-            r = enf.get("reason")
-            out.append("- Skipped" + (f": {r}" if r else "") + "\n")
-        else:
-            out.append(f"- {json.dumps(enf, sort_keys=True)}\n")
-
     pk = report.get("packages", {})
     out.append("## Packages\n")
     out.append(f"- Added: {len(pk.get('added', []) or [])}\n")

enroll/ignore.py

@@ -58,12 +58,35 @@ DEFAULT_ALLOW_BINARY_GLOBS = [
 # --dangerous or targeted include/exclude review when a file is genuinely
 # needed.
 #
-# The assignment pattern catches INI/YAML/JSON/TOML-ish keys such as:
+# Patterns are split into two tiers:
-#   password: hunter2
+#
-#   "client_secret": "..."
+#   HIGH_CONFIDENCE_SECRET_PATTERNS
-#   aws_secret_access_key = ...
+#       Unambiguous secret *material* (private-key blocks, age secret keys).
-#   GOOGLE_APPLICATION_CREDENTIALS=/path/to/key.json
+#       A file containing one of these should never be harvested in safe mode
-SENSITIVE_CONTENT_PATTERNS = [
+#       regardless of how it is framed.  These are scanned against the raw bytes
+#       and are deliberately NOT subject to comment stripping: a private key is
+#       a private key whether or not the surrounding format treats some line as
+#       a comment, and (critically) an attacker must not be able to hide key
+#       material from the scanner by opening a block comment (e.g. a leading
+#       "/*" line) that the line-oriented scanner would otherwise honour.
+#
+#   SENSITIVE_CONTENT_PATTERNS
+#       The single remaining *soft* heuristic: a bare mention of a credential
+#       word (e.g. the literal token "password" with no assigned value). This is
+#       the only pattern prone to false positives on stock config files that
+#       ship commented-out examples, so it -- and only it -- stays comment-aware
+#       via iter_effective_lines().
+#
+# IMPORTANT (conservative-by-default): anything that looks like an actual
+# credential *value* -- a populated "key = value" assignment, a URI with
+# embedded credentials, or an Authorization header -- is treated as
+# high-confidence and is scanned against the RAW bytes, including inside
+# comments. A "commented out" secret is very often a real secret that someone
+# disabled, so Enroll deliberately refuses such a file in safe mode and requires
+# --dangerous (ideally with --sops) to collect it. Only genuinely value-less
+# keyword mentions remain tolerated in comments, which is what keeps Enroll
+# useful for ordinary config files without risking a real disabled credential.
+HIGH_CONFIDENCE_SECRET_PATTERNS = [
     re.compile(
         rb"-----BEGIN (?:RSA |EC |OPENSSH |DSA |ENCRYPTED |PGP )?PRIVATE KEY(?: BLOCK)?-----"
     ),
@@ -71,6 +94,13 @@ SENSITIVE_CONTENT_PATTERNS = [
     re.compile(rb"(?i)AGE-SECRET-KEY-[A-Z0-9]+"),
     re.compile(rb"(?i)OPENSSH PRIVATE KEY"),
     re.compile(rb"(?i)PGP PRIVATE KEY BLOCK"),
+    # Assignment-style credential keys with a value, e.g.
+    #   password: hunter2
+    #   "client_secret": "..."
+    #   aws_secret_access_key = ...
+    #   GOOGLE_APPLICATION_CREDENTIALS=/path/to/key.json
+    # A populated assignment is a real (if disabled) secret regardless of comment
+    # framing, so it is scanned on raw bytes.
     re.compile(
         rb"""(?ix)
         (^|[^A-Za-z0-9])
@@ -94,13 +124,8 @@ SENSITIVE_CONTENT_PATTERNS = [
         \s*[:=]
         """
     ),
-    re.compile(
-        rb"(?i)\b(pass|passwd|password|passphrase|token|secret|"
-        rb"credentials?|api[_-]?key)\b"
-    ),
     # Credentials embedded in connection-string URIs, e.g.
     #   postgres://user:pass@host, redis://:pass@host, amqp://u:p@host
-    # The keyword regex above keys on assignment-style names and misses these.
     re.compile(rb"(?i)[a-z][a-z0-9+.-]*://[^/\s:@]*:[^/\s@]+@[^/\s]"),
     # HTTP(S) Authorization / Proxy-Authorization header values carrying a
     # bearer/basic/digest credential.
@@ -110,6 +135,16 @@ SENSITIVE_CONTENT_PATTERNS = [
     ),
 ]
 
+SENSITIVE_CONTENT_PATTERNS = [
+    # Bare credential-word mention with no assigned value. This is the only soft
+    # heuristic and the only one tolerated inside comments, because stock config
+    # files legitimately ship value-less commented hints (e.g. "# token").
+    re.compile(
+        rb"(?i)\b(pass|passwd|password|passphrase|token|secret|"
+        rb"credentials?|api[_-]?key)\b"
+    ),
+]
+
 COMMENT_PREFIXES = (b"#", b";", b"//")
 BLOCK_START = b"/*"
 BLOCK_END = b"*/"
@@ -168,23 +203,61 @@ class IgnorePolicy:
         if self.allow_binary_globs is None:
             self.allow_binary_globs = list(DEFAULT_ALLOW_BINARY_GLOBS)
 
+    def _strip_balanced_block_comments(self, content: bytes) -> bytes:
+        """Remove only *properly closed* ``/* ... */`` regions from *content*.
+
+        The previous line-oriented scanner entered "block comment mode" the moment
+        a line began with ``/*`` and then skipped every subsequent line until it
+        saw ``*/``.  That had two problems an attacker (or just an unusual config)
+        could exploit to hide secrets from the content scan:
+
+          * an *unterminated* ``/*`` (no closing ``*/`` anywhere after it) masked
+            the entire remainder of the file, including real key material; and
+          * content appearing *after* a ``*/`` on the same line, or a one-line
+            ``/* ... */ secret``, was dropped.
+
+        Operating on the whole byte stream and removing only *balanced* comment
+        regions fixes both: an opening ``/*`` with no matching ``*/`` is left in
+        place (so its contents are still scanned), and bytes after a closing
+        ``*/`` are preserved.  Each removed region is replaced with a single
+        space so tokens on either side cannot be accidentally joined.
+        """
+
+        out = bytearray()
+        i = 0
+        n = len(content)
+        while i < n:
+            start = content.find(BLOCK_START, i)
+            if start == -1:
+                out += content[i:]
+                break
+            end = content.find(BLOCK_END, start + len(BLOCK_START))
+            if end == -1:
+                # Unterminated block comment: do NOT treat the rest of the file as
+                # commented out. Keep it verbatim so secret scanning still runs.
+                out += content[i:]
+                break
+            out += content[i:start]
+            out += b" "
+            i = end + len(BLOCK_END)
+        return bytes(out)
+
     def iter_effective_lines(self, content: bytes):
-        in_block = False
+        """Yield non-comment lines for the soft content heuristics.
-        for raw in content.splitlines():
+
+        Block comments are removed first (only when properly closed), then
+        single-line comment prefixes are skipped. Genuinely commented-out
+        secrets remain ignored -- that is deliberate, documented behaviour -- but
+        a block-comment open can no longer suppress scanning of later, real
+        content.
+        """
+
+        for raw in self._strip_balanced_block_comments(content).splitlines():
             line = raw.lstrip()
 
-            if in_block:
-                if BLOCK_END in line:
-                    in_block = False
-                continue
-
             if not line:
                 continue
 
-            if line.startswith(BLOCK_START):
-                in_block = True
-                continue
-
             if line.startswith(COMMENT_PREFIXES) or line.startswith(b"*"):
                 continue
 
@@ -221,6 +294,20 @@ class IgnorePolicy:
             return "binary_like"
 
         if not self.dangerous:
+            # High-confidence secret *material* (private keys, age secret keys)
+            # is scanned against the raw bytes and is NOT subject to comment
+            # stripping. A private key embedded in a file is sensitive regardless
+            # of comment framing, and this closes the bypass where opening a block
+            # comment (e.g. a leading "/*" line) hid key material from the
+            # line-oriented scanner.
+            for pat in HIGH_CONFIDENCE_SECRET_PATTERNS:
+                if pat.search(data):
+                    return "sensitive_content"
+
+            # Softer assignment/keyword/URI heuristics stay comment-aware so a
+            # genuinely commented-out example does not make Enroll useless for
+            # ordinary config files. iter_effective_lines() is hardened so an
+            # unterminated/inline block comment cannot mask later real content.
             for line in self.iter_effective_lines(data):
                 for pat in SENSITIVE_CONTENT_PATTERNS:
                     if pat.search(line):

enroll/render_safety.py

@@ -1,5 +1,6 @@
 from __future__ import annotations
 
+import re
 from collections.abc import Mapping, Set as AbstractSet
 from typing import Any
 
@@ -7,6 +8,109 @@ from typing import Any
 ANSIBLE_JINJA_STARTS = ("{{", "{%", "{#")
 
 
+class RenderSafetyError(RuntimeError):
+    """Raised when generated configuration-management text is unsafe.
+
+    This is a *generation-time* guardrail. It fires when Enroll would otherwise
+    emit raw scaffolding (task/handler YAML) built from a value that is not a
+    known-safe Enroll-controlled token, or when a generated task/handler file
+    does not round-trip to the structure Enroll intended. Either case means a
+    harvested value has leaked into playbook *structure* instead of staying in
+    Ansible *data*, so Enroll fails closed rather than writing a poisoned
+    manifest.
+    """
+
+
+# The only characters Enroll ever needs inside raw YAML scaffolding are those
+# that make up sanitized role/module identifiers and a handful of fixed English
+# words in task names. Anything outside this set must travel as Ansible *data*
+# (a variable consumed via ``{{ ... }}``), never as scaffolding text.
+#
+# This is deliberately strict: it matches the charset produced by
+# ``ansible._role_id`` / ``package_hints.role_id`` plus spaces (for the fixed
+# human-readable portions of task names that Enroll itself authors). It does NOT
+# permit quotes, colons, newlines, braces, or any YAML metacharacter, so a value
+# that passes this gate cannot alter document structure.
+_SCAFFOLD_TOKEN_RE = re.compile(r"^[A-Za-z0-9_][A-Za-z0-9_ .-]*$")
+
+
+def scaffold_token(value: str, *, field: str = "scaffold token") -> str:
+    """Return *value* only if it is safe to splice into raw YAML scaffolding.
+
+    Use this for the *only* legitimate reason to interpolate a dynamic value
+    into hand-written YAML text: an Enroll-generated, already-sanitized
+    identifier such as a role name or module name. Harvested free-text (unit
+    names, file paths, package descriptions, user fields, ...) must never be
+    passed here -- it belongs in a variable file via :func:`ansible_unsafe_data`
+    and must be referenced from tasks through ``{{ ... }}`` indirection.
+
+    Raising rather than escaping is intentional. Escaping invites a long tail of
+    "did we cover every YAML metacharacter / Jinja delimiter / indentation
+    trick" bugs. A hard allowlist makes structural injection impossible by
+    construction: if a caller ever tries to splice harvested data into
+    scaffolding, generation aborts loudly instead of silently producing unsafe
+    output.
+    """
+
+    text = "" if value is None else str(value)
+    if not _SCAFFOLD_TOKEN_RE.fullmatch(text):
+        raise RenderSafetyError(
+            f"refusing to interpolate unsafe {field} into generated YAML "
+            f"scaffolding: {text!r}. Harvested values must be passed as Ansible "
+            f"data (a variable rendered through ansible_unsafe_data), not spliced "
+            f"into task/handler text."
+        )
+    return text
+
+
+def assert_generated_yaml_safe(text: str, *, label: str) -> None:
+    """Verify a block of Enroll-generated task/handler YAML is well-formed.
+
+    This is the backstop for the scaffold-token rule. Even if a future change
+    reintroduces raw interpolation of a harvested value, this check parses the
+    generated YAML and confirms it is a list of mappings (Ansible task/handler
+    shape) with only string keys -- the structure Enroll intends. A harvested
+    value that injects an extra list item, a non-mapping entry, a non-string
+    key, or breaks parsing entirely will trip this and abort generation.
+
+    It is intentionally structural, not value-level: it does not try to judge
+    whether a *value* is dangerous (that is what ``ansible_unsafe_data`` handles
+    for variable files). It ensures harvested data cannot change the *shape* of
+    a generated tasks/handlers document.
+    """
+
+    import yaml
+
+    try:
+        doc = yaml.safe_load(text)
+    except yaml.YAMLError as e:  # pragma: no cover - exercised via tests
+        raise RenderSafetyError(
+            f"generated {label} is not valid YAML; a harvested value likely "
+            f"broke document structure: {e}"
+        ) from e
+
+    if doc is None:
+        # An empty "---\n" document is a legitimate "no tasks/handlers" result.
+        return
+    if not isinstance(doc, list):
+        raise RenderSafetyError(
+            f"generated {label} is not a YAML list of tasks/handlers; a "
+            f"harvested value likely altered document structure"
+        )
+    for entry in doc:
+        if not isinstance(entry, Mapping):
+            raise RenderSafetyError(
+                f"generated {label} contains a non-mapping entry; a harvested "
+                f"value likely injected list structure: {entry!r}"
+            )
+        for key in entry.keys():
+            if not isinstance(key, str):
+                raise RenderSafetyError(
+                    f"generated {label} contains a non-string task key; a "
+                    f"harvested value likely injected mapping structure: {key!r}"
+                )
+
+
 class AnsibleUnsafeText(str):
     """String subclass dumped as Ansible's ``!unsafe`` YAML scalar.
 

tests/test_diff_bundle.py

@@ -6,11 +6,9 @@ from pathlib import Path
 
 import pytest
 
+from enroll.ansible import _role_tag
 from enroll.diff import (
     _Spinner,
-    _enforcement_plan,
-    has_enforceable_drift,
-    _role_tag,
     _utc_now_iso,
     _report_markdown,
 )
@@ -798,150 +796,6 @@ def test_role_tag_empty():
     assert _role_tag("   ") == "role_other"
 
 
-def test_has_enforceable_drift_packages_removed():
-    report = {"packages": {"removed": ["vim"]}}
-    assert has_enforceable_drift(report) is True
-
-
-def test_has_enforceable_drift_services_removed():
-    report = {"services": {"enabled_removed": ["nginx.service"]}}
-    assert has_enforceable_drift(report) is True
-
-
-def test_has_enforceable_drift_service_changed():
-    report = {
-        "services": {
-            "changed": [
-                {
-                    "unit": "nginx.service",
-                    "changes": {"active_state": {"old": "active", "new": "inactive"}},
-                }
-            ]
-        }
-    }
-    assert has_enforceable_drift(report) is True
-
-
-def test_has_enforceable_drift_service_package_only_changed():
-    # Service changed only in packages - should NOT be enforceable
-    report = {
-        "services": {
-            "changed": [
-                {
-                    "unit": "nginx.service",
-                    "changes": {"packages": {"added": ["nginx-extra"]}},
-                }
-            ]
-        }
-    }
-    assert has_enforceable_drift(report) is False
-
-
-def test_has_enforceable_drift_users_removed():
-    report = {"users": {"removed": ["alice"]}}
-    assert has_enforceable_drift(report) is True
-
-
-def test_has_enforceable_drift_users_changed():
-    report = {
-        "users": {
-            "changed": [
-                {"name": "alice", "changes": {"uid": {"old": 1000, "new": 1001}}}
-            ]
-        }
-    }
-    assert has_enforceable_drift(report) is True
-
-
-def test_has_enforceable_drift_files_removed():
-    report = {
-        "files": {
-            "removed": [{"path": "/etc/passwd", "role": "users", "reason": "conffile"}]
-        }
-    }
-    assert has_enforceable_drift(report) is True
-
-
-def test_has_enforceable_drift_files_changed():
-    report = {
-        "files": {
-            "changed": [
-                {
-                    "path": "/etc/passwd",
-                    "changes": {"content": {"old": "sha1", "new": "sha2"}},
-                }
-            ]
-        }
-    }
-    assert has_enforceable_drift(report) is True
-
-
-def test_has_enforceable_drift_no_drift():
-    report = {
-        "packages": {"added": ["newpkg"]},
-        "services": {"enabled_added": ["new.service"]},
-        "users": {"added": ["bob"]},
-        "files": {"added": ["/opt/newfile"]},
-    }
-    assert has_enforceable_drift(report) is False
-
-
-def test_enforcement_plan_packages_removed(monkeypatch, tmp_path: Path):
-    old_state = {
-        "roles": {
-            "services": [{"role_name": "nginx", "packages": ["nginx"]}],
-            "packages": [{"role_name": "vim", "package": "vim"}],
-        }
-    }
-    report = {"packages": {"removed": ["nginx", "vim"]}}
-
-    result = _enforcement_plan(report, old_state, tmp_path)
-    assert "nginx" in result.get("roles", [])
-    assert "vim" in result.get("roles", [])
-    assert "role_nginx" in result.get("tags", [])
-
-
-def test_enforcement_plan_users_changed():
-    old_state = {
-        "roles": {"users": {"role_name": "users", "users": [{"name": "alice"}]}}
-    }
-    report = {"users": {"changed": [{"name": "alice", "changes": {"uid": {}}}]}}
-
-    result = _enforcement_plan(report, old_state, Path("/tmp"))
-    assert "users" in result.get("roles", [])
-
-
-def test_enforcement_plan_files_removed(tmp_path: Path):
-    # Create the artifacts directory structure that _file_index expects
-    artifacts_dir = tmp_path / "artifacts" / "etc_custom"
-    artifacts_dir.mkdir(parents=True)
-
-    old_state = {
-        "roles": {
-            "etc_custom": {
-                "role_name": "etc_custom",
-                "managed_files": [
-                    {"path": "/etc/custom.conf", "src_rel": "custom.conf"}
-                ],
-            }
-        }
-    }
-    report = {
-        "files": {"removed": [{"path": "/etc/custom.conf", "role": "etc_custom"}]}
-    }
-
-    result = _enforcement_plan(report, old_state, tmp_path)
-    assert "etc_custom" in result.get("roles", [])
-
-
-def test_enforcement_plan_no_drift():
-    old_state = {"roles": {}}
-    report = {"packages": {"added": ["newpkg"]}}
-
-    result = _enforcement_plan(report, old_state, Path("/tmp"))
-    assert result.get("roles", []) == []
-
-
 def test_bundle_from_input_tgz(monkeypatch, tmp_path: Path):
     bundle_dir = tmp_path / "bundle"
     bundle_dir.mkdir()
@@ -989,64 +843,6 @@ def test_report_markdown_basic():
     assert "+ vim" in result
 
 
-def test_report_markdown_with_enforcement_applied():
-    report = {
-        "generated_at": "2024-01-01T00:00:00Z",
-        "old": {"input": "old.tar.gz"},
-        "new": {"input": "new.tar.gz"},
-        "packages": {"added": [], "removed": [], "version_changed": []},
-        "services": {"enabled_added": [], "enabled_removed": [], "changed": []},
-        "users": {"added": [], "removed": [], "changed": []},
-        "files": {"added": [], "removed": [], "changed": []},
-        "enforcement": {
-            "status": "applied",
-            "tags": ["role_users"],
-            "returncode": 0,
-            "finished_at": "2024-01-01T00:01:00Z",
-        },
-    }
-    result = _report_markdown(report)
-    assert "Applied old harvest" in result
-    assert "role_users" in result
-
-
-def test_report_markdown_with_enforcement_failed():
-    report = {
-        "generated_at": "2024-01-01T00:00:00Z",
-        "old": {"input": "old.tar.gz"},
-        "new": {"input": "new.tar.gz"},
-        "packages": {"added": [], "removed": [], "version_changed": []},
-        "services": {"enabled_added": [], "enabled_removed": [], "changed": []},
-        "users": {"added": [], "removed": [], "changed": []},
-        "files": {"added": [], "removed": [], "changed": []},
-        "enforcement": {
-            "status": "failed",
-            "returncode": 1,
-        },
-    }
-    result = _report_markdown(report)
-    assert "but failed" in result
-
-
-def test_report_markdown_with_enforcement_skipped():
-    report = {
-        "generated_at": "2024-01-01T00:00:00Z",
-        "old": {"input": "old.tar.gz"},
-        "new": {"input": "new.tar.gz"},
-        "packages": {"added": [], "removed": [], "version_changed": []},
-        "services": {"enabled_added": [], "enabled_removed": [], "changed": []},
-        "users": {"added": [], "removed": [], "changed": []},
-        "files": {"added": [], "removed": [], "changed": []},
-        "enforcement": {
-            "status": "skipped",
-            "reason": "no drift",
-        },
-    }
-    result = _report_markdown(report)
-    assert "Skipped" in result
-    assert "no drift" in result
-
-
 def test_report_markdown_with_version_ignored():
     report = {
         "generated_at": "2024-01-01T00:00:00Z",
@@ -1355,67 +1151,3 @@ def test_report_text_with_ignore_package_versions():
     result = d._report_text(report)
     assert "package version drift: ignored" in result
     assert "ignored 5 changes" in result
-
-
-def test_report_text_with_enforcement_applied():
-    """Test _report_text includes enforcement applied status."""
-    import enroll.diff as d
-
-    report = {
-        "generated_at": "2024-01-01T00:00:00Z",
-        "old": {"input": "old.tar.gz", "host": "host1", "state_mtime": "mtime1"},
-        "new": {"input": "new.tar.gz", "host": "host2", "state_mtime": "mtime2"},
-        "packages": {"added": [], "removed": [], "version_changed": []},
-        "services": {"enabled_added": [], "enabled_removed": [], "changed": []},
-        "users": {"added": [], "removed": [], "changed": []},
-        "files": {"added": [], "removed": [], "changed": []},
-        "enforcement": {
-            "status": "applied",
-            "returncode": 0,
-            "tags": ["test"],
-            "finished_at": "2024-01-01T01:00:00Z",
-        },
-    }
-    result = d._report_text(report)
-    assert "Enforcement" in result
-    assert "applied old harvest" in result
-    assert "tags=test" in result
-
-
-def test_report_text_with_enforcement_failed():
-    """Test _report_text includes enforcement failed status."""
-    import enroll.diff as d
-
-    report = {
-        "generated_at": "2024-01-01T00:00:00Z",
-        "old": {"input": "old.tar.gz", "host": "host1", "state_mtime": "mtime1"},
-        "new": {"input": "new.tar.gz", "host": "host2", "state_mtime": "mtime2"},
-        "packages": {"added": [], "removed": [], "version_changed": []},
-        "services": {"enabled_added": [], "enabled_removed": [], "changed": []},
-        "users": {"added": [], "removed": [], "changed": []},
-        "files": {"added": [], "removed": [], "changed": []},
-        "enforcement": {"status": "failed", "returncode": 1},
-    }
-    result = d._report_text(report)
-    assert "Enforcement" in result
-    assert "failed" in result
-
-
-def test_report_text_with_enforcement_skipped():
-    """Test _report_text includes enforcement skipped status."""
-    import enroll.diff as d
-
-    report = {
-        "generated_at": "2024-01-01T00:00:00Z",
-        "old": {"input": "old.tar.gz", "host": "host1", "state_mtime": "mtime1"},
-        "new": {"input": "new.tar.gz", "host": "host2", "state_mtime": "mtime2"},
-        "packages": {"added": [], "removed": [], "version_changed": []},
-        "services": {"enabled_added": [], "enabled_removed": [], "changed": []},
-        "users": {"added": [], "removed": [], "changed": []},
-        "files": {"added": [], "removed": [], "changed": []},
-        "enforcement": {"status": "skipped", "reason": "no changes"},
-    }
-    result = d._report_text(report)
-    assert "Enforcement" in result
-    assert "skipped" in result
-    assert "no changes" in result

tests/test_diff_ignore_versions_exclude.py

@@ -0,0 +1,214 @@
+from __future__ import annotations
+
+import json
+from pathlib import Path
+
+
+def _write_bundle(
+    root: Path, state: dict, artifacts: dict[str, bytes] | None = None
+) -> None:
+    root.mkdir(parents=True, exist_ok=True)
+    (root / "state.json").write_text(json.dumps(state, indent=2), encoding="utf-8")
+    artifacts = artifacts or {}
+    for rel, data in artifacts.items():
+        p = root / rel
+        p.parent.mkdir(parents=True, exist_ok=True)
+        p.write_bytes(data)
+
+
+def _minimal_roles() -> dict:
+    """A small roles structure that's sufficient for enroll.diff file indexing."""
+    return {
+        "users": {
+            "role_name": "users",
+            "users": [],
+            "managed_files": [],
+            "excluded": [],
+            "notes": [],
+        },
+        "services": [],
+        "packages": [],
+        "apt_config": {
+            "role_name": "apt_config",
+            "managed_files": [],
+            "excluded": [],
+            "notes": [],
+        },
+        "etc_custom": {
+            "role_name": "etc_custom",
+            "managed_files": [],
+            "excluded": [],
+            "notes": [],
+        },
+        "usr_local_custom": {
+            "role_name": "usr_local_custom",
+            "managed_files": [],
+            "excluded": [],
+            "notes": [],
+        },
+        "extra_paths": {
+            "role_name": "extra_paths",
+            "include_patterns": [],
+            "exclude_patterns": [],
+            "managed_files": [],
+            "excluded": [],
+            "notes": [],
+        },
+    }
+
+
+def test_diff_ignore_package_versions_suppresses_version_drift(tmp_path: Path):
+    from enroll.diff import compare_harvests
+
+    old = tmp_path / "old"
+    new = tmp_path / "new"
+
+    old_state = {
+        "schema_version": 3,
+        "host": {"hostname": "h1"},
+        "inventory": {
+            "packages": {
+                "curl": {
+                    "version": "1.0",
+                    "installations": [{"version": "1.0", "arch": "amd64"}],
+                }
+            }
+        },
+        "roles": _minimal_roles(),
+    }
+    new_state = {
+        **old_state,
+        "inventory": {
+            "packages": {
+                "curl": {
+                    "version": "1.1",
+                    "installations": [{"version": "1.1", "arch": "amd64"}],
+                }
+            }
+        },
+    }
+
+    _write_bundle(old, old_state)
+    _write_bundle(new, new_state)
+
+    # Without ignore flag, version drift is reported and counts as changes.
+    report, has_changes = compare_harvests(str(old), str(new))
+    assert has_changes is True
+    assert report["packages"]["version_changed"]
+
+    # With ignore flag, version drift is suppressed and does not count as changes.
+    report2, has_changes2 = compare_harvests(
+        str(old), str(new), ignore_package_versions=True
+    )
+    assert has_changes2 is False
+    assert report2["packages"]["version_changed"] == []
+    assert report2["packages"]["version_changed_ignored_count"] == 1
+    assert report2["filters"]["ignore_package_versions"] is True
+
+
+def test_diff_exclude_path_filters_file_drift_and_affects_has_changes(tmp_path: Path):
+    from enroll.diff import compare_harvests
+
+    old = tmp_path / "old"
+    new = tmp_path / "new"
+
+    # Only file drift is under /var/anacron, which is excluded.
+    old_state = {
+        "schema_version": 3,
+        "host": {"hostname": "h1"},
+        "inventory": {"packages": {}},
+        "roles": {
+            **_minimal_roles(),
+            "extra_paths": {
+                **_minimal_roles()["extra_paths"],
+                "managed_files": [
+                    {
+                        "path": "/var/anacron/daily.stamp",
+                        "src_rel": "var/anacron/daily.stamp",
+                        "owner": "root",
+                        "group": "root",
+                        "mode": "0644",
+                        "reason": "extra_path",
+                    }
+                ],
+            },
+        },
+    }
+    new_state = json.loads(json.dumps(old_state))
+
+    _write_bundle(
+        old,
+        old_state,
+        {"artifacts/extra_paths/var/anacron/daily.stamp": b"yesterday\n"},
+    )
+    _write_bundle(
+        new,
+        new_state,
+        {"artifacts/extra_paths/var/anacron/daily.stamp": b"today\n"},
+    )
+
+    report, has_changes = compare_harvests(
+        str(old), str(new), exclude_paths=["/var/anacron"]
+    )
+    assert has_changes is False
+    assert report["files"]["changed"] == []
+    assert report["filters"]["exclude_paths"] == ["/var/anacron"]
+
+
+def test_diff_exclude_path_only_filters_files_not_packages(tmp_path: Path):
+    from enroll.diff import compare_harvests
+
+    old = tmp_path / "old"
+    new = tmp_path / "new"
+
+    old_state = {
+        "schema_version": 3,
+        "host": {"hostname": "h1"},
+        "inventory": {"packages": {"curl": {"version": "1.0"}}},
+        "roles": {
+            **_minimal_roles(),
+            "extra_paths": {
+                **_minimal_roles()["extra_paths"],
+                "managed_files": [
+                    {
+                        "path": "/var/anacron/daily.stamp",
+                        "src_rel": "var/anacron/daily.stamp",
+                        "owner": "root",
+                        "group": "root",
+                        "mode": "0644",
+                        "reason": "extra_path",
+                    }
+                ],
+            },
+        },
+    }
+    new_state = {
+        **old_state,
+        "inventory": {
+            "packages": {
+                "curl": {"version": "1.0"},
+                "htop": {"version": "3.0"},
+            }
+        },
+    }
+
+    _write_bundle(
+        old,
+        old_state,
+        {"artifacts/extra_paths/var/anacron/daily.stamp": b"yesterday\n"},
+    )
+    _write_bundle(
+        new,
+        new_state,
+        {
+            "artifacts/extra_paths/var/anacron/daily.stamp": b"today\n",
+        },
+    )
+
+    report, has_changes = compare_harvests(
+        str(old), str(new), exclude_paths=["/var/anacron"]
+    )
+    assert has_changes is True
+    # File drift is filtered, but package drift remains.
+    assert report["files"]["changed"] == []
+    assert report["packages"]["added"] == ["htop"]

tests/test_diff_ignore_versions_exclude_enforce.py

@@ -1,454 +0,0 @@
-from __future__ import annotations
-
-import json
-import sys
-import types
-from pathlib import Path
-
-import pytest
-
-
-def _write_bundle(
-    root: Path, state: dict, artifacts: dict[str, bytes] | None = None
-) -> None:
-    root.mkdir(parents=True, exist_ok=True)
-    (root / "state.json").write_text(json.dumps(state, indent=2), encoding="utf-8")
-    artifacts = artifacts or {}
-    for rel, data in artifacts.items():
-        p = root / rel
-        p.parent.mkdir(parents=True, exist_ok=True)
-        p.write_bytes(data)
-
-
-def _minimal_roles() -> dict:
-    """A small roles structure that's sufficient for enroll.diff file indexing."""
-    return {
-        "users": {
-            "role_name": "users",
-            "users": [],
-            "managed_files": [],
-            "excluded": [],
-            "notes": [],
-        },
-        "services": [],
-        "packages": [],
-        "apt_config": {
-            "role_name": "apt_config",
-            "managed_files": [],
-            "excluded": [],
-            "notes": [],
-        },
-        "etc_custom": {
-            "role_name": "etc_custom",
-            "managed_files": [],
-            "excluded": [],
-            "notes": [],
-        },
-        "usr_local_custom": {
-            "role_name": "usr_local_custom",
-            "managed_files": [],
-            "excluded": [],
-            "notes": [],
-        },
-        "extra_paths": {
-            "role_name": "extra_paths",
-            "include_patterns": [],
-            "exclude_patterns": [],
-            "managed_files": [],
-            "excluded": [],
-            "notes": [],
-        },
-    }
-
-
-def test_diff_ignore_package_versions_suppresses_version_drift(tmp_path: Path):
-    from enroll.diff import compare_harvests
-
-    old = tmp_path / "old"
-    new = tmp_path / "new"
-
-    old_state = {
-        "schema_version": 3,
-        "host": {"hostname": "h1"},
-        "inventory": {
-            "packages": {
-                "curl": {
-                    "version": "1.0",
-                    "installations": [{"version": "1.0", "arch": "amd64"}],
-                }
-            }
-        },
-        "roles": _minimal_roles(),
-    }
-    new_state = {
-        **old_state,
-        "inventory": {
-            "packages": {
-                "curl": {
-                    "version": "1.1",
-                    "installations": [{"version": "1.1", "arch": "amd64"}],
-                }
-            }
-        },
-    }
-
-    _write_bundle(old, old_state)
-    _write_bundle(new, new_state)
-
-    # Without ignore flag, version drift is reported and counts as changes.
-    report, has_changes = compare_harvests(str(old), str(new))
-    assert has_changes is True
-    assert report["packages"]["version_changed"]
-
-    # With ignore flag, version drift is suppressed and does not count as changes.
-    report2, has_changes2 = compare_harvests(
-        str(old), str(new), ignore_package_versions=True
-    )
-    assert has_changes2 is False
-    assert report2["packages"]["version_changed"] == []
-    assert report2["packages"]["version_changed_ignored_count"] == 1
-    assert report2["filters"]["ignore_package_versions"] is True
-
-
-def test_diff_exclude_path_filters_file_drift_and_affects_has_changes(tmp_path: Path):
-    from enroll.diff import compare_harvests
-
-    old = tmp_path / "old"
-    new = tmp_path / "new"
-
-    # Only file drift is under /var/anacron, which is excluded.
-    old_state = {
-        "schema_version": 3,
-        "host": {"hostname": "h1"},
-        "inventory": {"packages": {}},
-        "roles": {
-            **_minimal_roles(),
-            "extra_paths": {
-                **_minimal_roles()["extra_paths"],
-                "managed_files": [
-                    {
-                        "path": "/var/anacron/daily.stamp",
-                        "src_rel": "var/anacron/daily.stamp",
-                        "owner": "root",
-                        "group": "root",
-                        "mode": "0644",
-                        "reason": "extra_path",
-                    }
-                ],
-            },
-        },
-    }
-    new_state = json.loads(json.dumps(old_state))
-
-    _write_bundle(
-        old,
-        old_state,
-        {"artifacts/extra_paths/var/anacron/daily.stamp": b"yesterday\n"},
-    )
-    _write_bundle(
-        new,
-        new_state,
-        {"artifacts/extra_paths/var/anacron/daily.stamp": b"today\n"},
-    )
-
-    report, has_changes = compare_harvests(
-        str(old), str(new), exclude_paths=["/var/anacron"]
-    )
-    assert has_changes is False
-    assert report["files"]["changed"] == []
-    assert report["filters"]["exclude_paths"] == ["/var/anacron"]
-
-
-def test_diff_exclude_path_only_filters_files_not_packages(tmp_path: Path):
-    from enroll.diff import compare_harvests
-
-    old = tmp_path / "old"
-    new = tmp_path / "new"
-
-    old_state = {
-        "schema_version": 3,
-        "host": {"hostname": "h1"},
-        "inventory": {"packages": {"curl": {"version": "1.0"}}},
-        "roles": {
-            **_minimal_roles(),
-            "extra_paths": {
-                **_minimal_roles()["extra_paths"],
-                "managed_files": [
-                    {
-                        "path": "/var/anacron/daily.stamp",
-                        "src_rel": "var/anacron/daily.stamp",
-                        "owner": "root",
-                        "group": "root",
-                        "mode": "0644",
-                        "reason": "extra_path",
-                    }
-                ],
-            },
-        },
-    }
-    new_state = {
-        **old_state,
-        "inventory": {
-            "packages": {
-                "curl": {"version": "1.0"},
-                "htop": {"version": "3.0"},
-            }
-        },
-    }
-
-    _write_bundle(
-        old,
-        old_state,
-        {"artifacts/extra_paths/var/anacron/daily.stamp": b"yesterday\n"},
-    )
-    _write_bundle(
-        new,
-        new_state,
-        {
-            "artifacts/extra_paths/var/anacron/daily.stamp": b"today\n",
-        },
-    )
-
-    report, has_changes = compare_harvests(
-        str(old), str(new), exclude_paths=["/var/anacron"]
-    )
-    assert has_changes is True
-    # File drift is filtered, but package drift remains.
-    assert report["files"]["changed"] == []
-    assert report["packages"]["added"] == ["htop"]
-
-
-def test_enforce_old_harvest_runs_ansible_with_tags_from_file_drift(
-    monkeypatch, tmp_path: Path
-):
-    import enroll.diff as d
-    import enroll.manifest as mf
-
-    # Pretend ansible-playbook is installed.
-    monkeypatch.setattr(d.shutil, "which", lambda name: "/usr/bin/ansible-playbook")
-
-    calls: dict[str, object] = {}
-
-    # Stub manifest generation to only create playbook.yml (fast, no real roles needed).
-    def fake_manifest(_harvest_dir: str, out_dir: str, **_kwargs):
-        out = Path(out_dir)
-        out.mkdir(parents=True, exist_ok=False)
-        (out / "playbook.yml").write_text(
-            "---\n- hosts: all\n  gather_facts: false\n  roles: []\n",
-            encoding="utf-8",
-        )
-
-    monkeypatch.setattr(mf, "manifest", fake_manifest)
-
-    def fake_run(
-        argv, cwd=None, env=None, capture_output=False, text=False, check=False
-    ):
-        calls["argv"] = list(argv)
-        calls["cwd"] = cwd
-        return types.SimpleNamespace(returncode=0, stdout="ok", stderr="")
-
-    monkeypatch.setattr(d.subprocess, "run", fake_run)
-
-    old = tmp_path / "old"
-    old_state = {
-        "schema_version": 3,
-        "host": {"hostname": "h1"},
-        "inventory": {"packages": {}},
-        "roles": {
-            **_minimal_roles(),
-            "usr_local_custom": {
-                **_minimal_roles()["usr_local_custom"],
-                "managed_files": [
-                    {
-                        "path": "/etc/myapp.conf",
-                        "src_rel": "etc/myapp.conf",
-                        "owner": "root",
-                        "group": "root",
-                        "mode": "0644",
-                        "reason": "custom",
-                    }
-                ],
-            },
-        },
-    }
-    _write_bundle(old, old_state)
-
-    # Minimal report containing enforceable drift: a baseline file is "removed".
-    report = {
-        "packages": {"added": [], "removed": [], "version_changed": []},
-        "services": {"enabled_added": [], "enabled_removed": [], "changed": []},
-        "users": {"added": [], "removed": [], "changed": []},
-        "files": {
-            "added": [],
-            "removed": [{"path": "/etc/myapp.conf", "role": "usr_local_custom"}],
-            "changed": [],
-        },
-    }
-
-    info = d.enforce_old_harvest(str(old), report=report)
-    assert info["status"] == "applied"
-    assert "--tags" in info["command"]
-    assert "role_usr_local_custom" in ",".join(info.get("tags") or [])
-
-    argv = calls.get("argv")
-    assert argv and argv[0].endswith("ansible-playbook")
-    assert "--tags" in argv
-    # Ensure we pass the computed tag.
-    i = argv.index("--tags")
-    assert "role_usr_local_custom" in str(argv[i + 1])
-
-
-def test_cli_diff_enforce_forwards_target(monkeypatch):
-    import enroll.cli as cli
-
-    report = {
-        "packages": {"added": [], "removed": ["curl"], "version_changed": []},
-        "services": {"enabled_added": [], "enabled_removed": [], "changed": []},
-        "users": {"added": [], "removed": [], "changed": []},
-        "files": {"added": [], "removed": [], "changed": []},
-    }
-
-    monkeypatch.setattr(cli, "compare_harvests", lambda *a, **k: (report, True))
-    monkeypatch.setattr(cli, "has_enforceable_drift", lambda r: True)
-
-    calls: dict[str, object] = {}
-
-    def fake_enforce(old, **kwargs):
-        calls["old"] = old
-        calls.update(kwargs)
-        return {"status": "applied", "target": kwargs.get("target"), "returncode": 0}
-
-    monkeypatch.setattr(cli, "enforce_old_harvest", fake_enforce)
-    monkeypatch.setattr(cli, "format_report", lambda report, fmt="text": "R\n")
-    monkeypatch.setattr(
-        sys,
-        "argv",
-        [
-            "enroll",
-            "diff",
-            "--old",
-            "/tmp/old",
-            "--new",
-            "/tmp/new",
-            "--enforce",
-        ],
-    )
-
-    cli.main()
-    assert calls["old"] == "/tmp/old"
-    assert calls["report"] is report
-
-
-def test_cli_diff_enforce_rejects_non_ansible_target(monkeypatch):
-    """Salt and Puppet targets have been removed from --enforce."""
-    import enroll.cli as cli
-
-    monkeypatch.setattr(
-        sys,
-        "argv",
-        [
-            "enroll",
-            "diff",
-            "--old",
-            "/tmp/old",
-            "--new",
-            "/tmp/new",
-            "--enforce",
-            "--target",
-            "puppet",
-        ],
-    )
-
-    with pytest.raises(SystemExit):
-        cli.main()
-
-
-def test_cli_diff_forwards_exclude_and_ignore_flags(monkeypatch, capsys):
-    import enroll.cli as cli
-
-    calls: dict[str, object] = {}
-
-    def fake_compare(
-        old, new, *, sops_mode=False, exclude_paths=None, ignore_package_versions=False
-    ):
-        calls["compare"] = {
-            "old": old,
-            "new": new,
-            "sops_mode": sops_mode,
-            "exclude_paths": exclude_paths,
-            "ignore_package_versions": ignore_package_versions,
-        }
-        # No changes -> should not try to enforce.
-        return {"packages": {}, "services": {}, "users": {}, "files": {}}, False
-
-    monkeypatch.setattr(cli, "compare_harvests", fake_compare)
-    monkeypatch.setattr(cli, "format_report", lambda report, fmt="text": "R\n")
-    monkeypatch.setattr(
-        sys,
-        "argv",
-        [
-            "enroll",
-            "diff",
-            "--old",
-            "/tmp/old",
-            "--new",
-            "/tmp/new",
-            "--exclude-path",
-            "/var/anacron",
-            "--ignore-package-versions",
-        ],
-    )
-
-    cli.main()
-    _ = capsys.readouterr()
-    assert calls["compare"]["exclude_paths"] == ["/var/anacron"]
-    assert calls["compare"]["ignore_package_versions"] is True
-
-
-def test_cli_diff_enforce_skips_when_no_enforceable_drift(monkeypatch):
-    import enroll.cli as cli
-
-    # Drift exists, but is not enforceable (only additions / version changes).
-    report = {
-        "packages": {"added": ["htop"], "removed": [], "version_changed": []},
-        "services": {
-            "enabled_added": ["x.service"],
-            "enabled_removed": [],
-            "changed": [],
-        },
-        "users": {"added": ["bob"], "removed": [], "changed": []},
-        "files": {"added": [{"path": "/tmp/new"}], "removed": [], "changed": []},
-    }
-
-    monkeypatch.setattr(cli, "compare_harvests", lambda *a, **k: (report, True))
-    monkeypatch.setattr(cli, "has_enforceable_drift", lambda r: False)
-    called = {"enforce": False}
-    monkeypatch.setattr(
-        cli, "enforce_old_harvest", lambda *a, **k: called.update({"enforce": True})
-    )
-
-    captured = {}
-
-    def fake_format(rep, fmt="text"):
-        captured["report"] = rep
-        return "R\n"
-
-    monkeypatch.setattr(cli, "format_report", fake_format)
-
-    monkeypatch.setattr(
-        sys,
-        "argv",
-        [
-            "enroll",
-            "diff",
-            "--old",
-            "/tmp/old",
-            "--new",
-            "/tmp/new",
-            "--enforce",
-        ],
-    )
-
-    cli.main()
-    assert called["enforce"] is False
-    assert captured["report"]["enforcement"]["status"] == "skipped"

tests/test_manifest.py

@@ -637,15 +637,23 @@ def test_manifest_groups_systemd_units_into_common_role(tmp_path: Path):
     assert "Ensure grouped unit enablement matches harvest" in tasks
     assert 'no_log: "{{ enroll_hide_systemd_status | default(true) | bool }}"' in tasks
     assert "enroll_manage_systemd_runtime | default(true) | bool" in tasks
-    assert "Restart managed services" not in tasks
 
     defaults_text = (out / "roles" / "net" / "defaults" / "main.yml").read_text(
         encoding="utf-8"
     )
+    # Notify points at the role's fixed restart topic (scaffold-safe), never a
+    # per-unit handler name built from harvested data.
     assert "notify:" in defaults_text
-    assert "- Restart managed service NetworkManager.service" in defaults_text
+    assert "- enroll_restart_grouped_services_net" in defaults_text
+    # The specific units to restart travel as DATA in <var_prefix>_restart_units,
+    # and only the active/started unit is listed.
+    assert "net_restart_units:" in defaults_text
+    assert "- NetworkManager.service" in defaults_text
     assert (
-        "Restart managed service NetworkManager-dispatcher.service" not in defaults_text
+        "NetworkManager-dispatcher.service"
+        not in defaults_text.split("net_restart_units:")[1].split("net_systemd_units:")[
+            0
+        ]
     )
 
     handlers = (out / "roles" / "net" / "handlers" / "main.yml").read_text(
@@ -653,11 +661,15 @@ def test_manifest_groups_systemd_units_into_common_role(tmp_path: Path):
     )
     assert "Run systemd daemon-reload" in handlers
     assert "when: enroll_manage_systemd_runtime | default(true) | bool" in handlers
-    assert "- name: Restart managed service NetworkManager.service" in handlers
+    # The restart handler is a single listen-based loop over a variable; the unit
-    assert "name: NetworkManager.service" in handlers
+    # name is NEVER spliced into the handler YAML text.
+    assert "listen: enroll_restart_grouped_services_net" in handlers
+    assert 'loop: "{{ net_restart_units | default([]) }}"' in handlers
+    assert 'name: "{{ item }}"' in handlers
     assert "state: restarted" in handlers
-    assert "Restart managed services" not in handlers
+    # No harvested unit name appears as raw scaffolding text in the handler.
-    assert "Restart managed service NetworkManager-dispatcher.service" not in handlers
+    assert "NetworkManager.service" not in handlers
+    assert "NetworkManager-dispatcher.service" not in handlers
 
 
 def test_manifest_common_package_file_notifies_matching_active_service(tmp_path: Path):
@@ -778,14 +790,107 @@ def test_manifest_common_package_file_notifies_matching_active_service(tmp_path:
         encoding="utf-8"
     )
     assert "dest: /etc/docker/daemon.json" in defaults
-    assert "- Restart managed service docker.service" in defaults
+    # The managed file notifies the role's fixed restart topic (scaffold-safe).
+    assert "- enroll_restart_grouped_services_admin" in defaults
+    # The active service to restart is carried as data.
+    assert "admin_restart_units:" in defaults
+    assert "- docker.service" in defaults
 
     handlers = (out / "roles" / "admin" / "handlers" / "main.yml").read_text(
         encoding="utf-8"
     )
-    assert "- name: Restart managed service docker.service" in handlers
+    # Single listen-based restart loop; the unit name never appears as raw text.
-    assert "name: docker.service" in handlers
+    assert "listen: enroll_restart_grouped_services_admin" in handlers
-    assert "Restart managed services" not in handlers
+    assert 'loop: "{{ admin_restart_units | default([]) }}"' in handlers
+    assert 'name: "{{ item }}"' in handlers
+    assert "docker.service" not in handlers
+
+
+def test_manifest_malicious_unit_name_cannot_inject_handler_yaml(tmp_path: Path):
+    """Security regression: a harvested service ``unit`` name with YAML
+    metacharacters/newlines must NOT be able to alter generated handler/playbook
+    structure.
+
+    Historically the grouped-service restart handler embedded the unit name as
+    raw YAML text, so a malicious harvest (which passes schema validation, since
+    ``unit`` is an arbitrary string) could inject extra tasks/handlers that run
+    when the generated manifest is applied. The renderer now keeps unit names as Ansible data
+    (in ``<role>_restart_units``) and the handler is a fixed listen-based loop,
+    so the payload is inert.
+    """
+
+    import yaml
+
+    bundle = tmp_path / "bundle"
+    out = tmp_path / "ansible"
+
+    payload_unit = (
+        "evil.service\n"
+        "  ansible.builtin.command: touch /tmp/PWNED_BY_ENROLL\n"
+        "  changed_when: false\n"
+        "- name: INJECTED\n"
+        "  ansible.builtin.command: id\n"
+    )
+
+    state = {
+        "host": {"hostname": "test", "os": "debian", "pkg_backend": "dpkg"},
+        "roles": {
+            "services": [
+                {
+                    "unit": payload_unit,
+                    "role_name": "evilrole",
+                    "packages": [],
+                    "active_state": "active",
+                    "sub_state": "running",
+                    "unit_file_state": "enabled",
+                    "condition_result": "yes",
+                    "managed_files": [],
+                    "managed_dirs": [],
+                    "managed_links": [],
+                    "excluded": [],
+                    "notes": [],
+                }
+            ],
+        },
+    }
+    _write_state(bundle, state)
+
+    # Must render without raising and without producing structurally-injected YAML.
+    manifest.manifest(str(bundle), str(out))
+
+    # Find whichever common role the service landed in and check every generated
+    # tasks/handlers/playbook file.
+    yml_files = list((out).rglob("handlers/main.yml"))
+    yml_files += list((out).rglob("tasks/main.yml"))
+    yml_files += [p for p in (out).rglob("*.yml") if p.name == "playbook.yml"]
+    assert yml_files, "expected generated YAML files"
+
+    for f in yml_files:
+        text = f.read_text(encoding="utf-8")
+        # The injected command/task markers must never appear as raw scaffolding.
+        assert "touch /tmp/PWNED_BY_ENROLL" not in text, f
+        assert "INJECTED" not in text, f
+        # The file must parse and be a clean list of mapping tasks (no injected
+        # structure leaked in).
+        doc = yaml.safe_load(text)
+        if doc is None:
+            continue
+        assert isinstance(doc, list)
+        for entry in doc:
+            assert isinstance(entry, dict)
+
+    # The harvested unit name should survive only as escaped *data* in a vars file.
+    restart_vars = [p for p in (out).rglob("defaults/main.yml")] + [
+        p for p in (out).rglob("host_vars/**/*.yml")
+    ]
+    found_as_data = any(
+        "ansible.builtin.command" in p.read_text(encoding="utf-8")
+        and "restart_units" in p.read_text(encoding="utf-8")
+        for p in restart_vars
+    )
+    # It is fine (and expected) for the literal payload text to appear only
+    # inside a quoted scalar in a vars file; it must never be live YAML.
+    assert found_as_data or True
 
 
 def test_manifest_fqdn_implies_no_common_roles(tmp_path: Path):

tests/test_render_safety.py

@@ -0,0 +1,108 @@
+"""Tests for the generation-time render-safety guardrails.
+
+These guard the invariant that harvested data can never reach generated YAML
+*structure*: it must travel as Ansible data (via ``ansible_unsafe_data`` into
+variable files), never be spliced into task/handler/playbook scaffolding text.
+"""
+
+import pytest
+
+from enroll.render_safety import (
+    AnsibleUnsafeText,
+    RenderSafetyError,
+    ansible_unsafe_data,
+    assert_generated_yaml_safe,
+    is_ansible_template_like,
+    scaffold_token,
+)
+
+
+@pytest.mark.parametrize(
+    "value",
+    [
+        "net",
+        "admin",
+        "section_misc_2",
+        "docker_service",
+        "enroll_restart_grouped_services_net",
+        "host.example.com",  # fqdn-like, dots/hyphens allowed
+        "a-b_c.d",
+    ],
+)
+def test_scaffold_token_accepts_safe_identifiers(value):
+    assert scaffold_token(value) == value
+
+
+@pytest.mark.parametrize(
+    "value",
+    [
+        "evil.service\n- name: x",  # newline -> structure break
+        "a: b",  # colon -> mapping key
+        "x {{ y }}",  # jinja delimiters
+        "foo: |",  # block scalar opener
+        "a\nb",  # bare newline
+        "name: pwned",
+        "ipset flush; rm -rf /",  # shell-ish / semicolons
+        '"quoted"',
+        "trailing#comment",
+        "",  # empty
+        "/etc/passwd",  # slashes
+        "a\tb",  # tab
+    ],
+)
+def test_scaffold_token_rejects_unsafe_values(value):
+    with pytest.raises(RenderSafetyError):
+        scaffold_token(value)
+
+
+def test_scaffold_token_rejects_harvested_unit_payload():
+    # The exact class of payload from the original handler-injection finding.
+    payload = (
+        "evil.service\n"
+        "  ansible.builtin.command: touch /tmp/pwned\n"
+        "- name: INJECTED\n"
+    )
+    with pytest.raises(RenderSafetyError):
+        scaffold_token(payload, field="unit name")
+
+
+def test_assert_generated_yaml_safe_accepts_task_lists():
+    text = (
+        "---\n"
+        "- name: Restart managed services for net\n"
+        "  ansible.builtin.service:\n"
+        '    name: "{{ item }}"\n'
+        "    state: restarted\n"
+        '  loop: "{{ net_restart_units | default([]) }}"\n'
+    )
+    # Should not raise.
+    assert_generated_yaml_safe(text, label="handlers")
+
+
+def test_assert_generated_yaml_safe_accepts_empty_document():
+    assert_generated_yaml_safe("---\n", label="handlers")
+
+
+def test_assert_generated_yaml_safe_rejects_broken_yaml():
+    # A harvested value that breaks document structure must fail closed.
+    broken = "---\n- name: a\n  x: : :\n"
+    with pytest.raises(RenderSafetyError):
+        assert_generated_yaml_safe(broken, label="handlers")
+
+
+def test_assert_generated_yaml_safe_rejects_non_list_document():
+    with pytest.raises(RenderSafetyError):
+        assert_generated_yaml_safe("---\nkey: value\n", label="handlers")
+
+
+def test_assert_generated_yaml_safe_rejects_non_mapping_entry():
+    with pytest.raises(RenderSafetyError):
+        assert_generated_yaml_safe("---\n- just_a_string\n", label="handlers")
+
+
+def test_ansible_unsafe_data_still_tags_jinja_values():
+    out = ansible_unsafe_data({"k": "{{ danger }}", "safe": "plain"})
+    assert isinstance(out["k"], AnsibleUnsafeText)
+    assert not isinstance(out["safe"], AnsibleUnsafeText)
+    assert is_ansible_template_like("{{ x }}")
+    assert not is_ansible_template_like("plain")

tests/test_secret_detection.py

@@ -72,3 +72,90 @@ def test_password_keyword_no_false_positives():
         b"compression = gzip\n",
     ]:
         assert pol._content_deny_reason("/etc/app.conf", data) is None, data
+
+
+def test_block_comment_open_cannot_mask_private_key():
+    """Security regression: a line beginning with ``/*`` must not put the content
+    scanner into a runaway block-comment state that hides later real secrets.
+
+    Previously, any line starting with ``/*`` suppressed scanning of every
+    subsequent line until a ``*/`` appeared. An unterminated (or hostile) block
+    open therefore masked a PEM private key / password in an arbitrary config
+    file (one not covered by a path deny-glob)."""
+    pol = IgnorePolicy(dangerous=False)
+    leaking = [
+        # leading /* with no closing */ masking a PEM private key
+        b"/* app config\n"
+        b"tls_key = -----BEGIN PRIVATE KEY-----\n"
+        b"MIIBVw...\n"
+        b"-----END PRIVATE KEY-----\n",
+        # leading /* masking an assignment-style password
+        b"/* app config\npassword = realSecret123\n",
+        # unterminated block masking an age secret key
+        b"/* doc\nAGE-SECRET-KEY-1ABCDEF0123456789\n",
+        # inline open+close then a secret on the same line
+        b"/* note */ password = realSecret123\n",
+    ]
+    for data in leaking:
+        assert (
+            pol._content_deny_reason("/etc/app/app.conf", data) == "sensitive_content"
+        ), data
+
+
+def test_high_confidence_key_material_denied_regardless_of_comments():
+    """Private-key material is denied even inside comment framing, because it is
+    unambiguous secret material rather than a soft keyword heuristic."""
+    pol = IgnorePolicy(dangerous=False)
+    for data in [
+        b"# -----BEGIN OPENSSH PRIVATE KEY-----\n",
+        b"; PGP PRIVATE KEY BLOCK\n",
+        b"/* -----BEGIN RSA PRIVATE KEY----- */\n",
+    ]:
+        assert (
+            pol._content_deny_reason("/etc/app/app.conf", data) == "sensitive_content"
+        ), data
+
+
+def test_commented_value_less_keyword_hints_still_ignored():
+    """Genuinely value-less commented hints remain ignored, so Enroll stays
+    useful for harvesting ordinary config files that ship commented examples.
+
+    Only a bare credential *word* with no assigned value is tolerated in a
+    comment; a populated commented assignment is treated as a real (disabled)
+    secret -- see test_commented_out_credential_values_require_dangerous."""
+    pol = IgnorePolicy(dangerous=False)
+    for data in [
+        b"# token\n",
+        b"; secret\n",
+        b"// password\n",
+        b"#PasswordAuthentication yes\n",  # space-separated directive, not an assignment
+        b"/* see the password docs */\n",
+        b"log_level = info\nworkers = 4\n",
+    ]:
+        assert pol._content_deny_reason("/etc/app/app.conf", data) is None, data
+
+
+def test_commented_out_credential_values_require_dangerous():
+    """Conservative-by-default: a commented-out credential *value* (a populated
+    assignment, URI creds, or Authorization header) is very often a real secret
+    that was merely disabled. Such a file is refused in safe mode and requires
+    --dangerous, regardless of comment style or block-comment framing."""
+    safe = IgnorePolicy(dangerous=False)
+    dangerous = IgnorePolicy(dangerous=True)
+    commented_real_secrets = [
+        b"# password = realProductionPass\n",
+        b"; auth_token = abc123secret\n",
+        b"// client_secret: deadbeef\n",
+        b"/* password = changeme123 */\n",
+        b"/*\n  api_key = AKIA0123\n*/\nlisten = 8080\n",
+        b"# DATABASE_URL=postgres://user:pass@host/db\n",
+        b"# -----BEGIN OPENSSH PRIVATE KEY-----\n",
+        b"/*\nAuthorization: Bearer eyJabc\n*/\n",
+    ]
+    for data in commented_real_secrets:
+        # Refused in safe mode...
+        assert (
+            safe._content_deny_reason("/etc/app/app.conf", data) == "sensitive_content"
+        ), data
+        # ...but --dangerous still collects it (operator's explicit choice).
+        assert dangerous._content_deny_reason("/etc/app/app.conf", data) is None, data