CLI UX Review

Blocking

None.

Suggestions

1. Message format inconsistency vs. #121 precedent (src/cli/resolve_datastore.ts:208–212)
   The #121 auto-resolver refusal (renderAutoResolveAlreadyInstalled) renders as three separate logger.error lines and supports a JSON-mode branch. This PR emits a single long logger.warn string with no JSON branch. The PR acknowledges the JSON-mode silence as swamp-club#130 (pre-existing, ~50 sites), which is fair — data is still protected in JSON mode. But consider splitting the warn into two lines the way the #121 renderer does, so it's easier to scan:

   Not auto-updating @swamp/s3-datastore 2026.03.15.1 → 2026.03.30.1: local edits detected under .swamp/pulled-extensions/@swamp/s3-datastore/.
   Run `swamp extension pull @swamp/s3-datastore --force` to discard your edits and install the latest version.
   

2. Optional version fields in buildLocalEditsWarning (src/cli/resolve_datastore.ts:207)
   The signature accepts { previousVersion?: string; newVersion?: string } but if either is undefined the output would read …undefined → undefined…. In practice the service always populates both when it returns skipped: "local_edits", but narrowing the parameter type (or adding a ?? "(unknown)" fallback) would future-proof the message.

Verdict

PASS — warning message is clear, actionable, and consistent in form with other warn-level messages in the same file. Data protection works in both log and JSON modes; the JSON-mode notification gap is a pre-existing issue correctly tracked separately.

Code Review

Clean, well-designed fix for the data-loss issue. The DDD layering is correct: pure domain combiner in src/domain/extensions/installed_extension_digest.ts (no FS access), infrastructure reader in src/infrastructure/persistence/installed_extension_digest_reader.ts (walks disk, produces entries), and detectLocalEdits injected as a dependency into the auto-update service — proper dependency inversion. The tri-state match | mismatch | no-anchor design with a structured result (not an exception) for the refusal path is a good fit for the "never block command execution for auto-update failures" contract.

Test coverage is thorough: 7 tests for digest determinism/sensitivity, 6 tests for the FS reader (including macOS resource fork exclusion and edge cases), 3 tests for the service-layer tri-state decision tree, and 1 test for the warning message shape. All new files have the AGPLv3 header. License headers, test naming conventions, and file placement all match CLAUDE.md requirements.

Blocking Issues

None.

Suggestions

1. Export new types from src/libswamp/mod.ts: LocalEditsStatus and DatastoreAutoUpdateSkipReason are part of the public interface of already-exported types (DatastoreAutoUpdateDeps.detectLocalEdits returns Promise<LocalEditsStatus>; DatastoreAutoUpdateResult.skipped is typed as DatastoreAutoUpdateSkipReason). External consumers implementing DatastoreAutoUpdateDeps or inspecting the result would need these types. Consider adding them to the re-export block at lines 568–571.

2. Symlinks silently excluded from digest: walk() in installed_extension_digest_reader.ts only processes entry.isFile entries. In Deno, symlinks have isSymlink: true / isFile: false, so a user-added symlink wouldn't change the digest. Unlikely in practice for extension directories, but worth a brief note or a test confirming the intentional exclusion.

3. Pre-existing (not introduced by this PR): resolve_datastore.ts:44 imports maybeAutoUpdateDatastoreExtension from the internal libswamp path ../libswamp/extensions/datastore_auto_update.ts rather than from ../libswamp/mod.ts, which conflicts with the CLAUDE.md import boundary rule. Not a regression — just noting for a future cleanup pass.

Adversarial Review

Critical / High

None found.

Medium

1. Duplicated isMacOsResourceFork — divergence invalidates every anchor (src/infrastructure/persistence/installed_extension_digest_reader.ts:78 and src/libswamp/extensions/pull.ts:211): Both files define an identical private isMacOsResourceFork(name) helper. The install-time digest (via pull.ts's copyDir) and the check-time digest (via the reader's walk) must agree on which files exist. If someone modifies one function but not the other — e.g., adding .DS_Store filtering to the installer but not the reader — every existing filesChecksum anchor silently becomes invalid, and all auto-updates start reporting false "local edits detected" for every extension. This isn't a current bug, but it's a maintenance landmine with blast radius proportional to the number of installed extensions. Consider extracting a shared shouldSkipEntry(name) predicate or at minimum a cross-reference comment.

Low

1. TOCTOU between detectLocalEdits and pullExtension (src/libswamp/extensions/datastore_auto_update.ts:155-178): Between detectLocalEdits returning "match" and pullExtension executing, the user could edit a file. The protection degrades silently to the pre-fix behavior (overwrite). Inherent in any check-then-act pattern; acceptable here because auto-update is a convenience feature, not a security boundary.

2. Symlinks silently excluded from digest (src/infrastructure/persistence/installed_extension_digest_reader.ts:61-75): walk only processes entry.isFile and entry.isDirectory. In Deno's readDir, symlinks have isSymlink: true but isFile: false and isDirectory: false, so they are silently skipped. This is consistent with the installer's copyDir (which also only handles files and directories), so install-time and check-time digests agree. But a user who adds a symlinked file to the extension tree would not trigger the protection. Very unlikely in practice.

3. Double lockfile read per auto-update (src/cli/resolve_datastore.ts:101-103 and 119): getInstalledVersion and detectLocalEdits each independently call readUpstreamExtensions(lockfilePath). Two reads per check is a minor inefficiency, not a correctness issue.

Verdict

PASS — The code is well-structured and handles edge cases thoughtfully. The tri-state detectLocalEdits contract is clean, the degradation paths ("no-anchor", catch → "no-anchor") correctly prevent the safety check from ever blocking a user command, and the lockfile anchor is written at the right point in the install lifecycle (after all file writes, before the lockfile commit). The one medium finding is a maintenance concern, not a shipping blocker.