fix(distill/skills): enumerate every MEMORY*.md; resolve memory dir via the engine probe - #59
Merged
Merged
Conversation
…ia the engine probe Two modules were still deriving, by hand, facts the engine already owns — and lane routing (#53) turned both into silent, unsafe-direction bugs. `plugins/memory_distill.py` read only the root `MEMORY.md`. A lane index is a dense bag of every hook in that lane, so feeding it to `_page_token_set` made `load_coverage` report almost any topic as already-covered and distill suppressed genuine suggestions; separately, any page whose pointer moved to a lane index got an empty hook string and a systematically thinner token set. Both predicates now delegate to `memory_reindex.page_files()` / `index_files()`, and the local rich-link-only regex is replaced by `read_lines()` + `parse_pointers()`, which also resolves bare `- slug.md — hook` pointers. `skills/okfmem-curate/SKILL.md` derived its memory dir with `SLUG=$(pwd -P | sed 's|/|-|g')`. That replaces only the separator, but `encode_root` also encodes the Windows drive colon, so curate could not run at all on Windows without an explicit path argument — and it blamed a missing directory rather than its own slug. It now asks `okfmem init --project-link-state`, which resolves the name through the registry and so handles renames on POSIX too. Pre-PR adversarial review caught two further defects, both fixed here: the probe block collapsed the memory dir to the store's projects root whenever no name was printed (including when `okfmem` is off `PATH`, a supported install), in curate and in the okfmem-save block it was ported from — where the same collapse would have written `STATE.md` and pages into the projects root; and the follow-on fix left `readlink` testing a symlink that is no longer the resolved path, which reported "not recoverable via git" on every correctly linked store. - plugins/memory_distill.py: page/index enumeration + hooks via memory_reindex; _SKIP_PAGE_NAMES deleted; guarded engine import - tests/test_distill.py: lane-routed store fixture + 4 regression tests - skills/okfmem-curate/SKILL.md: engine probe, per-state branching, git-backed check via rev-parse, ${VAR:?} guards at every call site - skills/okfmem-save/SKILL.md: same probe hardening (write path) Closes #57 Closes #58
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two modules were still deriving by hand facts the engine already owns, and lane routing (#53) turned both into silent, unsafe-direction bugs. Both now ask the engine.
Closes #57
Closes #58
#57 — distill reads every
MEMORY*.md, not just the rootplugins/memory_distill.pyread only the rootMEMORY.mdand treated every otherMEMORY*.mdas a durable page. Both assumptions were correct-by-accident before #53:_page_token_setmadeload_coveragereport almost any topic in that lane as already-covered — and distill suppressed genuine suggestions. Silent, no error, invisible in the output.MEMORY-<lane>.mdgot""as its hook, so it carried a systematically thinner token set than root-pointed ones. Under Two-tier index: route pointers to lane indexes, make MEMORY.md a map of content #53's default routing that is most pages, so it was the common case rather than the edge case.Both predicates now delegate to
memory_reindex.page_files()/index_files()— the single home for the page-vs-index rule as of #56 — and the local rich-link-only regex is replaced byread_lines()+parse_pointers(), which also resolves bare- slug.md — hookpointers the old regex missed._SKIP_PAGE_NAMESwas deleted rather than extended, so the local rule cannot drift back.Four regression tests, verified failing against
git show HEAD:plugins/memory_distill.pyin a scratch tree — not by reasoning. The headline one fails exactly as predicted:assert 5 == 3, with the two extra sets being the lane indexes.#58 — curate resolves its memory dir via the engine probe
skills/okfmem-curate/SKILL.mdPhase 1 derived the dir withSLUG=$(pwd -P | sed 's|/|-|g'). That replaces only the separator, butencode_rootalso encodes the Windows drive colon (C:\Users\you\okfmem→C--Users-you-okfmem), so on Windows the slug matched nothing, Phase 1's own guard fired, and/okfmem-curatecould not run at all without an explicit path argument — while reporting a missing directory when the directory existed and the slug was wrong. A raw cwd-to-slug transform also can't see the registry, so renames broke on POSIX too.It now calls
okfmem init --project-link-state(shipped #48), which resolves the name through the registry. Each probe state gets its own message;unlinkednamesokfmem initas the remedy. The explicit-path escape hatch is unchanged.Caught by pre-PR adversarial review, fixed here
awk '{print $2}'yields empty fornot-a-repo,no-claude, and forokfmemoffPATH— a configurationinstall.shexplicitly supports.MEM_DIRbecame$STORE/projects/, a real directory that passes any-dguard, so Phases 2–3 reported on the wrong tree and Phase 4 asked the user to approve deletions derived from it. The same block inskills/okfmem-save/SKILL.mdhad the identical defect with a larger blast radius, because okfmem-save writes —STATE.mdand memory pages would have landed in the projects root, where nothing auto-loads them. Fixed in both:read -r STATE NAME(so a bare state word can never become a project name, which${LINK_STATE#* }would have allowed), two separate probe assignments, the dir assigned only on thelinkedbranch, and a fifth "engine unreachable" table row. Every downstream call site carries a${VAR:?...}guard so an unresolved dir refuses instead of failing open —reindex --report ""otherwise auto-resolves to the cwd project and prints a plausible report.readlink "$MEM_DIR"git-recoverability probe printed nothing and exited 1 on every correctly linked store. Curate would have told the user their deletions were unrecoverable 100% of the time on the happy path, including in Phase 4's hard approval gate — the same shape as Save-state badge stuck 'unsaved': SAVE_BASH_RE misses path- and interpreter-prefixedokfmem sync#46/fix(badge): recognize path- and interpreter-prefixedokfmem syncas a save #47, where a benign-direction misfire firing always killed the signal. Replaced withgit -C "$MEM_DIR" rev-parse --show-toplevel, which tests the property instead of a proxy and also covers the explicit-path escape hatch thatreadlinknever handled.Test plan
python3 scripts/check-leaks.pyexits 0 — 69 tracked files, no leaks; added prose eyeballed for private strings the gate can't judge (only~/okfmem,~/okfmem-store,$OKFMEM_STOREplaceholders)python3 -m pytest tests/— 318 passed / 1 skipped, unchanged across every review roundruff checkclean on both changed Python files (ruff format --checkflags both; confirmed pre-existing onHEAD, advisory in CI)linked/unlinked/not-a-repo/no-claude/ off-PATH/ engine-unreachable / barelinkedwith no name / a project name containing a spaceplugins/copied outside the repo with no engine anywhere — one stderr line, exit 2, no tracebackverify— the CI gateKnown, deliberate
memory_reindex.NON_PAGE_NAMESis{STATE.md, CONTEXT.md}, while the deleted_SKIP_PAGE_NAMESalso listedSESSIONS.mdandREADME.md. Neither is ever seeded into aprojects/<name>/dir (_seed_store_projectwrites onlyMEMORY.md+STATE.md; the store'sREADME.mdsits at the projects root, which is not scanned), and an old-vs-new enumeration diff across all 14 projects of a real store showed 12 files newly excluded — all genuine lane indexes — and 0 newly included. Re-adding a local filter would reinstate the predicate this PR exists to delete; wideningNON_PAGE_NAMESwould changereindex --verifyorphan semantics andstatusinventory counts for every other caller. Flagged rather than silently decided.Provenance
verifyAdversarial review
Two bounded rounds, reviewer independent of the implementers and prompted to break the change rather than bless it. Both rounds returned
clean: false.Round 1 — 1 blocking
skills/okfmem-curate/SKILL.md— the probe collapsedMEM_DIRto the store's projects root.awk '{print $2}'yields the empty string fornot-a-repo,no-claude, and forokfmemoffPATH. Reproduced end-to-end:env PATH=/usr/bin:/bin→LINK_STATE=[]→MEM_DIR=[$STORE/projects/]→ directory exists, guard passes, curate proceeds against the whole store. A regression — pre-change thesedresolved correctly on POSIX in that case. The reviewer also found the identical block inskills/okfmem-save/SKILL.md, where the blast radius is larger because that skill writesSTATE.mdand pages.Fixed in both files. The fix subagent tested and then rejected all three parts of the reviewer's suggested snippet:
${LINK_STATE#* }returns a single-word value unchanged, sonot-a-repowould have become the project name — replaced withread -r STATE NAME, which leavesNAMEempty instead.$(a || b)became two separate assignments so an error string cannot concatenate onto a valid state. The dir is initialised empty and assigned only on thelinkedbranch. Verified by extracting the fenced blocks from the shipped markdown and executing them against fake engines across 8 conditions.Round 2 — 1 blocking (collateral from the round-1 fix)
skills/okfmem-curate/SKILL.md— the git-recoverability probe went dead and lied at the deletion gate. The fix changedMEM_DIRfrom the harness symlink to the symlink's target, but the following line was stillreadlink "$MEM_DIR", which prints nothing and exits 1 on a real directory. Reproduced: new path →readlink exit=1(skill concludes "not git-backed"), old path → prints the target, ground truth → the dir is inside~/okfmem-store. Per the skill's own prose that means every curate run on a correctly linked store told the user recovery was harder, and Phase 4's hard approval gate carried "this is not recoverable via git" — a benign-direction misfire firing 100% of the time, the same shape as #46/#47.Fixed with
git -C "$MEM_DIR" rev-parse --show-toplevel— the property, not a proxy — which also covers the explicit-path escape hatchreadlinknever handled. Verifying that fix surfaced a second fail-open:git -C "" rev-parsereturns the cwd repo, so an unresolved dir would have reported the wrong repository as the recovery mechanism. Hence the${VAR:?...}guard at all 8 downstream call sites (6 curate, 2 okfmem-save), which also closes the reviewer's related finding thatreindex --report ""auto-resolves to the cwd project and prints a plausible report.Verified rather than trusted
Round 2 independently re-checked round 1's clears rather than inheriting them:
lines[p.line - 1]is aligned by construction (read_linessplits onb"\n",parse_pointerssetslineno = i + 1over that same list,rstrip("\r")preserves length);index_files()returns root-first by construction, sosetdefault's root-wins precedence is a guarantee rather than filesystem luck, and it is an improvement over the old last-wins;syntax == "external"can never match a basename, so the skip cannot drop a real hook;_find_engine_root()probes the local root first, so$OKFMEM_ENGINE/~/okfmemcannot silently supply a different engine than the dispatcher intends.Both rounds checked the Windows/platform-literal class individually per assertion — the class that bit this repo on #14/#16/#17, #41, #42, and three times on #56. Clean:
_writepinsnewline="\n", every fixture path usesos.path.join, expected counts derive frompage_files()rather than literals, and there is no byte-count assertion in the added code.Surviving nits — for the human reviewer
skills/okfmem-curate/SKILL.md:25still describes the target as~/.claude/projects/<project-slug>/memory/, the pre-change path shape. Same directory via the symlink, so not wrong, but stale next to the new Phase 1.plugins/memory_distill.py:78-82— the guarded-import message namesmemory_reindex.pywhileexcept ImportErrorcatches any transitive failure, so the sentence can pair two different module names. The real exception text is interpolated, so it stays diagnosable.plugins/memory_distill.py— the engine root is now inserted only when absent fromsys.path, which leavesplugins/ahead of it rather than behind. No colliding module name exists today.plugins/memory_distill.py:238— a bare pointer whose hook contains an inline link keys both targets to the same line, so the linked page inherits a hook that is not its own. Heuristic-only, and the regex it replaced had the same class with last-wins instead of first-wins.skills/okfmem-curate/SKILL.md:60-63claimed the engine-path hedge matched Phase 2's; Phase 2 in fact uses only thepython3 ~/okfmem/okfmemform. Comment corrected in this PR.Follow-up, not in this PR
The reviewer found the same restated predicate #57 exists to delete in two engine modules the issues did not name:
memory_backfill.py:36andmemory_consolidate.py:49both carry a localSKIP_NAMES = {"MEMORY.md","STATE.md","CONTEXT.md"}. Reproduced: 11 real lane indexes on a live machine pass that filter and reachprocess_file, which would stamp decay frontmatter into a lane index; consolidate would then become eligible to decay/archive it. Latent today only because both bail on files with no frontmatter. Same one-line fix (page_files(d)), but engine code with different blast radius — filed separately rather than expanded into this PR.