From 90d77d9e3840560c70dad75e244d4d7faf55b91a Mon Sep 17 00:00:00 2001 From: codejunkie99 Date: Thu, 23 Apr 2026 15:02:13 +0530 Subject: [PATCH] harden episodic writes: fcntl lock + fix pi skills orphan sync MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two pre-existing infrastructure bugs flagged during the PR #17 cross-model review, fixed here against master because they predate that PR and affect every harness. ## 1. Concurrent writes to AGENT_LEARNINGS.jsonl post_execution.py and on_failure.py both did plain `open(EPISODIC, "a")` → `f.write(json.dumps(entry) + "\n")`. POSIX O_APPEND makes single `write(2)` calls atomic only up to PIPE_BUF (4 KB on Linux/macOS). In practice most entries stay under that ceiling and the unlocked code never corrupts, but the `reflection` field is uncapped in log_execution and can easily exceed 4 KB on high-importance failure logs. Every downstream reader (auto_dream.py, cluster.py, context_budget.py, show.py) skips `json.JSONDecodeError` lines silently — so one over-PIPE_BUF interleave = one episodic entry gone with no signal. Fix: new `_episodic_io.append_jsonl()` helper that opens in append- binary mode (no Python text-mode buffering quirks) and wraps the write in `fcntl.flock(LOCK_EX)`. Shared by both writers. On platforms without fcntl (native Windows Python) behavior falls back to the pre-fix unlocked append; WSL, git-bash/Cygwin, macOS, Linux all have fcntl. Verified: 40 concurrent writers × 500 entries × 2 KB reflection each → 20,000 parseable lines, zero corruption. ## 2. pi install.sh silently leaves stale skills on re-install `ln -sfn src dest` where `dest` is a REAL directory (e.g. from an earlier copy-fallback install) silently creates `dest/` INSIDE the dir and exits 0. The existing `if ln -sfn; then` branch took the success path, the `rm -rf + cp` fallback never ran, and orphans stuck around forever. Verified on macOS, confirmed the symlink-inside-dir behavior. Fix: check `-L` (symlink) and `-d` (real dir) explicitly before calling `ln -sfn`, mirror the pattern used by the codex adapter (PR #16 follow-up). Existing symlink → cheap repoint. Real directory → rsync --delete when available, rm+cp otherwise. Non-existent → symlink or copy fallback. Same three-branch shape, no more silent wrong behavior. Verified: re-install after orphan-skill was added to a real-dir `.pi/skills` → rsync --delete removes the orphan. --- .agent/harness/hooks/_episodic_io.py | 45 ++++++++++++++++++++++++++ .agent/harness/hooks/on_failure.py | 6 ++-- .agent/harness/hooks/post_execution.py | 8 ++--- install.sh | 28 ++++++++++++---- 4 files changed, 72 insertions(+), 15 deletions(-) create mode 100644 .agent/harness/hooks/_episodic_io.py diff --git a/.agent/harness/hooks/_episodic_io.py b/.agent/harness/hooks/_episodic_io.py new file mode 100644 index 0000000..e434908 --- /dev/null +++ b/.agent/harness/hooks/_episodic_io.py @@ -0,0 +1,45 @@ +"""Cross-platform locked append for episodic JSONL writes. + +POSIX `write(2)` in O_APPEND mode is atomic for payloads up to PIPE_BUF +(4 KB on Linux, 512 B minimum per POSIX). Most episodic entries fit, +but failure entries with reflection + context + detail can exceed that, +and two harness hooks writing from the same process (or from two Pi +sessions on the same repo) can interleave bytes mid-line. Silent +corruption is worse than a visible error because every downstream +reader (`auto_dream.py`, `cluster.py`, `context_budget.py`, +`show.py`) skips `JSONDecodeError` lines without surfacing the loss. + +This module serializes appends with `fcntl.flock(LOCK_EX)` on POSIX. +On platforms without `fcntl` (native Windows Python) the lock is a +no-op and behavior matches the pre-lock baseline. WSL, git-bash via +Cygwin, macOS, and Linux all provide `fcntl`. +""" +import json +import os + +try: + import fcntl # POSIX + _HAVE_FLOCK = True +except ImportError: + _HAVE_FLOCK = False + + +def append_jsonl(path: str, entry: dict) -> dict: + """Serialize `entry` to one JSON line and append to `path`. + + Uses `open(..., "ab")` (append-binary) to bypass Python's text-mode + buffering and guarantee a single `write(2)` per call. `fcntl.flock` + provides cross-process mutual exclusion on POSIX. + """ + payload = (json.dumps(entry) + "\n").encode("utf-8") + os.makedirs(os.path.dirname(path), exist_ok=True) + with open(path, "ab") as f: + if _HAVE_FLOCK: + fcntl.flock(f.fileno(), fcntl.LOCK_EX) + try: + f.write(payload) + f.flush() + finally: + if _HAVE_FLOCK: + fcntl.flock(f.fileno(), fcntl.LOCK_UN) + return entry diff --git a/.agent/harness/hooks/on_failure.py b/.agent/harness/hooks/on_failure.py index 3a92c61..5312bc0 100644 --- a/.agent/harness/hooks/on_failure.py +++ b/.agent/harness/hooks/on_failure.py @@ -1,6 +1,7 @@ """Failures are learning. High pain score + rewrite flag after repeat offenses.""" import json, datetime, os from ._provenance import build_source +from ._episodic_io import append_jsonl ROOT = os.path.join(os.path.dirname(__file__), "..", "..") EPISODIC = os.path.join(ROOT, "memory/episodic/AGENT_LEARNINGS.jsonl") @@ -69,7 +70,4 @@ def on_failure(skill_name, action, error, context="", confidence=0.9, f"Flag for rewrite." ) entry["pain_score"] = 10 - os.makedirs(os.path.dirname(EPISODIC), exist_ok=True) - with open(EPISODIC, "a") as f: - f.write(json.dumps(entry) + "\n") - return entry + return append_jsonl(EPISODIC, entry) diff --git a/.agent/harness/hooks/post_execution.py b/.agent/harness/hooks/post_execution.py index 92bacf9..6f587f0 100644 --- a/.agent/harness/hooks/post_execution.py +++ b/.agent/harness/hooks/post_execution.py @@ -1,6 +1,7 @@ """Runs after every action. Appends a structured entry to episodic memory.""" -import json, datetime, os +import datetime, os from ._provenance import build_source +from ._episodic_io import append_jsonl ROOT = os.path.join(os.path.dirname(__file__), "..", "..") EPISODIC = os.path.join(ROOT, "memory/episodic/AGENT_LEARNINGS.jsonl") @@ -15,7 +16,6 @@ def log_execution(skill_name, action, result, success, reflection="", a higher value (e.g. 5) for high-importance successful operations so recurring patterns cross the dream-cycle promotion threshold (7.0). """ - os.makedirs(os.path.dirname(EPISODIC), exist_ok=True) if pain_score is None: pain_score = 2 if success else 7 entry = { @@ -31,6 +31,4 @@ def log_execution(skill_name, action, result, success, reflection="", "source": build_source(skill_name), "evidence_ids": list(evidence_ids) if evidence_ids else [], } - with open(EPISODIC, "a") as f: - f.write(json.dumps(entry) + "\n") - return entry + return append_jsonl(EPISODIC, entry) diff --git a/install.sh b/install.sh index b14ca78..a6259a0 100755 --- a/install.sh +++ b/install.sh @@ -133,15 +133,31 @@ case "$ADAPTER" in echo " + AGENTS.md" fi mkdir -p "$TARGET/.pi" - # symlink .pi/skills -> .agent/skills so pi sees the one true skill tree. - # ln -sfn atomically replaces an existing symlink; fall back to cp -R - # on filesystems that don't support symlinks (e.g. Windows without dev mode). + # Keep .pi/skills in sync with .agent/skills (the one true skill tree). + # Handle three shapes explicitly: `ln -sfn src dest` against a REAL + # directory silently creates `dest/` INSIDE the dir + # on macOS/Linux and exits 0, which would leave stale orphans forever. + # Check -L before -d so existing symlinks take the cheap repoint path, + # and reserve the rsync path for real dirs left from a prior copy + # fallback install. SKILLS_SRC="$(cd "$TARGET/.agent/skills" && pwd)" - if ln -sfn "$SKILLS_SRC" "$TARGET/.pi/skills" 2>/dev/null; then + SKILLS_DEST="$TARGET/.pi/skills" + if [[ -L "$SKILLS_DEST" ]]; then + ln -sfn "$SKILLS_SRC" "$SKILLS_DEST" + echo " + .pi/skills -> $SKILLS_SRC" + elif [[ -d "$SKILLS_DEST" ]]; then + if command -v rsync >/dev/null 2>&1; then + rsync -a --delete "$SKILLS_SRC/" "$SKILLS_DEST/" + echo " ~ synced .agent/skills → .pi/skills (rsync --delete)" + else + rm -rf "$SKILLS_DEST" + cp -R "$SKILLS_SRC" "$SKILLS_DEST" + echo " ~ replaced .pi/skills with current .agent/skills (no rsync)" + fi + elif ln -sfn "$SKILLS_SRC" "$SKILLS_DEST" 2>/dev/null; then echo " + .pi/skills -> $SKILLS_SRC" else - rm -rf "$TARGET/.pi/skills" - cp -R "$SKILLS_SRC" "$TARGET/.pi/skills" + cp -R "$SKILLS_SRC" "$SKILLS_DEST" echo " + .pi/skills (copy; symlink not supported here)" fi ;;