Skip to content

docs: state-sync allowlist design (#872, design phase only) - #879

Merged
qingyun-wu merged 1 commit into
mainfrom
feat/state-sync-allowlist-design
May 19, 2026
Merged

qingyun-wu merged 1 commit into
mainfrom
feat/state-sync-allowlist-design

Conversation

@qingyun-wu

Copy link
Copy Markdown
Collaborator

Summary

Covers

  • Allowlist file format: state/.sync-allowlist, newline-delimited, #-comments, no globs in v1.
  • Coordination protocol: claim-via-atomic-rename (POSIX single-FS atomic) + per-host done-flag for side-effect idempotency.
  • Cross-machine race handling: last-rename-wins for the sync gap; acceptable v1 trade as long as side-effect processors gate on the done-flag.
  • State transitions: queue → claimed → done-flag → result → archive.
  • Lifecycle invariants: single-Mac wipe is recoverable via next sync; all-Macs-simultaneous wipe is user-action consequence.
  • Implementation sketch: scripts/sync-fleet-state.sh sibling of scripts/sync-memory.sh; task-router separates fleet-eligible from single-machine flows.
  • Failure-mode table with mitigations.
  • Open questions (transport choice, granularity, TTL on claims).
  • Out-of-scope list (CRDTs, external coordinator, team-shared fleet, retro-fitting tasks/ itself).

Acceptance for the eventual implementation PR

  • Opt-in only; default behavior unchanged.
  • Cross-Mac queue works with zero lost-task incidents.
  • Duplicate-process incidents bounded to at most one duplicate per task in a sync-gap window.
  • Tests cover empty allowlist, missing allowlist, path-escape attempt, two-Mac race simulation.

Test plan

Tracks #872. Closes when the implementation PR lands; this PR alone is the design contract.

🤖 Generated with Claude Code

Per the issue's "design phase first, code later" scope. RFC #858 Decision 4.

Adds docs/state-sync-allowlist.md covering:
- Allowlist file format (newline-delimited, #-comments, no globs in v1)
- Coordination protocol: claim-via-atomic-rename + per-host done-flag for
  side-effect idempotency
- Race handling: last-rename-wins for cross-machine claim collisions in
  the sync gap; acceptable v1 trade with idempotent side-effect helpers
- State transitions (queue → claimed → done-flag → result → archive)
- Lifecycle invariants — rm -rf workspace on one Mac is survivable, on
  all Macs simultaneously is user-action consequence (documented, not
  engineered around)
- Implementation sketch (sync-fleet-state.sh sibling of sync-memory.sh,
  task-router separates fleet-eligible from single-machine flows)
- Failure-mode table + mitigations
- Open questions (transport choice, granularity, TTL on claims)
- Out-of-scope list (CRDTs, external coordinator, team-shared fleet)

No code change. Implementation tracked under #872 as a follow-up; this
doc is the contract the implementation has to land against.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

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

Reviewed feat/state-sync-allowlist-design HEAD — 171-line design doc, design-phase-only (no code). Implementation tracked under #872; this is the contract.

Like:

  • Preserves the 3-space invariant — opt-in allowlist file, default-off (file missing = no sync), per-machine wipe survivable. The "rm -rf workspace on ONE Mac is recoverable, rm -rf on ALL is user-action consequence" framing draws the line in the right place.
  • Honest about race semantics. Claim-via-atomic-rename is POSIX-atomic on a single FS; cross-machine races are eventual-consistency races, not POSIX races. Doc names this distinction explicitly rather than papering over it.
  • Failure-mode table is concrete. Especially the email-dedup-via-Message-Id row — that's the right shape of mitigation for the "duplicate side effect" failure mode (transport-level dedup, not protocol-level locking).
  • Out-of-scope list is principled. CRDTs, external coordinator, multi-user fleet — all the over-engineering temptations called out by name and deferred.
  • Open questions section defers TTL/lease, granularity, transport choice with the rationale for deferral. That's the right move in a design doc — don't pre-litigate questions that operational data will answer cheaply.

Asks (substantive, since this is the contract code will be held to):

  1. The claim-race semantics are transport-dependent, but the doc waves at "eventual consistency" without naming the transport. With rsync --update (mtime-wins), if Mac A renames at t=10 and Mac B renames at t=11, ONE file survives post-sync (not two) — last-rename-wins via OVERWRITE, not via dual-existence. With git, both claimed-A.txt and claimed-B.txt files exist post-merge (different paths). The doc says "we'll have two claimed-by-different-hosts files post-merge" which is the git semantic; the inline text + the implementation sketch say "rsync mtime-wins" which is the opposite. Pick one transport explicitly for v1 and walk the race table against THAT transport's actual merge semantic. Today this contradiction is the single thing most likely to surprise the eventual implementer.

  2. The duplicate-vs-loss trade is asserted both ways. Acceptance says "zero lost-task incidents, duplicate-bounded to 1 per sync gap." But the done-flag-before-side-effect convention gives the OPPOSITE trade — if a processor writes the flag, then crashes before the side effect, post-merge both Macs see the flag and skip the side effect. That's a lost task, not a duplicate. Either: (a) flip the convention to side-effect-then-flag, accept the bounded-duplicate window, and lean on dedup helpers (Message-Id, idempotency keys); or (b) add a retry-on-orphan-flag mechanism that detects flag-without-result and reissues. Today's doc text supports both interpretations.

  3. route: fleet body-field needs a written-by-whom answer. "Owner marks fleet-eligible" — by what UI? Voice tool? Bridge field? Auto-classifier on task content? Without this nailed down, the implementation lands and zero tasks are fleet-eligible because nobody knows how to flag them. Worth at least: "v1 = manual route: fleet line in the task file body; future = voice/Telegram tool surface."

  4. fleet-router as "new process OR proactive-loop step" is two architectures. Latency profile is very different — new process (event-driven, sub-second) vs proactive-loop step (5-minute cadence, batch). Pick one. Lean toward proactive-loop step for v1: matches the existing memory-sync cadence, no new process to supervise.

  5. No mention of data/conversation.sqlite (or any sqlite file). It's the obvious foot-gun for fleet sync — single-machine WAL files don't merge through rsync OR git. Worth one explicit line: "sqlite files are not v1 allowlist candidates; cross-machine session history is a separate problem out of scope." Otherwise an enthusiastic operator adds data/ to .sync-allowlist and corrupts the conversation log on first sync.

  6. No documented manual recovery for stranded claims. TTL/lease is deferred (right call), but the failure mode that motivates a lease ("Mac claims, crashes, never re-syncs → task lost") still exists in v1 with no operator escape hatch. Worth a one-liner under "Failure modes": find state/fleet/queue -name '*.claimed-*' -mtime +1 -delete (or equivalent) as the manual recovery procedure for stranded claims. Document the diagnostic; don't engineer a lease yet.

  7. Allowlist path-escape validation — table says "entries that resolve outside state/ are skipped with a warning." Good defense, but the doc doesn't name the validation pattern. The CodeQL-recognized sanitizer is os.path.realpath(p) + .startswith(state_root) (same shape as discord-bridge.py:_is_path_sendable). Worth naming explicitly so the eventual implementation can be grep'd for that pattern.

Nits:

  • "v1" is used liberally for "the first implementation"; nothing wrong with that, but consider naming the implementation milestone explicitly (the design doc is technically v1 of the spec, not v1 of the code) to avoid ambiguity.
  • "Acceptance for the eventual implementation PR" + "Acceptance for the implementation PR (when it happens)" are two near-duplicate sections at top + bottom. Merge.
  • The Relationship-to-other-docs section at the end should probably also link forward to where #872 (the implementation issue) lives.

Solid design contract overall. The transport-vs-merge contradiction (ask #1) and the duplicate-vs-loss reconciliation (ask #2) are the two pieces I'd want resolved before the implementation PR starts; everything else is good-to-have.

@vidhuUC vidhuUC left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

APPROVE. Design-only doc PR for state-sync allowlist (#872). The 3-space survivability invariant is preserved (workspace rm is safe). Claim-via-atomic-rename is correct for POSIX single-FS. Last-rename-wins for the sync gap is acceptable for v1 given side-effect gating on done-flag. No code means no blast radius here. Good to have this as the reference contract before implementation.

@qingyun-wu
qingyun-wu merged commit 22eb6e3 into main May 19, 2026
1 check passed
@qingyun-wu
qingyun-wu deleted the feat/state-sync-allowlist-design branch May 19, 2026 17:34
liususan091219 pushed a commit to liususan091219/sutando-fork that referenced this pull request May 28, 2026
…onichi#879)

Per the issue's "design phase first, code later" scope. RFC sonichi#858 Decision 4.

Adds docs/state-sync-allowlist.md covering:
- Allowlist file format (newline-delimited, #-comments, no globs in v1)
- Coordination protocol: claim-via-atomic-rename + per-host done-flag for
  side-effect idempotency
- Race handling: last-rename-wins for cross-machine claim collisions in
  the sync gap; acceptable v1 trade with idempotent side-effect helpers
- State transitions (queue → claimed → done-flag → result → archive)
- Lifecycle invariants — rm -rf workspace on one Mac is survivable, on
  all Macs simultaneously is user-action consequence (documented, not
  engineered around)
- Implementation sketch (sync-fleet-state.sh sibling of sync-memory.sh,
  task-router separates fleet-eligible from single-machine flows)
- Failure-mode table + mitigations
- Open questions (transport choice, granularity, TTL on claims)
- Out-of-scope list (CRDTs, external coordinator, team-shared fleet)

No code change. Implementation tracked under sonichi#872 as a follow-up; this
doc is the contract the implementation has to land against.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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