Conversation
…nfig resolution resolveDatastoreConfig() calls resolveDatastoreType() which checks the datastore type registry. After PR #1050's lazy loading change, the registry is empty at this point because ensureLoaded() hasn't been called yet. This causes the auto-resolver to fire unnecessarily, writing progress JSON ({"event":"auto_resolve","status":"searching","type":"@swamp/..."}) to stdout — corrupting --json output for any command that uses an extension-based datastore (S3, GCS). Add ensureLoaded() calls before auto-resolve checks in all three code paths within resolveDatastoreConfig() and parseDatastoreEnvVar(), so the registry is populated from disk before the auto-resolver is consulted. Fixes datastore UAT failures: status_test.ts:14, sync_test.ts:19, sync_test.ts:96 on both S3 and GCS backends.
There was a problem hiding this comment.
Code Review
Clean, well-scoped bug fix. The root cause analysis is solid — after PR #1050's lazy loading change, datastoreTypeRegistry is empty when resolveDatastoreConfig() / parseDatastoreEnvVar() check it, causing the auto-resolver to fire and leak progress JSON to stdout.
Blocking Issues
None.
Suggestions
- Pre-existing import boundary violation (not introduced by this PR): Line 44 imports
maybeAutoUpdateDatastoreExtensionfrom../libswamp/extensions/datastore_auto_update.tsinstead of../libswamp/mod.ts. Per CLAUDE.md, CLI code should import fromsrc/libswamp/mod.ts. Worth fixing in a follow-up since the symbol is already re-exported frommod.ts. - The comment block at lines 422-425 referencing "PR #1050" is useful context now but will become noise over time. A shorter comment like
// Ensure lazy-loaded extensions are populated before registry lookupwould age better. The detailed explanation fits well in the commit message (where it already lives).
LGTM — the fix correctly ensures the lazy-loaded registry is populated before each auto-resolve check point, and the existing test file covers the public API surface.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
resolveDatastoreConfig()callsresolveDatastoreType()which checks the datastore type registry. After PR fix: lazy extension loading — only load registries when accessed #1050's lazy loading change, the registry is empty at this point becauseensureLoaded()hasn't been called yet.{"event":"auto_resolve","status":"searching","type":"@swamp/s3-datastore"}) to stdout — corrupting--jsonoutput for any command that uses an extension-based datastore (S3, GCS).ensureLoaded()calls before auto-resolve checks in all three code paths withinresolveDatastoreConfig()andparseDatastoreEnvVar().The rogue JSON was 74 chars for S3 (position 75) and 75 chars for GCS (position 76), matching the 1-char difference between
@swamp/s3-datastoreand@swamp/gcs-datastore.Test Plan
deno check,deno lint,deno fmtall cleanstatus_test.ts:14,sync_test.ts:19,sync_test.ts:96on both S3 and GCS backends (CI run: https://github.com/systeminit/swamp-uat/actions/runs/23912975675)🤖 Generated with Claude Code