From 70f737a1bfe633330496742d9982b95864fa09c0 Mon Sep 17 00:00:00 2001 From: JaidenSy Date: Wed, 22 Jul 2026 17:42:57 -0700 Subject: [PATCH 1/3] feat: guarantee a Progress.md note per run + Nous-style post-task learning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two follow-ups to #18 so a completed run leaves durable, trustworthy state. 1. DETERMINISTIC Progress.md note (the guarantee). On every terminal outcome the completion path now writes the run into the project's Progress.md under a dedicated "## Hermes Run Log" section (newest-first, human sections untouched). This is plain Python on the completion path — "done" GUARANTEES a note exists; it no longer depends on a sub-agent choosing to write one. (Nothing wrote Progress.md before — agents only ever wrote their own step output notes.) 2. NOUS-STYLE post-task learning (the nice-to-have). On success, a non-blocking background reviewer distills the run's step notes via LOCAL Ollama (free, zero Claude tokens — same path as the resume handoff) into a Progress.md "Learnings" entry, and — only if a genuinely reusable procedure emerged — a skill CANDIDATE staged in ~/hermes/skill-candidates/ (gitignored) with a Telegram ping to review. Candidates are NEVER auto-installed into ~/.claude/skills (globally active in every session) — Jaiden promotes by hand. Refactor: extracted the shared local-Ollama call into `_ollama_generate` (handoff + reviewer both use it). Tests: 149 pass / 1 skip (+5 — deterministic writer create/append/newest-first + review parsing). Co-Authored-By: Claude Opus 4.8 --- .gitignore | 1 + hermes.py | 231 ++++++++++++++++++++++++++++++++---- tests/test_progress_note.py | 97 +++++++++++++++ 3 files changed, 307 insertions(+), 22 deletions(-) create mode 100644 tests/test_progress_note.py diff --git a/.gitignore b/.gitignore index f148045..38ade80 100644 --- a/.gitignore +++ b/.gitignore @@ -4,5 +4,6 @@ __pycache__/ logs/ runs/ tasks/ +skill-candidates/ config/telegram_state.json config/imessage_state.json diff --git a/hermes.py b/hermes.py index a88233e..879d385 100644 --- a/hermes.py +++ b/hermes.py @@ -45,6 +45,31 @@ def load_config(): return yaml.safe_load(f) +def _ollama_generate(prompt: str, timeout: int = 120, model: str = "llama3.1:8b") -> str: + """POST a prompt to the local Ollama server; return the response text ("" on any + failure). Shared by the resume-handoff and post-task-review writers — both distill + text locally so they cost zero Claude tokens.""" + try: + import json as _json + import urllib.request as _urllib + + payload = _json.dumps({"model": model, "prompt": prompt, "stream": False}).encode() + req = _urllib.Request( + "http://localhost:11434/api/generate", + data=payload, + headers={"Content-Type": "application/json"}, + method="POST", + ) + with _urllib.urlopen(req, timeout=timeout) as resp: + return _json.loads(resp.read().decode()).get("response", "").strip() + except TimeoutError: + log.error(f"Ollama generate timed out after {timeout}s") + return "" + except OSError: + log.error("Ollama not reachable — is it running?") + return "" + + def write_handoff_via_ollama( task: str, session_name: str, @@ -86,24 +111,12 @@ def write_handoff_via_ollama( Be specific and factual. Only include what is supported by the partial output above.""" - try: - import json as _json - import urllib.request as _urllib - - payload = _json.dumps({"model": "llama3.1:8b", "prompt": prompt, "stream": False}).encode() - req = _urllib.Request( - "http://localhost:11434/api/generate", - data=payload, - headers={"Content-Type": "application/json"}, - method="POST", - ) - with _urllib.urlopen(req, timeout=120) as resp: - response_text = _json.loads(resp.read().decode()).get("response", "").strip() - - if not response_text: - log.error("Ollama handoff generation returned empty response") - return "" + response_text = _ollama_generate(prompt, timeout=120) + if not response_text: + log.error("Ollama handoff generation returned empty response") + return "" + try: handoff_dir = Path.home() / "Documents" / "RaphBrain" / "Projects" / "handoffs" handoff_dir.mkdir(parents=True, exist_ok=True) handoff_path = handoff_dir / f"{session_name}-handoff.md" @@ -116,11 +129,8 @@ def write_handoff_via_ollama( ) log.info(f"Handoff written: {handoff_path}") return str(handoff_path) - except TimeoutError: - log.error("Ollama handoff timed out after 120s") - return "" - except OSError: - log.error("Ollama not found — is it installed?") + except OSError as exc: + log.error(f"Failed to write handoff file: {exc}") return "" @@ -292,6 +302,163 @@ def _append_pipeline_to_daily_note( log.warning(f"[daily-note] Failed to write to daily note: {exc}") +RAPHBRAIN_PROJECTS_DIR = Path.home() / "Documents" / "RaphBrain" / "Projects" +HERMES_RUN_LOG_HEADER = "## Hermes Run Log" +# Auto-drafted skills land here for review — NEVER written straight into +# ~/.claude/skills, which is globally active in every Claude Code session. Jaiden +# promotes a candidate by hand once he's read it. +SKILL_CANDIDATES_DIR = Path.home() / "hermes" / "skill-candidates" + + +def _prepend_under_runlog(project: str, entry: str) -> None: + """Insert `entry` newest-first under the '## Hermes Run Log' header in the + project's Progress.md, creating the file + section if missing. Shared by the + deterministic run-note and the post-task learnings writer.""" + proj_cap = project.capitalize() + progress_path = RAPHBRAIN_PROJECTS_DIR / proj_cap / "Progress.md" + if progress_path.exists(): + content = progress_path.read_text() + if HERMES_RUN_LOG_HEADER + "\n" in content: + head, _, rest = content.partition(HERMES_RUN_LOG_HEADER + "\n") + content = head + HERMES_RUN_LOG_HEADER + "\n" + entry + "\n" + rest + else: + content = content.rstrip() + f"\n\n{HERMES_RUN_LOG_HEADER}\n{entry}" + progress_path.write_text(content) + else: + progress_path.parent.mkdir(parents=True, exist_ok=True) + progress_path.write_text(f"# {proj_cap} — Progress\n\n{HERMES_RUN_LOG_HEADER}\n{entry}") + + +def _append_pipeline_to_progress_note( + project: str, + branch: str, + final_status: str, + duration: str, + pr_url: str = "", + failed_step: str = "", + reason: str = "", + done_count: int = 0, + step_count: int = 0, +) -> None: + """Deterministically record a run's outcome in the project's Progress.md. + + Runs in Python on the completion path, so 'done' GUARANTEES a note exists — it + never depends on a sub-agent remembering to write one. Entries go under a + dedicated '## Hermes Run Log' section so human-curated sections are untouched.""" + try: + stamp = datetime.now().strftime("%Y-%m-%d %H:%M") + emoji = {"done": "✅", "failed": "❌", "aborted": "🛑"}.get(final_status, "🏁") + lines = [f"### {stamp} — {emoji} {project}/{branch or '(no branch)'} — {final_status}"] + lines.append( + f"- {done_count}/{step_count} steps · {duration}" if step_count else f"- {duration}" + ) + if pr_url: + lines.append(f"- PR: {pr_url}") + if final_status == "failed" and failed_step: + lines.append(f"- Failed at `{failed_step}`" + (f" — {reason}" if reason else "")) + _prepend_under_runlog(project, "\n".join(lines) + "\n") + log.info(f"[progress-note] Recorded {final_status} run for {project} in Progress.md") + except Exception as exc: + log.warning(f"[progress-note] Failed to write Progress.md: {exc}") + + +_POST_TASK_REVIEW_PROMPT = """You are reviewing a COMPLETED automation run to capture reusable knowledge for next time. Be factual — use ONLY what the notes below support. + +ORIGINAL TASK: {task} +PROJECT: {project} BRANCH: {branch} + +WHAT THE AGENTS PRODUCED (their handoff notes): +{context} + +Respond with EXACTLY these two sections and nothing else: + +## Learnings +2-4 short bullets: what worked, and any gotcha worth remembering. Specific and factual. + +## Skill Candidate +Only if this run followed a genuinely reusable, repeatable procedure worth reusing on future tasks. If it was one-off or too project-specific, write exactly: NO_SKILL +If reusable, output ONLY a skill in this exact format: +--- +name: +description: +--- + +""" + + +def _split_review(text: str) -> tuple[str, str]: + """Split an Ollama review into (learnings, skill_candidate). skill is "" when the + model wrote NO_SKILL or omitted the section.""" + skill = "" + pre = text + if "## Skill Candidate" in text: + pre, _, post = text.partition("## Skill Candidate") + if "NO_SKILL" not in post.upper(): + skill = post.strip() + learnings = pre.split("## Learnings", 1)[1].strip() if "## Learnings" in pre else pre.strip() + return learnings, skill + + +def _skill_name(skill_md: str) -> str: + """Pull the kebab `name:` out of a skill candidate's frontmatter, else "".""" + for line in skill_md.splitlines(): + if line.strip().startswith("name:"): + return line.split("name:", 1)[1].strip().strip("\"'") + return "" + + +def spawn_post_task_review(engine, run_id: str, project: str, branch: str, hermes_config) -> None: + """Nous-style 'learning': after a DONE run, distill it (local Ollama, free, + non-blocking) into a Learnings entry in Progress.md + an optional skill CANDIDATE + staged in ~/hermes/skill-candidates for review. Never self-installs a skill — + an 8B model's draft is a suggestion, not a globally-active skill. + ponytail: local model + human promote-gate; good enough for a draft.""" + + def _worker(): + try: + run = engine.get_run(run_id) + task = run.get("task_raw", "") or run.get("task", "") + notes = [] + for step in run.get("pipeline", []): + op = step.get("output_path") + if not op: + continue + note_path = Path.home() / "Documents" / "RaphBrain" / op + if note_path.exists(): + notes.append(f"### {step.get('role')}\n{note_path.read_text()[:1500]}") + context = ("\n\n".join(notes))[:6000] or "(no step notes captured)" + out = _ollama_generate( + _POST_TASK_REVIEW_PROMPT.format( + task=task[:400], project=project, branch=branch, context=context + ), + timeout=180, + ) + if not out: + return + learnings, skill = _split_review(out) + if learnings: + stamp = datetime.now().strftime("%Y-%m-%d %H:%M") + _prepend_under_runlog(project, f"#### {stamp} — 🧠 Learnings\n{learnings}\n") + candidate_path = "" + if skill: + SKILL_CANDIDATES_DIR.mkdir(parents=True, exist_ok=True) + name = _skill_name(skill) or f"{project}-{datetime.now().strftime('%Y%m%d-%H%M')}" + candidate_path = str(SKILL_CANDIDATES_DIR / f"{name}.md") + Path(candidate_path).write_text(skill + "\n") + _send_reply( + hermes_config, + f"🧠 {project}: drafted a skill candidate — review to promote:\n{candidate_path}", + ) + log.info( + f"[post-task-review] {project}: learnings={'y' if learnings else 'n'} " + f"skill_candidate={'y' if candidate_path else 'n'}" + ) + except Exception as exc: + log.warning(f"[post-task-review] failed for {project}: {exc}") + + threading.Thread(target=_worker, daemon=True, name=f"hermes-review-{run_id}").start() + + def _format_duration(started_iso: str, ended_iso: str) -> str: """Return a human-readable duration like '2m 14s'.""" try: @@ -607,6 +774,26 @@ def _finalize_step_failure(reason: str) -> None: _append_pipeline_to_daily_note( project, branch, final_status, total_duration, failed_step, failed_reason ) + + # Guarantee a Progress.md entry for every terminal run — deterministic Python, + # not dependent on a sub-agent choosing to write one. + _append_pipeline_to_progress_note( + project, + branch, + final_status, + total_duration, + pr_url or "", + failed_step, + failed_reason, + done_count, + step_count, + ) + + # Nous-style "learning": on success, distill the run into a Progress.md + # Learnings note + an optional skill CANDIDATE (local Ollama, free, staged + # for review — never auto-installed). Non-blocking. + if final_status == "done": + spawn_post_task_review(engine, run_id, project, branch, hermes_config) except Exception as exc: log.error(f"[orchestrate] Failed to send completion notification: {exc}") diff --git a/tests/test_progress_note.py b/tests/test_progress_note.py new file mode 100644 index 0000000..23f8396 --- /dev/null +++ b/tests/test_progress_note.py @@ -0,0 +1,97 @@ +""" +test_progress_note.py — the deterministic Progress.md writer + post-task-review +parsing. The write path is what makes 'done' GUARANTEE a note exists, so it gets a +real check; RAPHBRAIN_PROJECTS_DIR is patched to a tmp so the real vault is untouched. +""" + +import sys +import tempfile +import unittest +from pathlib import Path +from unittest.mock import patch + +sys.path.insert(0, str(Path.home() / "hermes")) +import hermes # noqa: E402 + + +class TestProgressNote(unittest.TestCase): + def setUp(self): + self._tmp = tempfile.TemporaryDirectory() + self._dir = Path(self._tmp.name) / "Projects" + self._p = patch.object(hermes, "RAPHBRAIN_PROJECTS_DIR", self._dir) + self._p.start() + + def tearDown(self): + self._p.stop() + self._tmp.cleanup() + + def _progress(self, proj="Arbiter"): + return self._dir / proj / "Progress.md" + + def test_creates_file_and_section_when_missing(self): + # done ⇒ a note MUST exist even if Progress.md didn't. + hermes._append_pipeline_to_progress_note( + "arbiter", + "feature/x", + "done", + "12m", + pr_url="http://pr/1", + done_count=4, + step_count=4, + ) + body = self._progress().read_text() + self.assertIn(hermes.HERMES_RUN_LOG_HEADER, body) + self.assertIn("✅ arbiter/feature/x — done", body) + self.assertIn("4/4 steps · 12m", body) + self.assertIn("PR: http://pr/1", body) + + def test_failed_entry_records_step_and_reason(self): + hermes._append_pipeline_to_progress_note( + "arbiter", + "", + "failed", + "3m", + failed_step="deployer", + reason="no PR marker", + done_count=2, + step_count=4, + ) + body = self._progress().read_text() + self.assertIn("❌ arbiter/(no branch) — failed", body) + self.assertIn("Failed at `deployer` — no PR marker", body) + + def test_newest_first_and_preserves_human_sections(self): + p = self._progress() + p.parent.mkdir(parents=True, exist_ok=True) + p.write_text( + "# Arbiter — Progress\n\n## Status\nhand-curated\n\n" + f"{hermes.HERMES_RUN_LOG_HEADER}\n### old entry\n" + ) + hermes._append_pipeline_to_progress_note( + "arbiter", "feature/new", "done", "1m", done_count=1, step_count=1 + ) + body = p.read_text() + self.assertIn("hand-curated", body) # human section survived + # newest entry sits above the old one under the header + self.assertLess(body.index("feature/new"), body.index("### old entry")) + + def test_split_review_no_skill(self): + learnings, skill = hermes._split_review( + "## Learnings\n- did a thing\n\n## Skill Candidate\nNO_SKILL" + ) + self.assertIn("did a thing", learnings) + self.assertEqual(skill, "") + + def test_split_review_extracts_skill_and_name(self): + text = ( + "## Learnings\n- reusable flow\n\n## Skill Candidate\n" + "---\nname: deploy-vercel-site\ndescription: ship a static site\n---\n1. build\n2. deploy" + ) + learnings, skill = hermes._split_review(text) + self.assertIn("reusable flow", learnings) + self.assertIn("name: deploy-vercel-site", skill) + self.assertEqual(hermes._skill_name(skill), "deploy-vercel-site") + + +if __name__ == "__main__": + unittest.main() From 9f31772c55b850cb816d3ad0cc9e8950316af847 Mon Sep 17 00:00:00 2001 From: JaidenSy Date: Wed, 22 Jul 2026 18:04:55 -0700 Subject: [PATCH 2/3] harden per Fable review: path-injection, folder routing, write race, no-skill gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Applies the Fable code-review findings on this PR: - H1 (security): _skill_name now sanitizes the model-supplied name to a safe bare filename (`[^a-z0-9-]`→`-`). An 8B `name: ../../.claude/skills/x` (or absolute path) would otherwise escape the staging dir into the globally-active skills folder — defeating the whole review gate. Staging also re-checks the resolved path stays inside skill-candidates/. - M2 (spam): stage a candidate ONLY when the section parses as a real frontmatter skill with a name — no timestamp fallback. A paraphrased "No skill needed" or prose no longer stages a garbage file or pings Jaiden on a plain success. Reviewer prompt also tightened to DEFAULT TO NO_SKILL. - H2 (routing): _prepend_under_runlog resolves the vault folder via the registry (raphbrain_dir), not project.capitalize() — hyphenated projects (finance-tracker) no longer write to an orphan folder nothing reads. - M1 (race): _prepend_under_runlog runs under a module lock — a slow review thread and the next run's deterministic write can't clobber each other's Progress.md. - L1/L2/nits: line-anchored header match, utf-8 on reads/writes, _ollama_generate catches HTTPError/JSONDecodeError with the real message. Tests: 153 pass / 1 skip (+4): name sanitization incl. traversal, paraphrased no-skill, registry-folder routing, header-without-trailing-newline. Co-Authored-By: Claude Opus 4.8 --- hermes.py | 98 +++++++++++++++++++++++++------------ tests/test_progress_note.py | 62 ++++++++++++++++++++--- 2 files changed, 121 insertions(+), 39 deletions(-) diff --git a/hermes.py b/hermes.py index 879d385..79720d1 100644 --- a/hermes.py +++ b/hermes.py @@ -6,6 +6,7 @@ import subprocess import logging +import re import time import threading import yaml @@ -65,8 +66,10 @@ def _ollama_generate(prompt: str, timeout: int = 120, model: str = "llama3.1:8b" except TimeoutError: log.error(f"Ollama generate timed out after {timeout}s") return "" - except OSError: - log.error("Ollama not reachable — is it running?") + except (OSError, ValueError) as exc: + # OSError covers connect-refused AND HTTPError (e.g. model not pulled); + # ValueError covers a non-JSON body. Honor the "" -on-any-failure contract. + log.error(f"Ollama generate failed: {exc}") return "" @@ -310,23 +313,38 @@ def _append_pipeline_to_daily_note( SKILL_CANDIDATES_DIR = Path.home() / "hermes" / "skill-candidates" +_PROGRESS_LOCK = threading.Lock() +_RUN_LOG_HEADER_RE = re.compile(r"(?m)^" + re.escape(HERMES_RUN_LOG_HEADER) + r"$") + + def _prepend_under_runlog(project: str, entry: str) -> None: """Insert `entry` newest-first under the '## Hermes Run Log' header in the project's Progress.md, creating the file + section if missing. Shared by the - deterministic run-note and the post-task learnings writer.""" - proj_cap = project.capitalize() - progress_path = RAPHBRAIN_PROJECTS_DIR / proj_cap / "Progress.md" - if progress_path.exists(): - content = progress_path.read_text() - if HERMES_RUN_LOG_HEADER + "\n" in content: - head, _, rest = content.partition(HERMES_RUN_LOG_HEADER + "\n") - content = head + HERMES_RUN_LOG_HEADER + "\n" + entry + "\n" + rest + deterministic run-note and the post-task learnings writer. + + Held under a lock: a slow post-task-review thread and the next run's + deterministic write can hit the same file's read-modify-write at once, and the + loser would silently clobber the other — including the deterministic guarantee.""" + # Registry is the source of truth for the vault folder — capitalize() misroutes + # hyphenated projects (finance-tracker → a Finance-tracker orphan nothing reads). + proj = resolve_project(project) + dir_name = (proj.raphbrain_dir if proj else None) or project.capitalize() + progress_path = RAPHBRAIN_PROJECTS_DIR / dir_name / "Progress.md" + with _PROGRESS_LOCK: + if progress_path.exists(): + content = progress_path.read_text(encoding="utf-8") + m = _RUN_LOG_HEADER_RE.search(content) # line-anchored — not any prose match + if m: + cut = m.end() + (1 if content[m.end() : m.end() + 1] == "\n" else 0) + content = content[:cut] + entry + "\n" + content[cut:] + else: + content = content.rstrip() + f"\n\n{HERMES_RUN_LOG_HEADER}\n{entry}" + progress_path.write_text(content, encoding="utf-8") else: - content = content.rstrip() + f"\n\n{HERMES_RUN_LOG_HEADER}\n{entry}" - progress_path.write_text(content) - else: - progress_path.parent.mkdir(parents=True, exist_ok=True) - progress_path.write_text(f"# {proj_cap} — Progress\n\n{HERMES_RUN_LOG_HEADER}\n{entry}") + progress_path.parent.mkdir(parents=True, exist_ok=True) + progress_path.write_text( + f"# {dir_name} — Progress\n\n{HERMES_RUN_LOG_HEADER}\n{entry}", encoding="utf-8" + ) def _append_pipeline_to_progress_note( @@ -376,8 +394,8 @@ def _append_pipeline_to_progress_note( 2-4 short bullets: what worked, and any gotcha worth remembering. Specific and factual. ## Skill Candidate -Only if this run followed a genuinely reusable, repeatable procedure worth reusing on future tasks. If it was one-off or too project-specific, write exactly: NO_SKILL -If reusable, output ONLY a skill in this exact format: +DEFAULT TO NO_SKILL. Propose a skill ONLY if this run followed a genuinely reusable, repeatable procedure you would run again on a DIFFERENT project. One-off, project-specific, or "just did the task" work is NOT a skill — when in doubt, write exactly: NO_SKILL +If (and only if) it is truly reusable, output ONLY a skill in this exact format: --- name: description: @@ -387,23 +405,31 @@ def _append_pipeline_to_progress_note( def _split_review(text: str) -> tuple[str, str]: - """Split an Ollama review into (learnings, skill_candidate). skill is "" when the - model wrote NO_SKILL or omitted the section.""" + """Split an Ollama review into (learnings, skill_candidate). skill is "" unless the + section actually parses as a frontmatter skill — a paraphrased "No skill needed" + or plain prose must NOT stage a candidate (the model rarely emits the literal + NO_SKILL token, so the gate is 'looks like a skill', not 'absence of NO_SKILL').""" skill = "" pre = text if "## Skill Candidate" in text: pre, _, post = text.partition("## Skill Candidate") - if "NO_SKILL" not in post.upper(): - skill = post.strip() + post = post.strip() + if post.startswith("---") and "NO_SKILL" not in post.upper(): + skill = post learnings = pre.split("## Learnings", 1)[1].strip() if "## Learnings" in pre else pre.strip() return learnings, skill def _skill_name(skill_md: str) -> str: - """Pull the kebab `name:` out of a skill candidate's frontmatter, else "".""" + """Pull the kebab `name:` from a skill candidate's frontmatter, sanitized to a safe + bare filename ("" if none). Sanitizing is load-bearing security, not cosmetics: the + name comes from an 8B model fed agent notes, and it becomes a path — an unsanitized + `../../.claude/skills/x` or absolute path would escape the staging dir into the + globally-active skills folder, defeating the whole review gate.""" for line in skill_md.splitlines(): if line.strip().startswith("name:"): - return line.split("name:", 1)[1].strip().strip("\"'") + raw = line.split("name:", 1)[1].strip().strip("\"'") + return re.sub(r"[^a-z0-9-]+", "-", raw.lower()).strip("-")[:60] return "" @@ -425,7 +451,9 @@ def _worker(): continue note_path = Path.home() / "Documents" / "RaphBrain" / op if note_path.exists(): - notes.append(f"### {step.get('role')}\n{note_path.read_text()[:1500]}") + notes.append( + f"### {step.get('role')}\n{note_path.read_text(encoding='utf-8')[:1500]}" + ) context = ("\n\n".join(notes))[:6000] or "(no step notes captured)" out = _ollama_generate( _POST_TASK_REVIEW_PROMPT.format( @@ -440,15 +468,21 @@ def _worker(): stamp = datetime.now().strftime("%Y-%m-%d %H:%M") _prepend_under_runlog(project, f"#### {stamp} — 🧠 Learnings\n{learnings}\n") candidate_path = "" - if skill: + # Stage ONLY a candidate that parses as a real skill with a safe name — no + # timestamp fallback. A nameless/prose blob is not a skill, so it never + # stages a garbage file or pings Jaiden on a plain successful run. + name = _skill_name(skill) if skill else "" + if skill and skill.startswith("---") and name: SKILL_CANDIDATES_DIR.mkdir(parents=True, exist_ok=True) - name = _skill_name(skill) or f"{project}-{datetime.now().strftime('%Y%m%d-%H%M')}" - candidate_path = str(SKILL_CANDIDATES_DIR / f"{name}.md") - Path(candidate_path).write_text(skill + "\n") - _send_reply( - hermes_config, - f"🧠 {project}: drafted a skill candidate — review to promote:\n{candidate_path}", - ) + candidate = SKILL_CANDIDATES_DIR / f"{name}.md" + # Extra belt: the resolved path must stay inside the staging dir. + if candidate.resolve().parent == SKILL_CANDIDATES_DIR.resolve(): + candidate.write_text(skill + "\n", encoding="utf-8") + candidate_path = str(candidate) + _send_reply( + hermes_config, + f"🧠 {project}: drafted a skill candidate — review to promote:\n{candidate_path}", + ) log.info( f"[post-task-review] {project}: learnings={'y' if learnings else 'n'} " f"skill_candidate={'y' if candidate_path else 'n'}" diff --git a/tests/test_progress_note.py b/tests/test_progress_note.py index 23f8396..4547531 100644 --- a/tests/test_progress_note.py +++ b/tests/test_progress_note.py @@ -1,7 +1,8 @@ """ test_progress_note.py — the deterministic Progress.md writer + post-task-review -parsing. The write path is what makes 'done' GUARANTEE a note exists, so it gets a -real check; RAPHBRAIN_PROJECTS_DIR is patched to a tmp so the real vault is untouched. +parsing/sanitizing. The write path is what makes 'done' GUARANTEE a note exists, so +it gets a real check; RAPHBRAIN_PROJECTS_DIR is patched to a tmp so the real vault is +untouched, and resolve_project is patched so folder resolution is deterministic. """ import sys @@ -14,14 +15,25 @@ import hermes # noqa: E402 +class _FakeProj: + def __init__(self, raphbrain_dir): + self.raphbrain_dir = raphbrain_dir + self.repo = "~/Projects/x" + + class TestProgressNote(unittest.TestCase): def setUp(self): self._tmp = tempfile.TemporaryDirectory() self._dir = Path(self._tmp.name) / "Projects" self._p = patch.object(hermes, "RAPHBRAIN_PROJECTS_DIR", self._dir) self._p.start() + # Default: no registry hit → falls back to project.capitalize() (deterministic, + # no dependency on the real ~/Projects layout). H2 test overrides this. + self._rp = patch.object(hermes, "resolve_project", return_value=None) + self._rp.start() def tearDown(self): + self._rp.stop() self._p.stop() self._tmp.cleanup() @@ -72,26 +84,62 @@ def test_newest_first_and_preserves_human_sections(self): ) body = p.read_text() self.assertIn("hand-curated", body) # human section survived - # newest entry sits above the old one under the header - self.assertLess(body.index("feature/new"), body.index("### old entry")) + self.assertLess(body.index("feature/new"), body.index("### old entry")) # newest first + + def test_header_without_trailing_newline_reuses_section(self): + # a hand-edited file whose header is the last line (no trailing \n) must not + # get a duplicate section appended. + p = self._progress() + p.parent.mkdir(parents=True, exist_ok=True) + p.write_text(f"# Arbiter — Progress\n\n{hermes.HERMES_RUN_LOG_HEADER}") + hermes._append_pipeline_to_progress_note( + "arbiter", "b", "done", "1m", step_count=1, done_count=1 + ) + self.assertEqual(p.read_text().count(hermes.HERMES_RUN_LOG_HEADER), 1) + + def test_note_uses_registry_folder_not_capitalize(self): + # hyphenated project must land in the registry folder, not a capitalize() orphan. + with patch.object(hermes, "resolve_project", return_value=_FakeProj("FinanceTracker")): + hermes._append_pipeline_to_progress_note( + "finance-tracker", "b", "done", "1m", done_count=1, step_count=1 + ) + self.assertTrue((self._dir / "FinanceTracker" / "Progress.md").exists()) + self.assertFalse((self._dir / "Finance-tracker" / "Progress.md").exists()) + + # --- post-task review parsing / sanitizing --- - def test_split_review_no_skill(self): + def test_split_review_literal_no_skill(self): learnings, skill = hermes._split_review( "## Learnings\n- did a thing\n\n## Skill Candidate\nNO_SKILL" ) self.assertIn("did a thing", learnings) self.assertEqual(skill, "") - def test_split_review_extracts_skill_and_name(self): + def test_split_review_paraphrased_no_skill_stages_nothing(self): + # the 8B model rarely emits the literal token — prose must not become a skill. + for prose in ("No skill needed here.", "We could maybe reuse the deploy step."): + _, skill = hermes._split_review(f"## Learnings\n- x\n\n## Skill Candidate\n{prose}") + self.assertEqual(skill, "", prose) + + def test_split_review_extracts_real_frontmatter_skill(self): text = ( "## Learnings\n- reusable flow\n\n## Skill Candidate\n" "---\nname: deploy-vercel-site\ndescription: ship a static site\n---\n1. build\n2. deploy" ) learnings, skill = hermes._split_review(text) self.assertIn("reusable flow", learnings) - self.assertIn("name: deploy-vercel-site", skill) + self.assertTrue(skill.startswith("---")) self.assertEqual(hermes._skill_name(skill), "deploy-vercel-site") + def test_skill_name_sanitizes_path_traversal(self): + # the name is model output used as a path — it must never escape the staging dir. + self.assertEqual( + hermes._skill_name("name: ../../.claude/skills/evil"), "claude-skills-evil" + ) + self.assertEqual(hermes._skill_name("name: /etc/passwd"), "etc-passwd") + self.assertEqual(hermes._skill_name('name: "My Cool Skill!"'), "my-cool-skill") + self.assertEqual(hermes._skill_name("no frontmatter"), "") + if __name__ == "__main__": unittest.main() From 20caba23cf25d3855295443cbeb8ae537bcf9214 Mon Sep 17 00:00:00 2001 From: JaidenSy Date: Wed, 22 Jul 2026 18:48:20 -0700 Subject: [PATCH 3/3] fix: header-glue + duplicate-section bug in _prepend_under_runlog (2nd review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fresh review caught the no-trailing-newline fix was incomplete AND its test masked it: when the header is the file's last line with no trailing newline, the entry glued onto it ('## Hermes Run Log### …') and the next run appended a 2nd section. Now insert a separator; test does two inserts + asserts one section. Also adds the missing end-to-end staging tests for spawn_post_task_review (real skill stages + pings; NO_SKILL stages nothing), and documents cross-process as out of scope. Tests: 155 pass / 1 skip (+2). Co-Authored-By: Claude Opus 4.8 --- hermes.py | 18 ++++++++--- tests/test_progress_note.py | 61 ++++++++++++++++++++++++++++++++++--- 2 files changed, 69 insertions(+), 10 deletions(-) diff --git a/hermes.py b/hermes.py index 79720d1..78cd9d7 100644 --- a/hermes.py +++ b/hermes.py @@ -322,9 +322,10 @@ def _prepend_under_runlog(project: str, entry: str) -> None: project's Progress.md, creating the file + section if missing. Shared by the deterministic run-note and the post-task learnings writer. - Held under a lock: a slow post-task-review thread and the next run's - deterministic write can hit the same file's read-modify-write at once, and the - loser would silently clobber the other — including the deterministic guarantee.""" + Held under a lock so the daemon's two writer threads (a slow post-task-review + thread and the next run's deterministic write) can't clobber each other's + read-modify-write. Cross-PROCESS writers (a `claude -p` sub-agent, Obsidian sync) + are out of scope — different sections, infrequent — and not synchronized here.""" # Registry is the source of truth for the vault folder — capitalize() misroutes # hyphenated projects (finance-tracker → a Finance-tracker orphan nothing reads). proj = resolve_project(project) @@ -335,8 +336,15 @@ def _prepend_under_runlog(project: str, entry: str) -> None: content = progress_path.read_text(encoding="utf-8") m = _RUN_LOG_HEADER_RE.search(content) # line-anchored — not any prose match if m: - cut = m.end() + (1 if content[m.end() : m.end() + 1] == "\n" else 0) - content = content[:cut] + entry + "\n" + content[cut:] + end = m.end() + if content[end : end + 1] == "\n": + content = content[: end + 1] + entry + "\n" + content[end + 1 :] + else: + # Header is the file's last line with no trailing newline — insert + # one so it stays on its own line. Without this the entry glues on + # ("## Hermes Run Log### …") and the next run can't match the header, + # appending a SECOND section and breaking the single-section rule. + content = content[:end] + "\n" + entry + "\n" + content[end:] else: content = content.rstrip() + f"\n\n{HERMES_RUN_LOG_HEADER}\n{entry}" progress_path.write_text(content, encoding="utf-8") diff --git a/tests/test_progress_note.py b/tests/test_progress_note.py index 4547531..2ea673a 100644 --- a/tests/test_progress_note.py +++ b/tests/test_progress_note.py @@ -7,6 +7,7 @@ import sys import tempfile +import threading import unittest from pathlib import Path from unittest.mock import patch @@ -21,6 +22,14 @@ def __init__(self, raphbrain_dir): self.repo = "~/Projects/x" +class _FakeEngine: + def __init__(self, run): + self._run = run + + def get_run(self, _run_id): + return self._run + + class TestProgressNote(unittest.TestCase): def setUp(self): self._tmp = tempfile.TemporaryDirectory() @@ -86,16 +95,25 @@ def test_newest_first_and_preserves_human_sections(self): self.assertIn("hand-curated", body) # human section survived self.assertLess(body.index("feature/new"), body.index("### old entry")) # newest first - def test_header_without_trailing_newline_reuses_section(self): - # a hand-edited file whose header is the last line (no trailing \n) must not - # get a duplicate section appended. + def test_header_without_trailing_newline_stays_one_section(self): + # a hand-edited file whose header is the last line (no trailing \n): the entry + # must NOT glue onto the header, and a SECOND run must not append a 2nd section. p = self._progress() p.parent.mkdir(parents=True, exist_ok=True) p.write_text(f"# Arbiter — Progress\n\n{hermes.HERMES_RUN_LOG_HEADER}") hermes._append_pipeline_to_progress_note( - "arbiter", "b", "done", "1m", step_count=1, done_count=1 + "arbiter", "one", "done", "1m", step_count=1, done_count=1 + ) + hermes._append_pipeline_to_progress_note( + "arbiter", "two", "done", "1m", step_count=1, done_count=1 ) - self.assertEqual(p.read_text().count(hermes.HERMES_RUN_LOG_HEADER), 1) + body = p.read_text() + self.assertEqual(body.count(hermes.HERMES_RUN_LOG_HEADER), 1) # still one section + self.assertIn( + f"{hermes.HERMES_RUN_LOG_HEADER}\n", body + ) # header on its own line, not glued + self.assertNotIn(f"{hermes.HERMES_RUN_LOG_HEADER}###", body) + self.assertLess(body.index("two"), body.index("one")) # newest-first preserved def test_note_uses_registry_folder_not_capitalize(self): # hyphenated project must land in the registry folder, not a capitalize() orphan. @@ -140,6 +158,39 @@ def test_skill_name_sanitizes_path_traversal(self): self.assertEqual(hermes._skill_name('name: "My Cool Skill!"'), "my-cool-skill") self.assertEqual(hermes._skill_name("no frontmatter"), "") + # --- end-to-end staging path (security-relevant: model output → file on disk) --- + + def _run_review(self, review_out, staging): + with ( + patch.object(hermes, "SKILL_CANDIDATES_DIR", staging), + patch.object(hermes, "_ollama_generate", return_value=review_out), + patch.object(hermes, "_send_reply") as reply, + ): + engine = _FakeEngine({"task_raw": "do a thing", "pipeline": []}) + hermes.spawn_post_task_review(engine, "rid", "arbiter", "feat/x", {}) + for t in list(threading.enumerate()): + if t.name == "hermes-review-rid": + t.join(timeout=5) + return reply + + def test_review_stages_real_skill_and_pings(self): + staging = self._dir.parent / "skill-candidates" + reply = self._run_review( + "## Learnings\n- did stuff\n\n## Skill Candidate\n" + "---\nname: my-skill\ndescription: does a thing\n---\n1. step\n", + staging, + ) + self.assertEqual([f.name for f in staging.glob("*.md")], ["my-skill.md"]) + reply.assert_called_once() + + def test_review_no_skill_stages_nothing_and_no_ping(self): + staging = self._dir.parent / "skill-candidates" + reply = self._run_review( + "## Learnings\n- did stuff\n\n## Skill Candidate\nNO_SKILL", staging + ) + self.assertFalse(staging.exists() and list(staging.glob("*.md"))) + reply.assert_not_called() + if __name__ == "__main__": unittest.main()