diff --git a/plugins/memory_distill.py b/plugins/memory_distill.py index 41fc7eb..b440244 100644 --- a/plugins/memory_distill.py +++ b/plugins/memory_distill.py @@ -41,14 +41,49 @@ import re import sys -# adapters + shared session lib live under plugins/; memory_init (decode_root) -# lives at the repo root. Put both on the path, mirroring memory_search.py. +# adapters + shared session lib live under plugins/; the engine modules +# (memory_init's decode_root, memory_reindex's index/page enumeration) live at +# the repo root. Put both on the path, mirroring memory_search.py. _PLUGINS_DIR = os.path.dirname(os.path.realpath(__file__)) _ROOT_DIR = os.path.dirname(_PLUGINS_DIR) sys.path.insert(0, _PLUGINS_DIR) -sys.path.insert(0, _ROOT_DIR) + +# One level up from plugins/ IS the engine root under both install shapes the +# dispatcher produces (it resolves its own symlink before joining `plugins/`). +# Probe anyway, and fall back to $OKFMEM_ENGINE / ~/okfmem, so a plugins/ dir +# copied out of the repo prints one actionable line instead of a bare +# ModuleNotFoundError traceback. Same probe shape as +# skills/okfmem-curate/scripts/inventory.py, which hit this on #56. +_ENGINE_MODULE = "memory_reindex.py" + + +def _find_engine_root(): + for cand in (_ROOT_DIR, os.environ.get("OKFMEM_ENGINE"), + os.path.expanduser("~/okfmem")): + if cand and os.path.isfile(os.path.join(cand, _ENGINE_MODULE)): + return cand + return None + + +_ENGINE_ROOT = _find_engine_root() or _ROOT_DIR +if _ENGINE_ROOT not in sys.path: + sys.path.insert(0, _ENGINE_ROOT) + from adapters import agy, claude_code # noqa: E402 -from memory_init import decode_root # noqa: E402 (reuse the FS-probing decoder) +try: + from memory_init import decode_root # noqa: E402 (FS-probing decoder) + # The single home for "which files are indexes, which are pages" and for + # pointer parsing (#54/#56). Restating either predicate here is the defect + # #57 exists to remove, so both are imported rather than reimplemented. + from memory_reindex import (index_files, page_files, # noqa: E402 + parse_pointers, read_lines) +except ImportError as exc: # pragma: no cover - broken install shape only + sys.stderr.write( + "okfmem distill: okfmem engine not found (%s) — no %s beside plugins/, " + "in $OKFMEM_ENGINE, or in ~/okfmem. Run distill through the engine's " + "dispatcher instead: python3 ~/okfmem/okfmem distill\n" + % (exc, _ENGINE_MODULE)) + sys.exit(2) DEFAULT_STORE = os.environ.get("OKFMEM_STORE", os.path.expanduser("~/okfmem-store")) @@ -72,9 +107,6 @@ # decisions/topics live in what the user and the assistant SAY. DEFAULT_ROLES = frozenset({"user", "assistant"}) -# Pages/index files that are not durable knowledge pages. -_SKIP_PAGE_NAMES = {"MEMORY.md", "STATE.md", "CONTEXT.md", "SESSIONS.md", "README.md"} - # Token = a word-ish run bounded by alphanumerics, with tech-y inner chars kept # (node.js, memory_init.py, github.com), lowercased. Requiring alnum edges drops # trailing punctuation ("step." -> "step") and stray separators. A token must @@ -157,7 +189,7 @@ def _emit_run(run): # --------------------------------------------------------------------------- def _page_token_set(store, proj, page_name, memory_hook=""): """Token set representing what an existing page already covers: its slug + - H1 title + its MEMORY.md hook line. Used to suppress already-covered topics.""" + H1 title + its index hook line. Used to suppress already-covered topics.""" tokens = set(_tokenize(page_name[:-3].replace("-", " ").replace("_", " "))) path = os.path.join(store, "projects", proj, page_name) try: @@ -172,19 +204,37 @@ def _page_token_set(store, proj, page_name, memory_hook=""): return tokens -def _memory_hooks(store, proj): - """Map slug.md -> its MEMORY.md hook text (the `- [Title](slug.md) — hook` - line), best-effort. Empty when no MEMORY.md.""" +def _memory_hooks(mem_dir): + """Map ``slug.md`` -> the index pointer line that names it, best-effort. + + Walks **every** ``MEMORY*.md`` in the dir, not just the root index. Under + #53's lane routing most pointers live in a ``MEMORY-.md``, so reading + only the root handed every lane-pointed page an empty hook — a + systematically thinner token set than its root-pointed siblings, and the + asymmetry grows with every lane created (#57). + + Parsing is `memory_reindex.parse_pointers`, the one pointer parser (#54), + so **both** syntaxes resolve: the rich ``- [Title](slug.md) — hook`` and + the bare ``- slug.md — hook`` a lane index actually uses. The local regex + this replaces saw only the rich form. `parse_pointers` returns a 1-based + line *number* rather than the line text, so the hook is read back out of + the same ``lines`` list it parsed. + + External/cross-directory targets are dropped — they name nothing in this + dir. First index wins (`index_files` puts the root first), so a page + pointed at from two indexes still gets a deterministic hook. Empty when the + dir holds no index at all. + """ hooks = {} - path = os.path.join(store, "projects", proj, "MEMORY.md") - try: - with open(path, "r", encoding="utf-8", errors="ignore") as f: - text = f.read() - except OSError: - return hooks - for ln in text.splitlines(): - for m in re.finditer(r"[(/]([A-Za-z0-9._-]+)\.md\)", ln): - hooks[m.group(1) + ".md"] = ln + for name in index_files(mem_dir): + try: + lines, _costs = read_lines(os.path.join(mem_dir, name)) + except OSError: + continue + for p in parse_pointers(lines): + if p.syntax == "external": + continue + hooks.setdefault(p.target, lines[p.line - 1]) return hooks @@ -192,7 +242,13 @@ def load_coverage(store, projects=None): """{store_project: [token_set_per_page]} for the given (or all) projects. A project with a dir but no pages yields []; a project absent from the store - yields no key at all (callers decide whether to still report it).""" + yields no key at all (callers decide whether to still report it). + + Pages come from `memory_reindex.page_files`, so a lane index is never + tokenized as a page. That mattered: a lane index is a dense bag of every + hook in its lane, so feeding it here made almost any topic in that lane + look already-covered and silently suppressed genuine proposals (#57). + """ proj_root = os.path.join(store, "projects") coverage = {} if not os.path.isdir(proj_root): @@ -204,15 +260,9 @@ def load_coverage(store, projects=None): pdir = os.path.join(proj_root, proj) if not os.path.isdir(pdir): continue - hooks = _memory_hooks(store, proj) - page_sets = [] - for fn in sorted(os.listdir(pdir)): - if not fn.endswith(".md") or fn in _SKIP_PAGE_NAMES: - continue - if fn.startswith("ck_"): - continue - page_sets.append(_page_token_set(store, proj, fn, hooks.get(fn, ""))) - coverage[proj] = page_sets + hooks = _memory_hooks(pdir) + coverage[proj] = [_page_token_set(store, proj, fn, hooks.get(fn, "")) + for fn in page_files(pdir)] return coverage diff --git a/skills/okfmem-curate/SKILL.md b/skills/okfmem-curate/SKILL.md index db34860..3918ec0 100644 --- a/skills/okfmem-curate/SKILL.md +++ b/skills/okfmem-curate/SKILL.md @@ -48,23 +48,64 @@ Five phases. Phases 4 and 5 are gated on explicit user approval — never execut ### Phase 1: Resolve target directory -Compute the memory dir from the current working directory: +If the user passed an explicit path argument (e.g. `/okfmem-curate audit `), use it as +`MEM_DIR` directly and skip the probe below — this is the escape hatch for running outside a +repo. + +Otherwise, ask the engine rather than re-deriving the encoded path by hand: `encode_root` also +encodes the drive colon on Windows, so a hand-rolled `sed`/replace resolves to the wrong +directory there, and it can't see registry overrides for a renamed project either. ```bash -# project slug = absolute cwd with / replaced by - -SLUG=$(pwd -P | sed 's|/|-|g') -MEM_DIR="$HOME/.claude/projects/$SLUG/memory" +# Rung 1 — read-only, never prompts. Try bare `okfmem` first, then fall back to +# the engine's own path: a manual install that leaves `~/.local/bin` off `PATH` +# is supported, and every later phase in this skill already calls the engine as +# `python3 ~/okfmem/okfmem ...` for that reason. +LINK_STATE="$(okfmem init --project-link-state 2>/dev/null)" \ + || LINK_STATE="$(python3 ~/okfmem/okfmem init --project-link-state 2>/dev/null)" +# "linked " | "unlinked " | "not-a-repo" | "no-claude" | "" (unreachable) + +# `read` consumes only the FIRST line and leaves NAME **empty** when the engine +# printed a bare state: `not-a-repo`/`no-claude` carry no name at all. Do not +# reach for `${LINK_STATE#* }` here — on a single-word value it hands back that +# word unchanged, i.e. `not-a-repo` silently becomes the "project name". +read -r STATE NAME <<< "$LINK_STATE" + +STORE="${OKFMEM_STORE:-$HOME/okfmem-store}" +MEM_DIR="" # meaningful ONLY for `linked` — see the table +if [ "$STATE" = "linked" ] && [ -n "$NAME" ]; then + MEM_DIR="$STORE/projects/$NAME" +fi ``` -If `$MEM_DIR` doesn't exist, tell the user and stop. If the user passed an explicit path argument, use it instead. +Branch on `$STATE`: + +| State | What Phase 1 does | +|---|---| +| `linked ` | proceed with the resolved `$MEM_DIR` | +| `unlinked ` | stop — tell the user this repo has no memory link; the fix is `okfmem init` from the repo root, then re-run `/okfmem-curate` | +| `not-a-repo` | stop — tell the user to `cd` to the project root first (or pass an explicit path) | +| `no-claude` | stop — tell the user the harness isn't installed here; nothing to curate | +| anything else — `$STATE` empty, or a word not in the four rows above | stop — the engine could not be reached (`okfmem` is off `PATH` *and* absent from `~/okfmem/okfmem`), or it wrote something unexpected to stdout. Report the raw `$LINK_STATE` and have the user run `python3 ~/okfmem/okfmem init --project-link-state` directly to see the real error on stderr, or re-invoke `/okfmem-curate` with an explicit path argument | + +**`$MEM_DIR` stays empty in every row but the first, and that is the point.** +Building it unconditionally is what made a missing name collapse to +`$STORE/projects/` — the store's *projects root*, a real directory that passes +any `-d` guard — so Phases 2–3 would report on the wrong tree and Phase 4 would +ask the user to approve deletions derived from it. Never proceed past this phase +with `$MEM_DIR` unset; there is no safe default for it. -Detect whether the dir is symlinked into `~/okfmem-store` (so deletions are git-recoverable). Run: +Detect whether the dir is git-backed (so deletions are recoverable). Run: ```bash -readlink "$MEM_DIR" 2>/dev/null +# Test the property, not a proxy for it. `$MEM_DIR` is now the symlink's TARGET +# inside the store, not the `~/.claude/projects//memory` symlink — so +# `readlink` prints nothing and exits 1 here, which would report "not +# recoverable" on every correctly linked, fully git-backed store. Ask git. +git -C "${MEM_DIR:?Phase 1 did not resolve a memory dir — do not fall back to a default}" rev-parse --show-toplevel 2>/dev/null ``` -If the link points into a git-backed location, surface that to the user as the recovery mechanism. If not, *say so explicitly* — recovery is harder. +Non-empty output is the enclosing repo — surface it to the user as the recovery mechanism (it is the `` in the Recovery section's `git -C restore .`). Empty output means the dir is genuinely not under git: *say so explicitly* — recovery is harder. This also works for the explicit-path escape hatch, which `readlink` never handled. ### Phase 2: Inventory (deterministic) @@ -76,7 +117,7 @@ dominant block is the finding that decides whether the remedy is "tighten a few hooks" or "split a lane"; total file size alone never shows it: ```bash -python3 ~/okfmem/okfmem reindex --report "$MEM_DIR" +python3 ~/okfmem/okfmem reindex --report "${MEM_DIR:?Phase 1 did not resolve a memory dir — do not fall back to a default}" ``` **If the `MEMORY.md` row reads `OVER`, the restructure trigger has fired @@ -96,7 +137,7 @@ enforces ≤150 chars at write time; this is the check that catches what slipped through): ```bash -python3 ~/okfmem/okfmem reindex --budget-check "$MEM_DIR" +python3 ~/okfmem/okfmem reindex --budget-check "${MEM_DIR:?Phase 1 did not resolve a memory dir — do not fall back to a default}" ``` It walks **every** index, not just the root — after a lane split (or once @@ -122,7 +163,7 @@ Lead the curation report (Phase 4) with these numbers: `MEMORY.md` / for per-file age, link status, and heuristic flags: ```bash -python3 ~/okfmem/skills/okfmem-curate/scripts/inventory.py "$MEM_DIR" +python3 ~/okfmem/skills/okfmem-curate/scripts/inventory.py "${MEM_DIR:?Phase 1 did not resolve a memory dir — do not fall back to a default}" ``` Page count and on-disk total bytes are **not auto-loaded and cost zero @@ -223,8 +264,8 @@ Once approved: 6. Run the verification block: ```bash -python3 ~/okfmem/okfmem reindex --verify "$MEM_DIR"; echo "verify exit: $?" -python3 ~/okfmem/okfmem reindex --report "$MEM_DIR" # after-numbers for step 8 +python3 ~/okfmem/okfmem reindex --verify "${MEM_DIR:?Phase 1 did not resolve a memory dir — do not fall back to a default}"; echo "verify exit: $?" +python3 ~/okfmem/okfmem reindex --report "${MEM_DIR:?Phase 1 did not resolve a memory dir — do not fall back to a default}" # after-numbers for step 8 ``` `--verify` walks **every** `MEMORY*.md` (not just the root index) and accepts diff --git a/skills/okfmem-save/SKILL.md b/skills/okfmem-save/SKILL.md index f251e88..c0f6470 100644 --- a/skills/okfmem-save/SKILL.md +++ b/skills/okfmem-save/SKILL.md @@ -73,14 +73,41 @@ PROJECT_NAME="$(basename "$PROJECT_ROOT")" # Ask the engine whether THIS repo is wired, instead of re-deriving the encoded # path by hand: `encode_root` also encodes the drive colon on Windows, so a # hand-rolled sed/replace resolves to the wrong directory there. -LINK_STATE="$(okfmem init --project-link-state)" # "linked " | "unlinked " | "not-a-repo" | "no-claude" +# Rung 1 — read-only, never prompts. Same engine-path hedge used for `okfmem +# pull` below and `okfmem sync` in Step 7: bare `okfmem` when it's on PATH, else +# the engine's own path (a manual install may leave `~/.local/bin` off `PATH`). +LINK_STATE="$(okfmem init --project-link-state 2>/dev/null)" \ + || LINK_STATE="$(python3 ~/okfmem/okfmem init --project-link-state 2>/dev/null)" +# "linked " | "unlinked " | "not-a-repo" | "no-claude" | "" (unreachable) + +# `read` consumes only the FIRST line and leaves NAME **empty** when the engine +# printed a bare state: `not-a-repo`/`no-claude` carry no name at all. Do not +# reach for `${LINK_STATE#* }` here — on a single-word value it hands back that +# word unchanged, i.e. `not-a-repo` silently becomes the "project name". +read -r STATE NAME <<< "$LINK_STATE" # The probe resolves the project name through the registry (honouring renames), # so take the name from it rather than assuming basename == project. STORE="${OKFMEM_STORE:-$HOME/okfmem-store}" -MEMORY_DIR="$STORE/projects/$(echo "$LINK_STATE" | awk '{print $2}')" +MEMORY_DIR="" # meaningful ONLY when STATE is `linked` +if [ "$STATE" = "linked" ] && [ -n "$NAME" ]; then + MEMORY_DIR="$STORE/projects/$NAME" +fi ``` +**`$MEMORY_DIR` stays empty for every state but `linked`, and that is +load-bearing — this skill *writes*.** Building it unconditionally lets a missing +name collapse to `$STORE/projects/`, the store's *projects root*: a real +directory that passes any `-d` guard, so `STATE.md` and this session's memory +pages would be written there, where nothing auto-loads them and they pollute the +store next to the per-project dirs. **Never write anything with `$MEMORY_DIR` +empty** — there is no safe default for it. If `$STATE` is empty or is none of the +four states the probe can print, the engine could not be reached (`okfmem` off +`PATH` *and* absent from `~/okfmem/okfmem`) or it wrote something unexpected to +stdout: stop, report the raw `$LINK_STATE`, and have the user run +`python3 ~/okfmem/okfmem init --project-link-state` directly to see the real +error on stderr. + **If the probe says `unlinked`, stop and fix that first** — this repo has no memory link, so anything you write would land in a directory the agent never auto-loads. Tell the user plainly, then run: ```bash @@ -219,7 +246,7 @@ Fill each section from the session. **Stamp `modified:` with the current wall-cl **`STATE.md` has a ceiling: 8192 bytes** (~2.2k tokens) — the same auto-load budget `okfmem reindex` checks `MEMORY.md` against (`memory_reindex.STATE_BUDGET_BYTES`; #52). Generous headroom for a bounded, six-section, single-session snapshot, so this is a signal something drifted (a section grew a changelog instead of staying a pointer), not a routine concern. After writing, spot-check it: ```bash -okfmem reindex --report "$MEMORY_DIR" # or: python3 ~/okfmem/okfmem reindex --report "$MEMORY_DIR" +okfmem reindex --report "${MEMORY_DIR:?Step 1 did not resolve a memory dir — do not fall back to a default}" # or: python3 ~/okfmem/okfmem reindex --report "${MEMORY_DIR:?Step 1 did not resolve a memory dir — do not fall back to a default}" ``` Look at the `STATE.md` row's Status column. If it reads `OVER`, report it in Step 8 — advisory, not a rewrite gate. diff --git a/tests/test_distill.py b/tests/test_distill.py index 04f2a15..49baa8f 100644 --- a/tests/test_distill.py +++ b/tests/test_distill.py @@ -182,8 +182,11 @@ def test_deterministic_across_runs(): # load_coverage # --------------------------------------------------------------------------- def _write(path, text): + # newline="\n" pinned: text mode would otherwise translate to CRLF on + # Windows, so a fixture written here would differ byte-for-byte between the + # two CI legs. os.makedirs(os.path.dirname(path), exist_ok=True) - with open(path, "w", encoding="utf-8") as f: + with open(path, "w", encoding="utf-8", newline="\n") as f: f.write(text) @@ -203,6 +206,97 @@ def test_load_coverage_reads_pages_and_skips_index_files(tmp_path): assert "scanner" in page # from the MEMORY.md hook +# --------------------------------------------------------------------------- +# load_coverage across lane indexes (#57) +# --------------------------------------------------------------------------- +def _lane_store(tmp_path): + """A store project shaped the way #53's lane routing leaves one: a root + index that only ROUTES, two lane indexes carrying every real pointer (one + rich, one bare, one of each syntax), three durable pages, plus the two + kinds of file that are never pages (STATE.md, a retired ck_ snapshot). + + Every path is composed with os.path.join, so the fixture is identical on + both CI legs; nothing here asserts a byte count. + """ + store = str(tmp_path) + pdir = os.path.join(store, "projects", "demoproj") + _write(os.path.join(pdir, "MEMORY.md"), + "# MEMORY — demoproj\n\n" + "- MEMORY-parser.md — covers the parser lane\n" + "- MEMORY-tooling.md — covers the tooling lane\n") + _write(os.path.join(pdir, "MEMORY-parser.md"), + "# MEMORY — parser lane\n\n" + "- [Rich pointer page](rich-page.md) — quaternion hook wording\n" + "- bare-page.md — kaleidoscope hook wording\n") + _write(os.path.join(pdir, "MEMORY-tooling.md"), + "# MEMORY — tooling lane\n\n" + "- [Third page](third-page.md) — obelisk hook wording\n") + for slug, title in (("rich-page.md", "Rich pointer page"), + ("bare-page.md", "Bare pointer page"), + ("third-page.md", "Third page")): + _write(os.path.join(pdir, slug), + "---\ntype: project\n---\n\n# %s\n\nbody\n" % title) + _write(os.path.join(pdir, "STATE.md"), "# state\n") + _write(os.path.join(pdir, "ck_2026-01-02_snap.md"), "# retired snapshot\n") + return store, pdir + + +def test_load_coverage_enumerates_pages_via_reindex_not_a_local_rule(tmp_path): + """A lane index is an index, not a durable page (#57). + + Pre-change the local `_SKIP_PAGE_NAMES` filter knew only the ROOT index, so + this store yielded 5 page sets rather than 3 — MEMORY-parser.md and + MEMORY-tooling.md tokenized as if they were pages. That matters because a + lane index is a dense bag of every hook in its lane, so it makes almost any + topic in that lane read as already-covered. + """ + from memory_reindex import page_files + store, pdir = _lane_store(tmp_path) + cov = md.load_coverage(store) + assert len(cov["demoproj"]) == len(page_files(pdir)) == 3 + # A lane index's own token set carries both halves of its slug plus its H1. + assert not any({"memory", "parser"} <= ps for ps in cov["demoproj"]) + assert not any({"memory", "tooling"} <= ps for ps in cov["demoproj"]) + # Retired ck_ snapshots stay excluded (already true; must not regress). + assert not any({"retired", "snapshot"} <= ps for ps in cov["demoproj"]) + + +def test_load_coverage_hooks_come_from_lane_indexes_in_both_syntaxes(tmp_path): + """A page whose pointer lives ONLY in a lane index still gets its hook, in + either pointer syntax (#57). + + Pre-change `_memory_hooks` read only the root index — which here holds no + page pointer at all — so all three of these pages got `""` and a + systematically thinner token set than a root-pointed sibling. The bare form + was doubly missed: the local regex matched only `](slug.md)`. + """ + store, _pdir = _lane_store(tmp_path) + cov = md.load_coverage(store) + assert any("quaternion" in ps for ps in cov["demoproj"]) # lane, rich + assert any("kaleidoscope" in ps for ps in cov["demoproj"]) # lane, bare + assert any("obelisk" in ps for ps in cov["demoproj"]) # second lane + + +def test_memory_hooks_maps_every_index_pointer_to_its_line(tmp_path): + """`_memory_hooks` keys off the pointer TARGET across every MEMORY*.md, + via the shared `memory_reindex.parse_pointers` (#54) rather than a second + local regex — so both syntaxes resolve and the hook is the whole line.""" + _store, pdir = _lane_store(tmp_path) + hooks = md._memory_hooks(pdir) + assert "quaternion" in hooks["rich-page.md"] + assert "kaleidoscope" in hooks["bare-page.md"] + assert "obelisk" in hooks["third-page.md"] + # A routing row is a pointer too, so the lane indexes are keyed; they are + # simply never looked up, because page_files() never yields them. + assert "covers the parser lane" in hooks["MEMORY-parser.md"] + + +def test_memory_hooks_empty_when_no_index(tmp_path): + pdir = os.path.join(str(tmp_path), "projects", "bare") + _write(os.path.join(pdir, "page.md"), "# page\n") + assert md._memory_hooks(pdir) == {} + + # --------------------------------------------------------------------------- # THE gate invariant: distill never writes to the store # ---------------------------------------------------------------------------