feat(s3,gcs): namespace manifest support for registerNamespace/listNamespaces (#547) #28
Loading…
Reference in a new issue
No description provided.
Delete branch "worktree-547"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
registerNamespace()andlistNamespaces()on both S3 and GCSDatastoreProviderimplementationsregisterNamespacewrites a.namespace.jsonmanifest to{namespace}/.namespace.jsonin the bucket with check-then-write conflict detection (differentrepoId→ error)listNamespacesscans the bucket for all*/.namespace.jsonkeys and returns namespace slugsWhat changed
datastore/s3/extensions/datastores/_lib/interfaces.tsregisterNamespace?andlistNamespaces?toDatastoreProviderinterfacedatastore/gcs/extensions/datastores/_lib/interfaces.tsdatastore/s3/extensions/datastores/s3.tsS3DatastoreProviderImpldatastore/gcs/extensions/datastores/gcs.tsGcsDatastoreProviderImpl, importedNotFoundErrordatastore/s3/extensions/datastores/s3_test.tsdatastore/gcs/extensions/datastores/gcs_test.tsdatastore/s3/manifest.yaml2026.06.03.1→2026.06.03.2datastore/gcs/manifest.yaml2026.06.03.1→2026.06.03.2datastore/s3/README.mddatastore/gcs/README.mdDesign decisions
registerNamespaceis a rare, human-initiated operation. If the PUT fails, the user re-runs the command..datastore.lockin the lock implementation.DatastoreProviderinterface from Phase 5.error.name === "NotFound" || "NoSuchKey", GCS catchesinstanceof NotFoundError.Verification
Unit tests (mock HTTP servers)
E2E: S3 via MinIO (full CLI path)
swamp datastore namespace set infra— manifest written, no warningaws s3 cp s3://manifest-test/infra/.namespace.json -— valid JSON with{namespace, repoId, registeredAt}namespace set infra—Error: 'Namespace "infra" is already registered'swamp datastore namespace list— shows bothinfraandsecurityfrom bucketswamp datastore status— healthy, all dirs present (solo mode regression pass)E2E: GCS via real bucket (
swamp-uat-gcs-datastore-testing)Test plan
swamp datastore namespace set/listCLIdeno check,deno lint,deno fmt --check,deno install --frozenpass for both extensionsCloses #547
Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com
Implement namespace manifest support for multi-repo conflict detection on S3 and GCS datastore providers. registerNamespace writes a .namespace.json manifest to {namespace}/.namespace.json in the bucket with check-then-write conflict detection. listNamespaces scans the bucket for all registered namespace manifests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>Code Review
Blocking Issues
None.
Suggestions
TOCTOU race in
registerNamespace(both providers) — The check-then-write pattern (getObject→ inspect repoId →putObject) is not atomic. Two concurrentregisterNamespacecalls for the same namespace with differentrepoIds can both pass the check before either write lands, causing the later write to silently overwrite the first. The README documents this honestly as "check-then-write conflict detection," so it appears intentional, but it's worth noting that GCS already exposesputObjectConditional(used by the lock path forifGenerationMatch=0CAS). Using that for a write-if-not-exists on first registration would close the window without extra round-trips. S3 similarly supportsIf-None-Match: *. Low urgency for an advisory detection mechanism, but worth a follow-up.datastore/gcs/extensions/datastores/gcs.ts:165–191datastore/s3/extensions/datastores/s3.ts:159–193S3
registerNamespace: redundant catch branch — Theelse if (error instanceof Error && error.message.startsWith("Namespace"))branch re-throws, and the trailingelsealso re-throws. Both arms do identical work, so the middle branch is dead code. Collapsing to a singleelse { throw error }would make the intent clearer.datastore/s3/extensions/datastores/s3.ts:172–185listNamespacesissues a full-bucket scan — Both implementations calllistAllObjects()with no key-prefix hint and then filter by suffix in application code. For a bucket with many objects (large deployment, many model files), this can be expensive. If the client API accepts a prefix argument, passing something like.namespace.jsonwould narrow the list. Alternatively, writing namespace manifests into a dedicated_namespaces/directory would allow a prefix-scoped list.datastore/gcs/extensions/datastores/gcs.ts:194–208datastore/s3/extensions/datastores/s3.ts:195–209Prefix handling not verifiable from changed files — Both
registerNamespaceandlistNamespacesconstruct keys as${namespace}/.namespace.jsonand calllistAllObjects()using a client built from the full config (which may include a user-suppliedprefix). IfGcsClient/S3Clientdo not applythis.config.prefixto these calls, then two installations sharing a bucket under different prefixes (e.g.,prodvsstaging) would see each other's namespace registrations. Worth a quick cross-check against the client implementations to confirm the prefix is applied consistently to namespace-path keys.Adversarial Review
Critical / High
TOCTOU race condition in registerNamespace — conflict detection is defeated by concurrent callers — datastore/s3/extensions/datastores/s3.ts:159-192 and datastore/gcs/extensions/datastores/gcs.ts:165-191
Both implementations use a non-atomic check-then-write pattern: GET the manifest, inspect repoId, then unconditionally PUT. Between the GET and PUT, another process can complete its own GET+PUT cycle, and the second writer silently overwrites the first without detecting the conflict.
Breaking scenario:
Both repos now believe they own the infra namespace. The README and interface doc explicitly describe this as conflict detection, so users will rely on it to prevent two repos from stomping on each other.
Suggested fix: Both clients already expose putObjectConditional — GCS uses ifGenerationMatch=0 and S3 uses IfNoneMatch: *. The first-time registration should use putObjectConditional instead of plain putObject. If it returns false/null (object already exists), then GET the existing manifest and check repoId. This makes first-writer-wins atomic. The re-registration (same repoId) path can still use plain putObject.
Severity: HIGH — This defeats the stated purpose of the feature (multi-repo conflict detection). Two repos racing to register the same namespace will silently corrupt state.
Medium
listNamespaces downloads the entire object listing to find namespace manifests — datastore/s3/extensions/datastores/s3.ts:195-209 and datastore/gcs/extensions/datastores/gcs.ts:194-208
listAllObjects() is called with no sub-prefix, which pages through every object under the configured bucket prefix. In a production datastore with thousands or millions of objects, this will be very slow and memory-intensive just to find the handful of .namespace.json files.
S3/GCS only support prefix-based filtering (not suffix), so the current key scheme does not allow efficient listing. A common pattern is to store manifests under a well-known prefix like .namespaces/slug.json, which enables a targeted listing that returns only manifest entries.
Not a correctness bug, but will degrade as datastores grow.
S3 registerNamespace error matching is fragile for corrupted manifest JSON — datastore/s3/extensions/datastores/s3.ts:172-185
If the stored .namespace.json contains syntactically valid JSON with an unexpected shape (e.g. repoId is null from a partial write or manual edit), then existing.repoId !== repoId evaluates to null !== "repo-aaa" which is true, and the error message becomes: Namespace "infra" is already registered by repo null. This is technically correct (it blocks the registration) but could be confusing to operators and masks a data corruption issue. Consider validating the parsed shape before comparing.
Low
Re-registration unconditionally overwrites the manifest — datastore/s3/extensions/datastores/s3.ts:187-192 and datastore/gcs/extensions/datastores/gcs.ts:186-191
When existing.repoId equals the provided repoId (idempotent re-registration), the code falls through to putObject and writes a new manifest with a fresh registeredAt timestamp. Every call writes to the bucket even when nothing changed. Not a bug — likely intentional idempotency — but an unnecessary write that could be avoided with an early return when the existing manifest already matches.
Verdict
FAIL — The TOCTOU race in registerNamespace (finding 1) defeats the stated purpose of multi-repo conflict detection. Two concurrent registrations for the same namespace with different repoIds can silently overwrite each other. Both providers already have putObjectConditional available to make first-writer-wins atomic. This should be fixed before merge.
Code Review
This PR adds
registerNamespace/listNamespacesto both the GCS and S3 datastore providers, replacing a non-atomic GET-then-PUT withputObjectConditionalfor first-writer-wins atomicity. The core mechanism is correct: conditional writes prevent TOCTOU races on initial registration, the conflict-detection read-back is well-structured, and tests cover the full lifecycle (create, idempotent re-register, conflict) using local mock servers with no live cloud dependencies.Blocking Issues
None.
Suggestions
Inconsistent
putObjectConditionalreturn-value checks (gcs.ts:173 vs s3.ts:167)GCS uses
if (created !== null) return;while S3 usesif (created) return;. If both underlying clients returnnullon a 412, the S3 check is also correct (null is falsy), but the divergence makes the invariant implicit. A short comment naming the expected return type contract for each client would prevent a future maintainer from accidentally inverting the logic.Unsafe
JSON.parsecast inregisterNamespace(gcs.ts:178, s3.ts:172)If the stored manifest is malformed or written by an old client that omitted
repoId,existing.repoIdisundefinedand the error message reads "already registered by repo undefined". Adding a simple presence check (if (typeof existing?.repoId !== "string")) before the comparison would give a clearer diagnostic.Namespace value used raw in object key (gcs.ts:163, s3.ts:157)
If
namespaceever contains a path-separator sequence like../the object key will be syntactically valid in both GCS and S3 (object stores treat keys as opaque strings, so this isn't a filesystem path-traversal risk) but it could silently produce keys outside the expected namespace layout. A guard rejecting slashes or..components would make the contract explicit and protect future callers.GCS README documents
defaultRequestTimeoutMsbut schema doesn't include it (gcs/README.md, gcs.ts:72–106)The README's configuration table lists
defaultRequestTimeoutMsas a supported user-facing option, butgcsConfigSchemaandGcsDatastoreProviderConfighave no such field. Zod's default strip behaviour means any value a user configures will be silently discarded before it reachesGcsClient. If the field is intentionally handled at a lower layer (e.g. read directly from env or passed differently), a brief README note to that effect would clarify; if it's unimplemented, the table row should be removed or marked as coming in a future release.Copyright header inconsistency in interfaces.ts files
_lib/interfaces.ts(both GCS and S3) still carriesCopyright (C) 2026 System Initiative, Inc.while the other changed files were updated toElder Swamp Club, Inc.per the commit message. Not a functional issue, but the commit message says headers were updated so the omission looks unintentional.Adversarial Review
Medium
TOCTOU race in
registerNamespace— unconditional overwrite on re-registrationdatastore/gcs/extensions/datastores/gcs.ts:189,datastore/s3/extensions/datastores/s3.ts:183repoId), the code reads the existing manifest (verifyingrepoIdmatches), then performs an unconditionalputObjectto update the timestamp. Between the read and the write, a third party could delete the manifest and a different process could successfully create it with a differentrepoIdviaputObjectConditional. The unconditionalputObjectthen silently overwrites the new registration.repoId: "aaa") re-registers namespace"infra". After A'sgetObjectconfirms repoId matches, an operator deletesinfra/.namespace.json(cleanup, lifecycle rule, etc.). Process B (repoId: "bbb") callsregisterNamespace— itsputObjectConditionalsucceeds (object was deleted). Then A's unconditionalputObjectfires and overwrites B's legitimate claim with A's manifest. No error is raised.putObjectConditionalGeneration/ S3 etag-based conditional) for the update so it fails if the object changed between read and write.listNamespacesperforms a full-prefix scan of all objectsdatastore/gcs/extensions/datastores/gcs.ts:192-206,datastore/s3/extensions/datastores/s3.ts:186-200listAllObjects()with no subPrefix lists every object under the configured bucket prefix, then filters client-side for keys ending in/.namespace.json. For a datastore with thousands of data files, metadata files, and index entries, this downloads a full object listing just to find a handful of namespace manifests.ListObjectsV2page returns 1000 keys, so this issues 100 LIST API calls just to find 3 namespace manifests. At scale, this is slow and expensive (S3 charges per LIST request)."/") to enumerate top-level prefixes, then issue targetedheadObjectorgetObjectcalls for{prefix}/.namespace.json. This reduces API calls from O(total objects / 1000) to O(namespaces).Low
No input validation on
namespaceparameter inregisterNamespacedatastore/gcs/extensions/datastores/gcs.ts:159,datastore/s3/extensions/datastores/s3.ts:153namespacestring is interpolated directly into the object key as${namespace}/.namespace.json. An empty string produces the key/.namespace.json(leading slash). A namespace containing/(e.g.,"a/b") createsa/b/.namespace.json, which nests deeper than expected and won't be discovered bylistNamespacesif it only expects single-segment prefixes.listNamespacesimplementation does handle multi-segment namespace slugs correctly (slices everything before/.namespace.json), so this is cosmetic rather than a correctness bug.namespaceis non-empty and contains no/characters before constructing the key, or document that multi-segment namespaces are intentionally supported.registerNamespacewill throw an opaque error if the object is deleted between conditional-write failure and subsequent readdatastore/gcs/extensions/datastores/gcs.ts:175-178,datastore/s3/extensions/datastores/s3.ts:169-172putObjectConditionalindicates the object already exists (returnsnull/false), but the object is deleted beforegetObjectruns, the caller gets a raw "Not Found" / "NoSuchKey" error with no indication that this was a namespace registration conflict check. The retry-safe behavior is fine, but the error message is confusing.Verdict
PASS — The core logic is correct: atomic first-writer-wins via conditional puts, proper idempotent re-registration, and correct handling of the different return types between S3 (
boolean) and GCS (GcsWriteResult | null). The TOCTOU in finding #1 requires external deletion during a narrow window and is a theoretical concern rather than a practical production risk. The full-scan inlistNamespacesis a scalability concern worth addressing before the namespace count or bucket size grows, but it's functionally correct. Tests are thorough and follow project conventions (local mock servers,sanitizeResources: falsewith explanatory comments,finallycleanup, conformance helpers).