Skip to content

fix(schedule-crons): remove cross-node-sync from default cron template - #936

Merged
sonichi merged 2 commits into
mainfrom
fix/758-remove-cross-node-sync-from-default-crons
May 20, 2026
Merged

sonichi merged 2 commits into
mainfrom
fix/758-remove-cross-node-sync-from-default-crons

Conversation

@sonichi

@sonichi sonichi commented May 20, 2026

Copy link
Copy Markdown
Owner

Summary

  • Remove cross-node-sync from skills/schedule-crons/crons.example.json so new users don't auto-wire it
  • Keeps the skill itself intact (skills/cross-node-sync/) — still available as explicit opt-in via its SKILL.md

Why

cross-node-sync requires:

  • SUTANDO_SYNC_PEER env var + SSH key pair between two physical nodes
  • Per-file-frontmatter memory model as source of truth

Default new-user installs:

  • have no peer (single-host) → rsync skips, but the chained regenerate-memory-index.py still fires
  • start with inline entries in MEMORY.md (# userEmail, # currentDate from auto-memory init)

#806 just landed a defensive guard so regen refuses to clobber when 0 frontmatter entries + non-empty target. But the guard only fires at the zero-entries boundary. Once a user creates one frontmatter file, regen silently drops their remaining inline entries.

Per #758's PR description, the dir-exists-but-zero-frontmatter-siblings case was never validated end-to-end (the unchecked @chetanunadkat to verify after his next sync item).

Net: make the cron explicit opt-in. Single-host users don't need rsync-over-ssh; multi-host users add it deliberately when they set up peer auth.

Test plan

  • crons.example.json parses as valid JSON after edit
  • cross-node-sync entry removed; other 5 default crons preserved
  • Existing users with already-copied crons.json are unaffected (this file is .example template only)

Refs #787 (the clobber bug), #806 (defensive guard, just merged), #758 (the auto-call wiring that introduced this).

🤖 Generated with Claude Code

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).

@bassilkhilo-ag2 bassilkhilo-ag2 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cold-review from Bassil's Sutando (Mac Mini, single-host).

LGTM. This matches our exact situation — the cross-node-sync cron fires every 7 min but always skips because SUTANDO_SYNC_PEER isn't set, and the chained regenerate-memory-index.py still runs. The defensive guard from #806 helps at the zero-entries boundary, but the partial-clobber risk Chi describes (one frontmatter file created → remaining inline entries dropped silently) is real and we've seen the MEMORY.md get clobbered in exactly that pattern.

Making it explicit opt-in is the right call. Single-host users get no benefit from an every-7-min rsync-over-ssh they never configured; multi-host users add it deliberately.

One note: existing users who already copied crons.json from the template (before this fix) will still have the entry active — worth a one-liner in the PR description or SKILL.md under 'Upgrading' telling them to manually remove the cross-node-sync block if they're single-host. Not blocking — just a DX thing.

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)
@bassilkhilo-ag2

Copy link
Copy Markdown
Collaborator

schedule-crons fix — reviewed, straightforward

Correct call. cross-node-sync requires SUTANDO_SYNC_PEER + SSH peer auth + per-file-frontmatter memory model — none of which apply to a fresh single-host install. Opt-in is the right default.

Two small things to confirm:

  1. jq selector fix is correct: changing .jobs[].name → .[].name in the verification snippet matches the actual crons.json format (top-level array, not {"jobs":[]}). Good catch.

  2. In-flight sessions: anyone already running with cross-node-sync in their live crons.json is unaffected by this PR — it only changes crons.example.json. The SKILL.md now documents the opt-in block explicitly, so upgraders can self-serve.

LGTM.

@liususan091219 liususan091219 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cold-review follow-up — the SKILL.md doc/code fix (commit d278e58) fully addresses the one issue I raised: cross-node-sync/SKILL.md's cron-wiring section now correctly documents the opt-in reality + gives the exact JSON block, and the stale jq '.jobs[].name' path is corrected to .[].name. Diff is minimal and correct — drops the cross-node-sync entry from the default crons.example.json template, JSON stays valid, skill itself retained as explicit opt-in. CI green.

— Lucy (Mac Studio bot)

@sonichi
sonichi merged commit 06c640a into main May 20, 2026
1 check passed
@sonichi
sonichi deleted the fix/758-remove-cross-node-sync-from-default-crons branch May 20, 2026 13:49
liususan091219 pushed a commit to liususan091219/sutando-fork that referenced this pull request May 28, 2026
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants