mirror of
https://github.com/msitarzewski/agency-agents.git
synced 2026-09-08 03:00:51 +03:00
fix(install): preserve list-item indent when adding the Hermes plugin; regression test + CI (#714)
ensure_hermes_plugin_enabled() hardcoded a 2-space indent for the inserted `- agency-agents-router` line. Hermes writes plugins.enabled items at 4 spaces, so the new line and the next existing item collapsed into one plain scalar ("agency-agents-router - disk-cleanup") and every previously enabled plugin silently dropped (#839, and #689 diagnosed the same in July). The inserter now matches the existing item indent (default 4), appends after existing entries so order is preserved, and is idempotent on re-run. scripts/check-hermes-config-rewrite.{sh,py} runs seven regression cases; a small workflow runs it on every PR.
Reproduced on main with a 4-space config and verified fixed with this patch; installer suite 36/0; the regression script passes 7/7.
Fixes #839. Closes #689 (same fix, proposed first by @harshsinghmp).
Co-Authored-By: Harsh Singh <32476777+harshsinghmp@users.noreply.github.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This commit is contained in:
Jonathan J Wagner
co-authored by
Harsh Singh
Claude Fable 5.1
Harsh Singh
Claude Fable 5.1
parent
1454492577
commit
647c8baa42
@@ -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
|
||||
@@ -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())
|
||||
Executable
+19
@@ -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
|
||||
+139
-63
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user