diff --git a/.github/workflows/check-hermes-config-rewrite.yml b/.github/workflows/check-hermes-config-rewrite.yml new file mode 100644 index 00000000..931edf14 --- /dev/null +++ b/.github/workflows/check-hermes-config-rewrite.yml @@ -0,0 +1,24 @@ +name: Check Hermes Config Rewrite + +# Regression test for the ensure_hermes_plugin_enabled() heredoc bug fixed +# alongside this workflow. Catches two related symptoms: +# 1. Wrong indent on insert (collapses plugins.enabled onto one line as a +# scalar string under any config whose items use a non-default indent). +# 2. Non-idempotent re-runs that silently duplicate the new entry. +# Runs on every PR — small surface area, fast, no deps beyond bash + python3. +on: + pull_request: + push: + branches: [main] + +jobs: + check-hermes-config-rewrite: + name: install.sh hermes config rewrite + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - name: Run regression cases + run: | + chmod +x scripts/check-hermes-config-rewrite.sh + ./scripts/check-hermes-config-rewrite.sh \ No newline at end of file diff --git a/scripts/check-hermes-config-rewrite.py b/scripts/check-hermes-config-rewrite.py new file mode 100644 index 00000000..f0b0cee0 --- /dev/null +++ b/scripts/check-hermes-config-rewrite.py @@ -0,0 +1,199 @@ +""" +Regression test runner for the ensure_hermes_plugin_enabled() heredoc. + +Extracts the heredoc body from scripts/install.sh and runs it against a +battery of synthetic configs plus the user's own Hermes config.yaml backup +if present. Fails if any case produces an invalid YAML file, collapses the +list into a scalar, duplicates the plugin, or isn't idempotent on re-run. +""" + +from __future__ import annotations + +import os +import re +import subprocess +import sys +import tempfile +import textwrap +from pathlib import Path + +INSTALL_SH = Path(__file__).resolve().parent / "install.sh" +HERMES_BACKUP = Path(os.path.expanduser("~/.hermes/config.yaml.bak.pre-agency-agents")) + +PLUGIN = "agency-agents-router" + + +def extract_heredoc(path: Path) -> str: + text = path.read_text() + # The heredoc body sits between <<'PY' and the next "PY" sentinel on + # its own line. The sentinel is exactly "PY" at column 0. + pattern = re.compile( + r"""python3 - "\$config" "\$plugin" <<'PY'\n(.+?)\nPY\n""", + re.DOTALL, + ) + match = pattern.search(text) + if not match: + raise SystemExit(f"heredoc not found in {path}") + return match.group(1) + + +def run_heredoc(heredoc: str, cfg_text: str): + """Run the heredoc once. Returns (parsed_yaml_dict, error_string).""" + import yaml + with tempfile.TemporaryDirectory() as d: + p = Path(d) / "config.yaml" + p.write_text(cfg_text) + result = subprocess.run( + ["python3", "-", str(p), PLUGIN], + input=heredoc, + capture_output=True, + text=True, + timeout=10, + ) + if result.returncode != 0: + return None, f"exit={result.returncode} stderr={result.stderr[:200]}" + try: + parsed = yaml.safe_load(p.read_text()) + return parsed, None + except yaml.YAMLError as e: + return None, f"yaml parse: {e}" + + +def check_case(heredoc: str, name: str, cfg_text: str) -> list[str]: + import yaml + failures: list[str] = [] + parsed, err = run_heredoc(heredoc, cfg_text) + if err: + failures.append(f"{name}: {err}") + return failures + enabled = (parsed or {}).get("plugins", {}).get("enabled") + if not isinstance(enabled, list): + failures.append(f"{name}: enabled is not a list (got {enabled!r})") + return failures + if PLUGIN not in enabled: + failures.append(f"{name}: plugin missing from enabled") + # Idempotency: re-run on the produced text; expect no further changes. + text = yaml.safe_dump(parsed, sort_keys=False) + parsed2, err2 = run_heredoc(heredoc, text) + if err2: + failures.append(f"{name}: idempotent re-run: {err2}") + return failures + enabled2 = (parsed2 or {}).get("plugins", {}).get("enabled") + if enabled2 != enabled: + failures.append( + f"{name}: idempotent re-run changed enabled: {enabled!r} -> {enabled2!r}" + ) + return failures + + +def main() -> int: + heredoc = extract_heredoc(INSTALL_SH) + print( + f"Extracted heredoc: {len(heredoc)} chars, " + f"{heredoc.count(chr(10)) + 1} lines" + ) + + configs: list[tuple[str, str]] = [ + ( + "Hermes 4-space indent, fresh install", + textwrap.dedent("""\ + model: + name: x + plugins: + disabled: + - old/dead + enabled: + - basic + - chronos + - ponytail + session_reset: + foo: bar + """), + ), + ( + "Corrupted-scalar (post-bug recovery)", + "model:\n name: x\nplugins:\n enabled:\n" + " - agency-agents-router - basic - chronos - ponytail\n", + ), + ( + "Already present (no-op)", + textwrap.dedent("""\ + model: + name: x + plugins: + enabled: + - agency-agents-router + - basic + - chronos + """), + ), + ( + "Empty inline enabled: []", + textwrap.dedent("""\ + model: + name: x + plugins: + enabled: [] + other: + x: 1 + """), + ), + ( + "No plugins: block at all", + textwrap.dedent("""\ + model: + name: x + session_reset: + foo: bar + """), + ), + ( + "Original 2-space indent (script's documented style)", + textwrap.dedent("""\ + model: + name: x + plugins: + enabled: + - basic + - chronos + """), + ), + ( + "Append not prepend (verify position)", + textwrap.dedent("""\ + model: + name: x + plugins: + enabled: + - basic + - chronos + - ponytail + other: + x: 1 + """), + ), + ] + + if HERMES_BACKUP.exists(): + configs.append( + ("Hermes actual config backup (ground truth)", HERMES_BACKUP.read_text()) + ) + + total = 0 + failures: list[str] = [] + for name, cfg in configs: + total += 1 + for f in check_case(heredoc, name, cfg): + failures.append(f) + + if failures: + print(f"\nFAIL ({len(failures)} error(s) across {total} cases):") + for f in failures: + print(f" - {f}") + return 1 + print(f"\nOK: all {total} regression cases passed") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) \ No newline at end of file diff --git a/scripts/check-hermes-config-rewrite.sh b/scripts/check-hermes-config-rewrite.sh new file mode 100755 index 00000000..0dd2fa69 --- /dev/null +++ b/scripts/check-hermes-config-rewrite.sh @@ -0,0 +1,19 @@ +#!/usr/bin/env bash +# +# check-hermes-config-rewrite.sh — regression test for the +# ensure_hermes_plugin_enabled() heredoc in scripts/install.sh. +# +# Reproduces and guards against the indent bug: the previous heredoc hardcoded +# a 2-space indent when inserting into plugins.enabled, which broke any +# config that used a different list-item indent (Hermes' default is 4 spaces). +# Symptom: plugins.enabled collapses onto one line as a plain scalar string +# when re-parsed, and the script's idempotency check fails to detect that +# the plugin is already there. +# +# Usage: ./scripts/check-hermes-config-rewrite.sh +# Exits non-zero on any failure. Mirrors scripts/check-X.sh style. + +set -euo pipefail +cd "$(dirname "$0")/.." + +python3 scripts/check-hermes-config-rewrite.py \ No newline at end of file diff --git a/scripts/install.sh b/scripts/install.sh index 2f8e9a91..fcd99d71 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -1071,79 +1071,155 @@ ensure_hermes_plugin_enabled() { python3 - "$config" "$plugin" <<'PY' from pathlib import Path import sys +import re path = Path(sys.argv[1]) plugin = sys.argv[2] text = path.read_text() if path.exists() else "" lines = text.splitlines() +plugin_strip = plugin.strip() -# Already enabled? -in_plugins = False -in_enabled = False -for line in lines: +# Locate the plugins block boundaries, the indent of the enabled: key, and +# the indent of any existing list items beneath it. Tracking these explicitly +# avoids the previous bug where the script hardcoded " " and broke any +# config that used a different list-item indent (Hermes' default is 4 spaces). +plugin_start = None +end_line = None +enabled_indent = "" +item_indent = "" +has_enabled = False +enabled_empty = False +for i, line in enumerate(lines): if line.startswith("plugins:"): - in_plugins = True - in_enabled = False - continue - if in_plugins and line and not line.startswith((" ", "\t")): - in_plugins = False - in_enabled = False - stripped_line = line.strip() - if in_plugins and stripped_line == "enabled:": - in_enabled = True - continue - if in_plugins and stripped_line.startswith("enabled:") and "[]" in stripped_line: - in_enabled = False - continue - if in_enabled: - stripped = line.strip() - if stripped.startswith("-"): - value = stripped[1:].strip().strip('"\'') - if value == plugin: - sys.exit(0) - elif line.startswith(" ") and stripped.endswith(":"): - in_enabled = False + plugin_start = i + j = i + 1 + broke = False + while j < len(lines): + jl = lines[j] + if jl and not jl.startswith((" ", "\t")): + broke = True + break + stripped = jl.strip() + if stripped.startswith("enabled:") and not enabled_indent: + has_enabled = True + enabled_indent = jl[: len(jl) - len(stripped)] + if "[]" in stripped: + enabled_empty = True + elif stripped.startswith("-") and has_enabled and not item_indent: + item_indent = jl[: len(jl) - len(stripped)] + j += 1 + # If the inner loop ran off the end of the file (no sibling key to + # break on), end_line must still point one past the last scanned line + # so subsequent inserts land at the right place. + end_line = j if broke else len(lines) + break -if not lines: - lines = ["plugins:", " enabled:", f" - {plugin}"] -elif not any(line.startswith("plugins:") for line in lines): +# Detect both "plugin already enabled" and the corrupted-scalar failure mode. +# The previous bug emitted a 2-space-indent entry under a 4-space-indented +# list, which PyYAML parses as a plain scalar string: +# plugins.enabled: ['agency-agents-router - basic - chronos - ponytail'] +# The file on disk still has literal "- " markers glued together — we repair +# it by splitting the line back into one item per line. +corrupted_lines = [] +has_plugin_already = False +if has_enabled and not enabled_empty: + for idx in range(plugin_start + 1, end_line): + l = lines[idx] + stripped = l.strip() + if not stripped.startswith("-"): + continue + # Count "- " occurrences in the full stripped line. A healthy item + # has exactly one (the leading "- " marker); a corrupted glued line + # has more. We can't use whitespace-strict matching because words + # like "agency-agents-router" contain dashes. + if stripped.count("- ") > 1: + corrupted_lines.append(idx) + else: + value = stripped[1:].strip().strip('"\'') + if value == plugin_strip: + has_plugin_already = True + +# Repair corrupted lines (reverse order so indices stay valid as we splice). +for idx in sorted(corrupted_lines, reverse=True): + l = lines[idx] + stripped = l.strip() + if not item_indent: + item_indent = l[: len(l) - len(stripped)] or (enabled_indent + " ") + content = stripped[1:].strip() + parts = re.split(r"\s+-\s+", content) + new_lines = [f"{item_indent}- {parts[0]}"] + for p in parts[1:]: + new_lines.append(f"{item_indent}- {p}") + lines[idx : idx + 1] = new_lines + end_line += len(new_lines) - 1 + # Re-evaluate plugin presence after the rewrite. + has_plugin_already = False + for nl in lines[plugin_start + 1 : end_line]: + if nl.strip().startswith("-") and nl[len(item_indent):].strip() == f"- {plugin_strip}": + has_plugin_already = True + break + +# Idempotent fast path. +if has_plugin_already: + path.write_text("\n".join(lines) + "\n") + sys.exit(0) + +new_item_line = f"{item_indent or (enabled_indent + ' ')}- {plugin}" + +# Case 1: no plugins: block at all. +if plugin_start is None: if lines and lines[-1].strip(): lines.append("") - lines.extend(["plugins:", " enabled:", f" - {plugin}"]) -else: - out = [] - in_plugins = False - inserted = False - saw_enabled = False - for idx, line in enumerate(lines): - if line.startswith("plugins:"): - in_plugins = True - out.append(line) - continue - if in_plugins and line and not line.startswith((" ", "\t")): - if not saw_enabled and not inserted: - out.extend([" enabled:", f" - {plugin}"]) - inserted = True - in_plugins = False - out.append(line) - continue - if in_plugins and line.strip().startswith("enabled:") and "[]" in line: - saw_enabled = True - out.extend([" enabled:", f" - {plugin}"]) - inserted = True - continue - if in_plugins and line.strip() == "enabled:": - saw_enabled = True - out.append(line) - # Insert before the next sibling key or top-level key; if the list is - # empty this still creates a valid block. - out.append(f" - {plugin}") - inserted = True - continue - out.append(line) - if in_plugins and not saw_enabled and not inserted: - out.extend([" enabled:", f" - {plugin}"]) - lines = out + lines.append("plugins:") + lines.append(f"{enabled_indent or ' '}enabled:") + lines.append(new_item_line) + path.write_text("\n".join(lines) + "\n") + sys.exit(0) + +# Case 2: enabled: [] (inline empty) — replace with a block-style list. +if enabled_empty: + new_block = [ + f"{enabled_indent}enabled:", + new_item_line, + ] + lines[plugin_start + 1 : plugin_start + 2] = new_block + path.write_text("\n".join(lines) + "\n") + sys.exit(0) + +# Case 3: enabled: block exists but has no items yet. +if has_enabled and not item_indent and not enabled_empty: + for idx in range(plugin_start + 1, end_line): + if lines[idx].strip() == "enabled:": + lines.insert(idx + 1, new_item_line) + break + path.write_text("\n".join(lines) + "\n") + sys.exit(0) + +# Case 4: enabled: block with existing items — append at the end of the list +# at the matching indent. Also normalize any sibling items whose indent +# doesn't match (e.g. the original 2-space bug entry) so the file is left +# consistent. +insert_at = None +for idx in range(end_line - 1, plugin_start, -1): + l = lines[idx] + stripped = l.strip() + if stripped.startswith("-"): + if l != item_indent + stripped: + lines[idx] = item_indent + stripped + insert_at = idx + 1 + break +# Fallback: no item line found in the scan (shouldn't happen if has_enabled +# is True, but stay correct). Insert directly under the enabled: key. +if insert_at is None and has_enabled: + for idx in range(plugin_start + 1, end_line): + if lines[idx].strip() == "enabled:": + insert_at = idx + 1 + break +if insert_at is None: + # Couldn't locate a sensible insertion point; bail without writing to + # avoid corrupting the file further. + sys.exit(1) +lines.insert(insert_at, new_item_line) path.write_text("\n".join(lines) + "\n") PY if [[ -f "$backup" ]]; then