feat(datastore): scoped sync and capability-gated concurrency (swamp-club#350)

[stack72]

Summary

• Add opt-in DatastoreCapabilities interface with scopedSync flag and optional capabilities() method on DatastoreProvider
• Add scope field to DatastoreSyncOptions for prefix-scoped pull/push (orthogonal to relPath which is markDirty-only)
• Gate acquireModelLocks flush on scopedSync: when true, iterate per-model with scoped pushChanged/pullChanged calls and no global lock; when false/absent, preserve current global-lock behavior unchanged
• Update testing package (datastore_types.ts, datastore_conformance.ts) so extension authors see the new contract and providers without capabilities() still pass conformance

Phase 1 of the scoped sync roadmap documented in design/datastores.md. Delivers the framework contracts so document-store backends (e.g. @keeb/mongodb-datastore) can declare scopedSync: true and benefit immediately. S3/GCS need index partitioning (Phase 3) before they can opt in.

Test Plan

• deno check — all files pass
• deno lint — clean
• deno fmt — clean
• deno run test — 5933 passed, 0 failed
• deno run compile — binary compiles successfully
• Testing package compatibility test passes (testing_package_compat_test.ts)
• 3 new unit tests: scoped sync path (scope passed to pull/push, no global lock), default-path regression (global lock, no scope), deduplication (duplicate models → one scoped push)
• Manual test: filesystem datastore — model create + method run
• Manual test: S3 datastore against Minio — model create, method run, sync, status (default path, no scopedSync declared)

🤖 Generated with Claude Code

[github-actions]

Code Review

Clean, well-designed PR. The capability-gating pattern is sound — providers opt in explicitly, the default path is preserved unchanged, and the safety invariant is clearly documented. The domain interfaces, testing package updates, and conformance checks all align. No blocking issues.

Suggestions

1. Hoist capabilities() check above the per-model pull loop (src/cli/repo_context.ts:941): customProvider?.capabilities?.()?.scopedSync is evaluated inside the for (const { modelType, modelId } of unique) loop, so it's called N times during pull (once per model). Since capabilities are stable per provider instance, this could be computed once before the loop — similar to how scopedSync is already computed once at line 962 for the flush closure. Minor efficiency nit, not a correctness issue.

2. Scoped push stops on first failure (src/cli/repo_context.ts:973-1001): In the scoped push path, if one model's push throws, remaining models' scoped pushes are skipped. The unpushed changes remain in local cache and will be retried on next flush, so this is safe. Worth noting if you anticipate wanting partial-success semantics in a later phase.

[github-actions]

Adversarial Review

Critical / High

None found.

Medium

1. capabilities() called redundantly inside the per-model pull loop (src/cli/repo_context.ts:941). The expression customProvider?.capabilities?.()?.scopedSync === true is evaluated on every iteration of the for (const { modelType, modelId } of unique) loop, then evaluated again at line 962 for the flush closure. Since capabilities() is a pure configuration query, these should all return the same value — but re-invoking an extension method N+1 times (once per model + once for flush) is unnecessary work and creates a theoretical TOCTOU window if a buggy extension mutates state between calls. Hoist the check once before the loop:

   const useScopedSync = customProvider?.capabilities?.()?.scopedSync === true;

   Then reuse that boolean in both the pull loop and the flush closure.

2. Conformance test passes null as a valid capabilities return (packages/testing/datastore_conformance.ts:170-173). The assertion assertEquals(typeof caps, "object", ...) accepts null because typeof null === "object" in JavaScript. If a provider returns null, the test passes the type check but then crashes with an unhelpful TypeError: Cannot read properties of null on the caps.scopedSync access at line 175. Production code is safe (optional chaining handles null), but the conformance test should guard against it:

   assertNotEquals(caps, null, "capabilities() must not return null");

Low

1. lockedModels = [...unique] is a redundant copy (src/cli/repo_context.ts:963). unique is a local const array created by sorted.filter(...) — it's never mutated after creation and never escapes the function. The spread copy is harmless but unnecessary.

2. Scoped push partial failure leaves remote in a half-pushed state (src/cli/repo_context.ts:973-1001). If model A's scoped push succeeds and model B's fails, model A's changes are persisted remotely while model B's are not, and all per-model locks are released in the finally. This is architecturally inherent to the scoped design (and not fundamentally worse than a global push failing mid-upload), and the SAFETY INVARIANT comment makes the contract clear. Flagging for awareness, not as a defect.

Verdict

PASS — The capability-gating logic is correct, the fallback to global-lock behavior for providers without capabilities() preserves backward compatibility, the deduplication and deterministic lock ordering are sound, and the test coverage hits the three key paths (scoped, unscoped, dedup). The medium items are minor quality improvements, not correctness bugs.