Repository navigation
fix(regenerate-memory-index): refuse to clobber MEMORY.md when 0 frontmatter entries found - #806
Conversation
… when frontmatter scan finds 0 entries (closes sonichi#787) The cron cross-node-sync fires every 7 min. When SUTANDO_SYNC_PEER is unset, rsync skips cleanly but regenerate-memory-index.py still runs. If no per-file frontmatter .md files exist (e.g. the user has only inline entries in MEMORY.md), the script overwrites MEMORY.md with an empty body — silently destroying any accumulated inline entries. The data loss is irrecoverable on machines where the inline entries are non-trivial and not visible in the loop's system reminder. Fix: guard the write. When the frontmatter scan produces 0 entries AND the existing MEMORY.md is non-trivial (has content lines beyond just headings/whitespace), refuse to write and print a warning instead. Exits 1 so the cron surface can detect the failure if desired. Still allows: - Writing when 0 entries and MEMORY.md is trivial (just a heading) - Writing when 0 entries and MEMORY.md doesn't exist (fresh install) - Writing when entries > 0 (normal operation) Issue also suggests long-term fix: migrate inline entries to per-file frontmatter automatically. This PR implements the stop-the-bleeding guard (option 1 from the issue).
qingyun-wu
left a comment
There was a problem hiding this comment.
Cold review — LGTM. Defensive guard for a real failure mode.
Verified
- Guards against the cron silently overwriting MEMORY.md with a bare header when the frontmatter scan returns 0 entries
- Triggers only when (a) 0 frontmatter entries AND (b) target file already exists AND (c) target has non-trivial content (not just headers/whitespace)
- Returns exit 1 — script fails loudly instead of silently destroying data
- Closes #787
Non-blocking observations
-
Content-detection heuristic —
content_lines = [l for l in existing.splitlines() if l.strip() and not l.strip().startswith("#")]skips both empty lines and header lines. Good. Worth a comment that "list items, paragraphs, fenced code, etc. all count as non-trivial content" — anyone reading later might wonder why this specific filter. -
Stderr-only warning — script fails with code 1 but doesn't write the warning to stdout or a structured log. Cron consumers checking stdout won't see anything (good — no noise on the happy path); ones checking stderr will see the error (which is what they want). Right separation.
-
--dry-runescape hatch mentioned in the warning is nice — gives the operator a path forward without immediately editing the script. -
Failure cascades: if this guard fires, the cron will retry every 7 min and stderr will accumulate. That's by design (visible failure) but worth noting for monitoring — if the warning rate spikes, it indicates a misconfiguration that needs human attention.
Ship it.
liususan091219
left a comment
There was a problem hiding this comment.
Clean defensive fix — the guard is well-scoped: only triggers when (entries=0 AND target exists AND target has content lines that aren't headings). Exit-1 is correct (cron should NOT silently overwrite the inline index). Closes #787 properly.
Verified the guard logic at line 110: content_lines = [l for l in existing.splitlines() if l.strip() and not l.strip().startswith("#")] correctly excludes headings and whitespace. So a fresh-install MEMORY.md with just # Memory Index\n won't trip the guard — only a populated inline index will.
Non-blocking observation: the error message tells the user to either --dry-run or "add per-file frontmatter to memory entries" — clear actionable. Could also suggest "or set SUTANDO_SYNC_PEER to fetch the synced memory dir" for the cron-without-peer case the body describes.
LGTM as-is.
— Lucy (Mac Studio bot)
qingyun-wu
left a comment
There was a problem hiding this comment.
Cold review — fix looks correct. The guard catches the real-world failure mode (cron silently clobbers inline-only MEMORY.md when SUTANDO_SYNC_PEER is unset) with a minimal patch shape.
One nuance — the guard runs unconditionally before the args.dry_run check, so --dry-run exits 1 with just the warning, never showing the would write 0 entries preview the existing dry-run path emits. Probably fine — a dry-run user who hits the guard learns the right thing (refusing to clobber) — but if you want dry-run to keep its read-only "show me what would happen" semantics, push the guard into an if not args.dry_run: branch and let dry-run print both the warning + the body preview side-by-side. Minor; not a merge blocker.
Heading detection (not l.strip().startswith("#")) — clean and matches the docstring. Works correctly for standard markdown.
Confirmed via git show main:skills/cross-node-sync/scripts/regenerate-memory-index.py that the fix has NOT landed on main yet, so this PR is still needed (the script there still writes unconditionally at line ~108).
Posting cold-review on behalf of @qingyunwu while the supersession sweep is fresh — owner has the merge call.
… guard fires Per qingyun-wu's review on sonichi#806: the guard was running before the --dry-run check, so dry-run would exit 1 without showing the 'would write N entries' preview. Now: - Guard sets a flag instead of returning immediately - dry-run always shows the preview, then exits 1 if guard fired - Normal run exits 1 if guard fired (same as before) - Also improved error message to suggest SUTANDO_SYNC_PEER - Added comment clarifying content_lines heuristic
jsun-m
left a comment
There was a problem hiding this comment.
Cold review — and a freshly-reproduced data-loss confirmation, since I just hit this exact failure mode in production a few minutes ago.
Live repro (this session, 2026-05-19):
- Cross-node-sync cron fired (
SUTANDO_SYNC_PEERunset → rsync exits early, regen runs anyway) python3 skills/cross-node-sync/scripts/regenerate-memory-index.py→wrote 0 entries to .../MEMORY.mdMEMORY.mdclobbered: pre-run had# userEmail+# currentDateinline entries + the index header; post-run had 2 blank lines.- Restored by hand from session-start cached contents. On a node where those inline entries hadn't been seen this session, the data would be unrecoverable.
The fix on this PR would have prevented all of it:
entries == 0(no per-file frontmatter exists on this node)target.exists()(MEMORY.md was already there)content_lines = ['The user's email address is...', 'Today's date is 2026-05-19.']→ guard fires → exit 1 → file preserved.
On the diff itself: the dry-run handling (return 1 if clobber_guard else 0) cleanly addresses qingyun-wu's second-review nuance — dry-run still emits the preview AND the guard warning side-by-side, then returns non-zero to signal "this would-have-clobbered." Good shape.
Mitigation this session: deleted the cross-node-sync cron job (id 59398688) from the active scheduler so it can't refire and clobber again before merge. The cron is currently a no-op anyway (peer unset, regen destructive) — losing it for one session has no downside.
Three LGTMs already on this PR; adding a fourth + a live-victim data point. Ship it.
#936) * fix(schedule-crons): remove cross-node-sync from default cron template Cross-node-sync requires `SUTANDO_SYNC_PEER` + SSH setup between two physical nodes, plus assumes per-file-frontmatter is the source of truth for memory entries. Default new-user installs: - have no peer (single-host) - start with inline entries in MEMORY.md (auto-memory init: `# userEmail`, `# currentDate`) #806 added a defensive guard so regen refuses to clobber when 0 frontmatter entries + non-empty target — but the guard fires only on the *zero-entries* boundary. Once a user creates one frontmatter file, regen silently drops remaining inline entries from MEMORY.md. Per #758 PR test plan, the `dir-exists-but-zero-frontmatter-siblings` case was never validated end-to-end (the unchecked @chetanunadkat verify item). Net: make the cron explicit opt-in via the SKILL.md. Single-host users don't need rsync-over-ssh; multi-host users add it deliberately when they set up peer auth. Refs #787 (the clobber bug), #806 (defensive guard, already merged), #758 (the auto-call wiring that introduced this for new users). * docs(cross-node-sync): rewrite cron-wiring section for opt-in reality Per Lucy's review on #936: SKILL.md previously said the cron-example wires cross-node-sync automatically via 'cp crons.example.json crons.json'. After the #936 removal that's false, so the doc actively misled. Rewrite the 'Cron wiring' section as a single 'explicit opt-in' block that: - Explains why it's opt-in (SUTANDO_SYNC_PEER + SSH + per-file-frontmatter requirements don't apply to single-host) - Includes the exact JSON entry operators copy into their crons.json - Updates the jq verify command (.[] not .jobs[] — crons.json is a top-level array, not an object)
…tmatter entries found (sonichi#806) * fix(regenerate-memory-index): refuse to clobber non-trivial MEMORY.md when frontmatter scan finds 0 entries (closes sonichi#787) The cron cross-node-sync fires every 7 min. When SUTANDO_SYNC_PEER is unset, rsync skips cleanly but regenerate-memory-index.py still runs. If no per-file frontmatter .md files exist (e.g. the user has only inline entries in MEMORY.md), the script overwrites MEMORY.md with an empty body — silently destroying any accumulated inline entries. The data loss is irrecoverable on machines where the inline entries are non-trivial and not visible in the loop's system reminder. Fix: guard the write. When the frontmatter scan produces 0 entries AND the existing MEMORY.md is non-trivial (has content lines beyond just headings/whitespace), refuse to write and print a warning instead. Exits 1 so the cron surface can detect the failure if desired. Still allows: - Writing when 0 entries and MEMORY.md is trivial (just a heading) - Writing when 0 entries and MEMORY.md doesn't exist (fresh install) - Writing when entries > 0 (normal operation) Issue also suggests long-term fix: migrate inline entries to per-file frontmatter automatically. This PR implements the stop-the-bleeding guard (option 1 from the issue). * fix(regenerate-memory-index): dry-run shows preview even when clobber guard fires Per qingyun-wu's review on sonichi#806: the guard was running before the --dry-run check, so dry-run would exit 1 without showing the 'would write N entries' preview. Now: - Guard sets a flag instead of returning immediately - dry-run always shows the preview, then exits 1 if guard fired - Normal run exits 1 if guard fired (same as before) - Also improved error message to suggest SUTANDO_SYNC_PEER - Added comment clarifying content_lines heuristic --------- Co-authored-by: Susan Xueqing Liu <xliu127@stevens.edu> Co-authored-by: Chi Wang <4250911+sonichi@users.noreply.github.com>
sonichi#936) * fix(schedule-crons): remove cross-node-sync from default cron template Cross-node-sync requires `SUTANDO_SYNC_PEER` + SSH setup between two physical nodes, plus assumes per-file-frontmatter is the source of truth for memory entries. Default new-user installs: - have no peer (single-host) - start with inline entries in MEMORY.md (auto-memory init: `# userEmail`, `# currentDate`) sonichi#806 added a defensive guard so regen refuses to clobber when 0 frontmatter entries + non-empty target — but the guard fires only on the *zero-entries* boundary. Once a user creates one frontmatter file, regen silently drops remaining inline entries from MEMORY.md. Per sonichi#758 PR test plan, the `dir-exists-but-zero-frontmatter-siblings` case was never validated end-to-end (the unchecked @chetanunadkat verify item). Net: make the cron explicit opt-in via the SKILL.md. Single-host users don't need rsync-over-ssh; multi-host users add it deliberately when they set up peer auth. Refs sonichi#787 (the clobber bug), sonichi#806 (defensive guard, already merged), sonichi#758 (the auto-call wiring that introduced this for new users). * docs(cross-node-sync): rewrite cron-wiring section for opt-in reality Per Lucy's review on sonichi#936: SKILL.md previously said the cron-example wires cross-node-sync automatically via 'cp crons.example.json crons.json'. After the sonichi#936 removal that's false, so the doc actively misled. Rewrite the 'Cron wiring' section as a single 'explicit opt-in' block that: - Explains why it's opt-in (SUTANDO_SYNC_PEER + SSH + per-file-frontmatter requirements don't apply to single-host) - Includes the exact JSON entry operators copy into their crons.json - Updates the jq verify command (.[] not .jobs[] — crons.json is a top-level array, not an object)
Closes #787.
Problem
regenerate-memory-index.pyunconditionally writesMEMORY.mdfrom the frontmatter scan, regardless of whether a sync actually ran or whether the existing index has content the scan didn't produce. WhenSUTANDO_SYNC_PEERis unset (no rsync peer), rsync skips cleanly but the regen step still runs. If no per-file frontmatter.mdfiles exist, the script overwritesMEMORY.mdwith an empty body — silently destroying any accumulated inline entries.The cron fires every 7 min, so a single boot of the proactive loop is enough to destroy inline memory. The data loss is silent and irrecoverable on machines where the inline entries are non-trivial.
Fix
Guard the write: when the frontmatter scan produces 0 entries AND the existing
MEMORY.mdis non-trivial (has content lines beyond just headings/whitespace), refuse to write and print a warning to stderr. Exits 1 so the cron surface can detect the failure.Still allows:
MEMORY.mdis trivial (just a heading) → ok to overwriteMEMORY.mddoesn't exist (fresh install) → creates empty indexThis is the cheapest stop-the-bleeding fix (option 1 from the issue). Long-term, inline entries should be migrated to per-file frontmatter automatically (option 3).
Testing