fix: exclude datastore-bundles from DEFAULT_DATASTORE_SUBDIRS#1104
Merged
fix: exclude datastore-bundles from DEFAULT_DATASTORE_SUBDIRS#1104
Conversation
The datastore loader runs during bootstrap before the DatastorePathResolver exists, so datastore extension bundles must always remain in local .swamp/. Including datastore-bundles in the datastore tier (added in the #1100 fix) caused the S3 datastore setup migration to delete .swamp/datastore-bundles/ after copying to cache, breaking subsequent S3 extension loading on all commands. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
There was a problem hiding this comment.
Code Review
No blocking issues found. This is a clean, well-scoped regression fix.
Highlights
- Root cause is well-documented: The comment at
datastore_config.ts:59-63clearly explains the bootstrap ordering constraint — whydatastore-bundlesmust stay local whiledriver-bundlesandreport-bundlescan live in the datastore tier. - Tests updated correctly: Both the
isDatastoreSubdirandresolvePathtests now verify thatdatastore-bundlesresolves to the local.swamp/path, not the S3 cache path. - Minimal blast radius: Only touches the config constant and its tests — no unrelated changes.
- DDD: The domain configuration module correctly captures the invariant that datastore extension bundles have different lifecycle constraints than other bundle types due to bootstrap ordering.
LGTM — approve.
There was a problem hiding this comment.
Adversarial Review
Critical / High
None.
Medium
None.
Low
design/extension.mddoesn't mention the bootstrap constraint — The design doc listsdatastore-bundlesalongside other bundle dirs without noting why it's excluded from the datastore tier. This is purely a docs concern and doesn't affect correctness, since the code comment indatastore_config.ts:59-63is clear.
Verdict
PASS — Correct, minimal fix. The removal of "datastore-bundles" from DEFAULT_DATASTORE_SUBDIRS is well-motivated by the bootstrap ordering constraint (the datastore loader runs before the DatastorePathResolver exists, so its bundles must remain in local .swamp/). The change is consistent with the existing design doc, which already documents datastore-bundles/ as living in .swamp/datastore-bundles/. The inline comment is precise about why and what breaks without it. Tests are updated to verify both the isDatastoreSubdir return value and the path resolution behavior.
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
datastore-bundlesfromDEFAULT_DATASTORE_SUBDIRSto fix S3 datastore sync regression introduced in fix: resolve bundle paths through DatastorePathResolver for S3 datastores #1103Root Cause
PR #1103 added
datastore-bundlestoDEFAULT_DATASTORE_SUBDIRSalongsidedriver-bundlesandreport-bundles. This caused the S3 datastore setup migration to:.swamp/datastore-bundles/(containing S3 extension bundles) to the S3 cache.swamp/datastore-bundles/from local on successful migrationOn subsequent commands, the datastore loader (which runs during bootstrap before the
DatastorePathResolverexists and therefore always reads from local.swamp/) could no longer find the S3 extension bundles. This broke all S3 datastore operations — the extension failed to load,resolveDatastoreConfigcouldn't resolve the S3 config, and sync operations failed silently.Why This Is Correct
The
UserDatastoreLoaderis explicitly excluded from resolver wiring due to bootstrap ordering — it loads datastore extensions that configure the resolver. Sodatastore-bundlesmust always remain in local.swamp/, never in the datastore tier. This mirrors the exclusion ofUserDatastoreLoaderfrom the resolver in #1103.driver-bundlesandreport-bundlesremain in the datastore tier because their loaders do receive the resolver and can find bundles in the cache path.Test Plan
datastore-bundlesis NOT recognized as a datastore subdir and resolves to local.swamp/path🤖 Generated with Claude Code