fix(curator): keep the skill ledger size-bounded (dedup rewrite + oldest-entry trim)
(cherry picked from commit 42572ebf207d411edd534055b1d750d91efcd4f7)
This commit is contained in:
committed by
kshitij
parent
4d9dd96e39
commit
71932b150b
@@ -1458,6 +1458,10 @@ DEFAULT_CONFIG = {
|
||||
# curator ledger` / `rollback <entry-id>`. Never a gate — failures can't block.
|
||||
# See #79686.
|
||||
"ledger": True,
|
||||
# Size cap for that ledger: once the file grows past this, the next append rewrites it
|
||||
# through the unchanged-file dedup and, if still over, drops the oldest entries (0 = keep
|
||||
# the ledger append-only forever, the previous behaviour).
|
||||
"ledger_max_bytes": 5 * 1024 * 1024,
|
||||
},
|
||||
|
||||
# Curator — background maintenance of AGENT-CREATED skills (never hub-installed): marks
|
||||
|
||||
@@ -600,3 +600,107 @@ def test_backup_fill_ignores_tar_path_traversal(ledger_env):
|
||||
)
|
||||
# Malicious members are not.
|
||||
assert not any(p.endswith("evil.md") or p.endswith("outside.md") for p in paths)
|
||||
import json
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
|
||||
|
||||
def _append_padded(skill_ledger, action: str, pad: str, n: int = 1) -> None:
|
||||
"""Append *n* entries padded with evidence text so the ledger file grows fast."""
|
||||
for _ in range(n):
|
||||
skill_ledger.append_entry(action, "my-skill", before=[], after=[], evidence={"pad": pad})
|
||||
|
||||
|
||||
def test_auto_compact_triggers_at_threshold(ledger_env, monkeypatch):
|
||||
"""Crossing skills.ledger_max_bytes rewrites the ledger through the delta
|
||||
dedup: identical before/after manifests shrink to nothing while ids and
|
||||
entry order survive."""
|
||||
from tools import skill_ledger
|
||||
|
||||
import hermes_cli.config as _cfg
|
||||
|
||||
monkeypatch.setattr(_cfg, "load_config", lambda *a, **k: {
|
||||
"skills": {"ledger_max_bytes": 8192}})
|
||||
|
||||
for i in range(3): # pre-delta-style entries: identical fat manifests on both sides
|
||||
fat = [{"path": f"my-skill/f{i}j{j}.md", "sha256": "a" * 64} for j in range(40)]
|
||||
skill_ledger.append_entry("patch", "my-skill", before=fat, after=list(fat))
|
||||
|
||||
# the maintenance sweep fired mid-append: the file stays under the cap
|
||||
assert skill_ledger.ledger_path().stat().st_size <= 8192
|
||||
rows = skill_ledger.list_entries()
|
||||
assert len(rows) == 3, "dedup alone must reach the cap — nothing trimmed"
|
||||
assert all(r["before"] == [] and r["after"] == [] for r in rows), (
|
||||
"identical manifests must be dropped by compaction"
|
||||
)
|
||||
|
||||
|
||||
def test_trim_oldest_when_still_over_cap(ledger_env, monkeypatch):
|
||||
"""When compaction alone cannot reach the cap (every entry genuinely
|
||||
differs), the oldest entries are dropped until it fits — newest entries
|
||||
survive, malformed lines are kept verbatim."""
|
||||
from tools import skill_ledger
|
||||
|
||||
import hermes_cli.config as _cfg
|
||||
|
||||
monkeypatch.setattr(_cfg, "load_config", lambda *a, **k: {
|
||||
"skills": {"ledger_max_bytes": 8192}})
|
||||
|
||||
newest_id = None
|
||||
for i in range(5):
|
||||
before = [{"path": f"my-skill/old{i}.md", "sha256": f"{i}" * 64}]
|
||||
after = [{"path": f"my-skill/new{i}.md", "sha256": f"{i + 1}" * 64}]
|
||||
newest_id = skill_ledger.append_entry(
|
||||
"edit", "my-skill", before=before, after=after,
|
||||
evidence={"pad": "y" * 2048})
|
||||
with open(skill_ledger.ledger_path(), "a", encoding="utf-8") as fh:
|
||||
fh.write("{not json at all\n")
|
||||
|
||||
skill_ledger._maintain_size()
|
||||
|
||||
rows = skill_ledger.list_entries()
|
||||
assert len(rows) < 5, "oldest entries must be trimmed when compaction is not enough"
|
||||
assert rows[0]["id"] == newest_id, "the newest entry always survives"
|
||||
# malformed lines are never parsed away — they stay in the file verbatim
|
||||
raw = skill_ledger.ledger_path().read_text(encoding="utf-8")
|
||||
assert "{not json at all" in raw
|
||||
assert skill_ledger.ledger_path().stat().st_size <= 8192
|
||||
|
||||
|
||||
def test_disabled_threshold_never_touches_the_file(ledger_env, monkeypatch):
|
||||
"""ledger_max_bytes: 0 keeps the append-only contract: no
|
||||
rewrite ever happens, however large the file is."""
|
||||
from tools import skill_ledger
|
||||
|
||||
import hermes_cli.config as _cfg
|
||||
|
||||
monkeypatch.setattr(_cfg, "load_config", lambda *a, **k: {
|
||||
"skills": {"ledger": True, "ledger_max_bytes": 0}})
|
||||
|
||||
_append_padded(skill_ledger, "patch", "z" * 4096, n=4)
|
||||
raw = skill_ledger.ledger_path().read_text(encoding="utf-8")
|
||||
|
||||
skill_ledger._maintain_size()
|
||||
|
||||
assert skill_ledger.ledger_path().read_text(encoding="utf-8") == raw
|
||||
|
||||
|
||||
def test_maintenance_failure_never_blocks_append(ledger_env, monkeypatch):
|
||||
"""Telemetry contract: a broken maintenance sweep logs and leaves the
|
||||
appended entries on disk."""
|
||||
from tools import skill_ledger
|
||||
|
||||
import hermes_cli.config as _cfg
|
||||
|
||||
monkeypatch.setattr(_cfg, "load_config", lambda *a, **k: {
|
||||
"skills": {"ledger_max_bytes": 8192}})
|
||||
|
||||
def _boom(*a, **k):
|
||||
raise OSError("disk full")
|
||||
|
||||
monkeypatch.setattr(skill_ledger, "compact_ledger", _boom)
|
||||
# appends past the threshold hit the broken sweep and must survive it
|
||||
_append_padded(skill_ledger, "patch", "w" * 4096, n=6)
|
||||
|
||||
assert len(skill_ledger.list_entries()) == 6
|
||||
|
||||
@@ -96,6 +96,21 @@ def ledger_enabled() -> bool:
|
||||
return True
|
||||
|
||||
|
||||
def _max_ledger_bytes() -> int:
|
||||
"""Config ``skills.ledger_max_bytes`` (default 5 MB, 0 disables): above it the
|
||||
next append triggers the maintenance sweep instead of growing the file forever."""
|
||||
try:
|
||||
from hermes_cli.config import cfg_get, load_config
|
||||
return int(
|
||||
cfg_get(
|
||||
load_config(), "skills", "ledger_max_bytes", default=5 * 1024 * 1024
|
||||
)
|
||||
)
|
||||
except Exception as e: # pragma: no cover — best-effort config read
|
||||
logger.debug("skill_ledger: config read failed (%s); defaulting to 5 MB", e)
|
||||
return 5 * 1024 * 1024
|
||||
|
||||
|
||||
def _rel_posix(path: Path | str, root: Path) -> Optional[str]:
|
||||
"""POSIX path of ``path`` relative to ``root`` (both normalized), or None when outside."""
|
||||
try:
|
||||
@@ -290,6 +305,7 @@ def append_entry(
|
||||
path.parent.mkdir(parents=True, exist_ok=True)
|
||||
with open(path, "a", encoding="utf-8") as fh:
|
||||
fh.write(json.dumps(entry, ensure_ascii=False) + "\n")
|
||||
_maintain_size()
|
||||
return entry["id"]
|
||||
except Exception as e:
|
||||
logger.warning("skill_ledger: failed to append entry (%s) — mutation unaffected", e)
|
||||
@@ -309,6 +325,57 @@ def _read_ledger(what: str, *, quiet_missing: bool = False) -> Optional[bytes]:
|
||||
return None
|
||||
|
||||
|
||||
def _maintain_size() -> None:
|
||||
"""Keep the ledger bounded: once it crosses ``skills.ledger_max_bytes`` run the
|
||||
delta-dedup rewrite, and if genuinely-divergent entries still exceed the cap,
|
||||
drop the oldest ones until it fits. Best-effort telemetry, never a gate: any
|
||||
failure is logged and the just-appended entry stays on disk."""
|
||||
max_bytes = _max_ledger_bytes()
|
||||
if max_bytes <= 0:
|
||||
return
|
||||
try:
|
||||
if ledger_path().stat().st_size <= max_bytes:
|
||||
return
|
||||
_, _, size_after = compact_ledger()
|
||||
if size_after > max_bytes:
|
||||
_trim_oldest(max_bytes)
|
||||
gc_blobs()
|
||||
except Exception as e:
|
||||
logger.warning(
|
||||
"skill_ledger: maintenance sweep failed (%s) — ledger left as-is", e
|
||||
)
|
||||
|
||||
|
||||
def _trim_oldest(max_bytes: int) -> None:
|
||||
"""Rewrite the ledger without its oldest parsed entries until it is at most
|
||||
*max_bytes*; the newest entry always survives, malformed lines are kept verbatim."""
|
||||
path = ledger_path()
|
||||
raw = _read_ledger("trim skipped")
|
||||
if raw is None:
|
||||
return
|
||||
lines = raw.decode("utf-8").splitlines()
|
||||
newest_json = None
|
||||
for line in reversed(lines):
|
||||
if line.strip():
|
||||
newest_json = line
|
||||
break
|
||||
kept: List[str] = []
|
||||
size = 2 # trailing newline + rounding slack
|
||||
for line in reversed(lines):
|
||||
addition = len(line.encode("utf-8")) + 1
|
||||
if kept and size + addition > max_bytes:
|
||||
break
|
||||
kept.append(line)
|
||||
size += addition
|
||||
kept.reverse()
|
||||
data = ("\n".join(kept) + "\n").encode("utf-8") if kept else b""
|
||||
if newest_json is not None and newest_json not in kept:
|
||||
data = (newest_json + "\n").encode("utf-8")
|
||||
tmp = path.with_name(path.name + ".trim.tmp")
|
||||
tmp.write_bytes(data)
|
||||
os.replace(tmp, path)
|
||||
|
||||
|
||||
def compact_ledger() -> Tuple[int, int, int]:
|
||||
"""Rewrite the ledger with every entry's unchanged paths dropped (see ``_delta``); ids, order and
|
||||
rollback semantics are preserved. Returns ``(entries, bytes_before, bytes_after)``. Atomic: the
|
||||
|
||||
@@ -175,6 +175,13 @@ skills:
|
||||
ledger: false
|
||||
```
|
||||
|
||||
The file is also size-bounded: once it grows past `skills.ledger_max_bytes` (default 5 MB), the next mutation first rewrites it through the unchanged-file dedup (exactly what `hermes curator ledger --compact` does) and, if genuinely-divergent entries still exceed the cap, drops the oldest ones — the newest entry and any malformed lines always survive, and the sweep frees blobs nothing references anymore. Set it to `0` to keep the ledger append-only forever.
|
||||
|
||||
```yaml
|
||||
skills:
|
||||
ledger_max_bytes: 5242880 # 0 = never auto-maintain
|
||||
```
|
||||
|
||||
## Archive TTL purge
|
||||
|
||||
Archived skills are kept forever by default. If you want `~/.hermes/skills/.archive/` bounded, set a TTL and purge explicitly — purging never runs automatically, and every purged skill is captured into the ledger (with blobs) first, so even a purge leaves an auditable, recoverable trail:
|
||||
|
||||
Reference in New Issue
Block a user