Review Summary

This is a well-designed PR that adds configurable datastores with S3 sync and distributed locking. The implementation follows domain-driven design principles and adheres to the project's code style guidelines.

Architecture (DDD Perspective)

The architecture properly separates concerns across layers:

• Domain layer (src/domain/datastore/): Pure interfaces (DistributedLock, DatastorePathResolver, DatastoreVerifier) and value objects (LockInfo, DatastoreConfig)
• Infrastructure layer (src/infrastructure/persistence/): Concrete implementations (FileLock, S3Lock, S3Client, S3CacheSyncService)
• CLI layer: Thin adapters that wire domain/infrastructure and handle presentation

Code Quality Checks

• TypeScript strict mode - all types properly defined
• Named exports used throughout
• AGPLv3 copyright headers present on all new files
• The type AnyOptions = any pattern in CLI commands follows established codebase convention (60+ files use it)

Test Coverage

Comprehensive unit tests provided:

• resolve_datastore_test.ts - Config resolution logic
• datastore_pattern_matcher_test.ts - Gitignore-style glob patterns
• distributed_lock_test.ts - LockTimeoutError behavior
• file_lock_test.ts - FileLock acquire/release/timeout/stale handling
• s3_lock_test.ts - S3Lock with mock client
• default_datastore_path_resolver_test.ts - Path resolution logic

Security Review

• Atomic file operations for lock creation (createNew: true)
• S3 conditional writes for lock acquisition (If-None-Match: *)
• Lock TTL + heartbeat prevents indefinite locks
• Force-release requires explicit --force flag
• Proper error handling without credential leakage
• No OWASP vulnerabilities identified

No Blocking Issues

The PR is well-structured and ready to merge.

Suggestions (non-blocking)

1. S3CacheSyncService (~500 lines) could be split into smaller components in future iterations if it grows further
2. Consider adding a dedicated test file for datastore_migration_service.ts in a future PR (migration is currently tested indirectly via setup commands)
3. The design document in design/datastores.md is excellent - great reference for future maintainers

LGTM! Excellent work on this feature.

Adversarial Review

Critical / High

1. CRITICAL - Path Traversal in S3 Cache Sync (s3_cache_sync.ts:139-143)

   S3 object keys can contain path traversal characters (../). When pulling files from S3, keys are used directly in path.join() which normalizes .. sequences, allowing escape from the cache directory.

   Breaking example:

   • Attacker controls S3 bucket (compromised credentials, malicious team member, social engineering)
   • Attacker creates S3 object with key: ../../../home/user/.bashrc
   • User runs swamp datastore sync --pull
   • Code executes: join(cachePath, "../../../home/user/.bashrc") → /home/user/.bashrc
   • Attacker content overwrites user's .bashrc (arbitrary file write)

   Affected code paths:

   • pullFile() line 141: const localPath = join(this.cachePath, relativePath);
   • pullChanged() uses keys from S3 index
   • pullAll() uses keys from listAllObjects()

   Fix: Validate that the resolved path is within the cache directory:

   const localPath = join(this.cachePath, relativePath);
   if (!localPath.startsWith(this.cachePath + "/")) {
     throw new Error(`Path traversal detected: ${relativePath}`);
   }

2. HIGH - Race Condition in Stale Lock Force-Acquire (s3_lock.ts:106-121, file_lock.ts:109-119)

   The stale lock detection has a TOCTOU race between checking staleness and deleting:

   Breaking example:

   1. Process A reads stale lock, decides to force-acquire (line 112)
   2. Process B legitimately acquires a fresh lock (replaces the stale one)
   3. Process A deletes Process B's fresh lock (line 115)
   4. Process A and Process C both acquire "the lock"
   5. Two processes hold the lock simultaneously → data corruption

   Fix: Use conditional delete. For S3, check the ETag matches the stale lock before deleting. For filesystem, use the lockfile mtime to verify it hasn't changed.

3. HIGH - Heartbeat Race on Lock Release (s3_lock.ts:161-168, file_lock.ts:164-170)

   The heartbeat's extend() method can race with release():

   Breaking example:

   1. Heartbeat interval fires, extend() starts executing
   2. extend() passes the if (!this.held) guard (held is still true)
   3. release() is called, sets this.held = false, deletes lock
   4. Process B acquires the lock
   5. extend() completes its await writeTextFile/putObject, overwriting Process B's lock

   Fix: Use a mutex or check this.held again after the async write, then delete if the lock was released while extending.

4. HIGH - Degraded Mode Silently Allows Concurrent Corruption (datastore_sync_coordinator.ts:77-84)

   If lock acquisition fails (network issue, permissions), the code logs a warning and continues without a lock. This silently degrades to unsafe mode where concurrent commands can corrupt data.

   Breaking example:

   • S3 bucket has restrictive permissions, lock acquire fails
   • User runs two swamp model run commands concurrently
   • Both proceed without locks → interleaved writes → data corruption
   • User only sees a warning in logs, not an error

   Fix: Lock acquisition failure should be fatal for data-mutating operations, not a warning.

Medium

5. MEDIUM - Hidden Files Excluded from pushChanged() (s3_cache_sync.ts:305-309)

   pushChanged() skips ALL files starting with . via rel.startsWith("."), but pullAll() only skips explicit metadata files. This asymmetry means:

   • Legitimate hidden files (.gitkeep, hidden output files) won't sync to S3
   • Other machines won't receive these files

   Fix: Only skip the specific metadata files, not all hidden files.

6. MEDIUM - No Validation of S3 Bucket Name (resolve_datastore.ts:63-72)

   The SWAMP_DATASTORE=s3:... parsing accepts any string as a bucket name without validation. Invalid bucket names (uppercase, special chars, wrong length) will fail later with cryptic AWS errors.

   Fix: Validate bucket name format before use.

7. MEDIUM - Response.Body! Non-Null Assertion (s3_client.ts:98)

   return new Uint8Array(await response.Body!.transformToByteArray()); - the ! assertion will throw a confusing error if Body is null/undefined.

   Fix: Check for null and throw a descriptive error.

Low

8. LOW - Migration Doesn't Preserve mtime (datastore_migration_service.ts:103-105)

   Deno.copyFile doesn't preserve modification times. Since sync uses mtime for change detection, this could cause spurious sync operations after migration.

9. LOW - SIGINT Handler Doesn't Await Release (datastore_sync_coordinator.ts:72-74)

   Deno.exit(130) is called immediately without awaiting lock.release(). The release may not complete before process exit.

10. LOW - Multiple requireInitializedRepo Calls Would Deadlock (latent)

    If requireInitializedRepo is called twice in the same process (nested commands, bugs), the second call creates a new lock instance and tries to acquire, blocking on the first lock. Eventually times out after 60s, but poor UX.

Verdict

FAIL - The path traversal vulnerability (finding #1) is a critical security issue that allows arbitrary file write when syncing from a malicious/compromised S3 bucket. The lock race conditions (#2, #3) undermine the distributed locking guarantees that are the core feature of this PR. These must be fixed before merge.

Code Review: Configurable Datastores with S3 Sync and Distributed Locking

This is an excellent, well-architected PR that adds significant functionality while maintaining clean code and following project conventions. No blocking issues found.

Domain-Driven Design Assessment

The implementation demonstrates excellent adherence to DDD principles:

• Clean layer separation: Domain interfaces (DistributedLock, DatastorePathResolver, DatastoreVerifier) are properly abstracted with infrastructure implementations
• Value objects: DatastoreConfig is a proper discriminated union type; LockInfo and LockOptions are well-structured
• Domain errors: LockTimeoutError extends Error properly with useful metadata
• Repository pattern: Only aggregate roots get repositories, following DDD guidance

Code Quality

Positive:

• All files include required AGPLv3 license headers
• Named exports used throughout (no default exports)
• All commands support both log and json output modes as required
• TypeScript strict mode followed (no new type violations)
• S3 bucket name validation (resolve_datastore.ts:38-47)
• Path traversal prevention in s3_cache_sync.ts:37-48 with assertSafePath()
• Thorough design document at design/datastores.md

Test Coverage:

• FileLock - 7 test cases covering acquire, release, timeout, stale locks
• S3Lock - 7 test cases with mock S3Client
• DatastorePatternMatcher - 10 test cases for glob patterns
• DefaultDatastorePathResolver - 10 test cases
• resolveDatastoreConfig - 5 test cases covering priority resolution
• LockTimeoutError - 3 test cases

Security

• S3 conditional writes (If-None-Match: *) for atomic lock acquisition
• TTL-based lock expiration prevents stuck locks from crashed processes
• SIGINT handler for best-effort lock release on Ctrl-C
• Helpful error messages for common AWS credential/permission issues

Architecture Highlights

• Shared S3Client instance between lock and sync service avoids duplicate credential resolution
• Lock-only mode for filesystem datastores keeps the coordinator generic
• Concurrent transfers in batches of 10 balances performance with rate limits
• Size+mtime change detection avoids expensive content hashing

Suggestions (non-blocking)

The type AnyOptions = any pattern in CLI commands follows the established codebase convention (used in 64+ existing command files) - not a concern for this PR.

Overall, this is production-quality code with thoughtful design decisions, comprehensive testing, and excellent documentation. The PR is ready to merge.

Adversarial Review

Critical / High

1. HIGH: Lock theft via unconditional heartbeat overwrites (s3_lock.ts:168-175, file_lock.ts:174-180)

   Both S3Lock.extend() and FileLock.extend() use unconditional writes (s3.putObject / Deno.writeTextFile) to refresh the lock. This creates a race condition where a recovered process can overwrite another process's legitimately acquired lock:

   Breaking scenario:

   • T=0: Process A acquires lock
   • T=5: Process A becomes unresponsive (network partition, GC pause, etc.)
   • T=40: Process B sees lock is stale (TTL=30s expired), deletes it, acquires with conditional write
   • T=40.2: Process A recovers, heartbeat fires, extend() runs
   • extend() checks this.held (still true - A never released), then does unconditional putObject/writeTextFile
   • A's lock info overwrites B's lock
   • Both A and B now believe they own the lock and proceed to modify data concurrently

   Data corruption result: Two processes simultaneously hold the "distributed" lock, defeating the entire purpose of locking.

   Fix: Use conditional writes for heartbeat extension. For S3, store the ETag on acquire and use IfMatch for extend. For filesystem, use file descriptor locking or atomic rename with a nonce. Alternatively, extend() should verify the lock content still belongs to this process before overwriting.

2. HIGH: S3Lock stale detection TOCTOU allows lock theft (s3_lock.ts:107-127)

   The stale lock detection has a time-of-check-time-of-use race:

   const recheck = await this.s3.headObject(this.lockKey);
   if (recheck.lastModified.getTime() === head.lastModified.getTime()) {
     await this.s3.deleteObject(this.lockKey);  // <-- TOCTOU gap here
   }

   Between the headObject recheck and the deleteObject, another process's heartbeat could refresh the lock. The delete then removes a valid lock, and the subsequent putObjectConditional succeeds, stealing the lock.

   Fix: Use S3 Object Lock or versioning with conditional deletes, or accept that stale lock cleanup is best-effort and increase TTL tolerance.

Medium

3. MEDIUM: Partial push failure leaves index inconsistent (s3_cache_sync.ts:350-369)

   In pushChanged(), if some files fail to push (e.g., transient S3 errors), the code still updates the index for successful files and pushes the index to S3. This means the index claims files exist in S3 that weren't actually pushed.

   Breaking scenario: Push 10 files, 3 fail due to network blip. Index says all 10 exist. Later pullChanged() on another machine fails silently when fetching the 3 missing files (error swallowed at line 188-201).

   Fix: Only update index after all pushes succeed, or track failed pushes and retry before updating index.

4. MEDIUM: getCachePath lacks path traversal protection (s3_cache_sync.ts:470-472)

   getCachePath(relativePath) does a simple join(cachePath, relativePath) without calling assertSafePath(). If any caller passes user-controlled or S3-derived paths through this method and then writes to the result, path traversal is possible.

   Currently this appears to only be used for informational purposes, but it's a landmine for future code.

   Fix: Apply assertSafePath() in getCachePath() or document it as internal-only.

5. MEDIUM: SIGINT handler may hang indefinitely (datastore_sync_coordinator.ts:72-76)

   The signal handler awaits lock.release() with no timeout:

   signalHandler = () => {
     lock.release().catch(() => {}).finally(() => {
       Deno.exit(130);
     });
   };

   If S3 is unreachable, deleteObject may hang for the full HTTP timeout (could be 60+ seconds), leaving the user stuck on Ctrl-C.

   Fix: Add a timeout (e.g., 5 seconds) before force-exiting.

6. MEDIUM: Force release can delete active lock (datastore_lock.ts:97-124)

   datastoreLockReleaseCommand does inspect() then unconditionally deletes. Between inspect and delete, another process could acquire the lock legitimately. The force release deletes their active lock.

   This is documented as a "breakglass" operation, but should at least verify the holder info matches what was inspected before deleting.

Low

7. LOW: hasSymlinks recursion may fail on permission errors (datastore_setup.ts:91-103)

   If any subdirectory in models/workflows/vaults is unreadable, readDir throws and fails the entire setup, even if the unreadable dir doesn't contain symlinks.

8. LOW: Index modification during concurrent pulls (s3_cache_sync.ts:192-197)

   pullChanged() modifies this.index.entries[rel] inside Promise.allSettled batches. While currently safe (each file writes to a different key), this pattern is fragile if the index structure changes.

9. LOW: Missing null check on S3 ListObjects keys (s3_client.ts:198-199)

   .map((obj) => obj.Key!) uses a non-null assertion. While unlikely, malformed S3 responses could cause crashes.

Verdict

FAIL - The unconditional heartbeat writes (Finding #1) allow two processes to simultaneously believe they hold the lock, completely defeating the distributed locking mechanism and enabling data corruption. This is a fundamental design flaw that must be fixed before merge.

The stale detection TOCTOU (Finding #2) compounds this issue by providing another path to lock theft.