fix: review hardening — provider correctness, write-path tests, release safety

.github/workflows/npm-publish-github-packages.yml

@@ -1,11 +1,15 @@
-# This workflow will run tests using node and then publish a package to GitHub Packages when a release is created
-# For more information see: https://help.github.com/actions/language-and-framework-guides/publishing-nodejs-packages
+# Publishes to GitHub Packages when a release is published.
+# See .claude/skills/release/SKILL.md for the release procedure.
 
 name: Node.js Package
 
 on:
   release:
-    types: [created]
+    # published, not created: GitHub fires `created` when a *draft* is saved, which
+    # would publish from the default branch before the tag exists — and then fires
+    # `published` (not `created`) when that draft is released, so the real release
+    # would ship nothing.
+    types: [published]
 
 env:
   NODE_VERSION: 24
@@ -21,6 +25,8 @@ jobs:
           node-version: ${{ env.NODE_VERSION }}
           cache: pnpm
       - run: pnpm install --frozen-lockfile
+      - run: pnpm run lint
+      - run: pnpm run build
       - run: pnpm test
 
   publish-gpr:
@@ -38,9 +44,23 @@ jobs:
           cache: pnpm
           registry-url: https://npm.pkg.github.com/
       - run: pnpm install --frozen-lockfile
+
+      # The release tag and package.json must agree, or the published version silently
+      # differs from the tag people will go looking for. TAG comes through env rather
+      # than being interpolated into the script.
+      - name: Verify tag matches package.json version
+        env:
+          TAG: ${{ github.event.release.tag_name }}
+        run: |
+          VERSION="v$(node -p 'require("./package.json").version')"
+          if [ "$VERSION" != "$TAG" ]; then
+            echo "Release tag $TAG does not match package.json $VERSION" >&2
+            exit 1
+          fi
+
       # npm publish, not pnpm publish: pnpm adds git-state checks the release flow
       # does not need. prepublishOnly runs the build either way.
       # No --provenance: GitHub Packages does not accept provenance attestations.
       - run: npm publish
         env:
-          NODE_AUTH_TOKEN: ${{secrets.GITHUB_TOKEN}}
+          NODE_AUTH_TOKEN: ${{ secrets.GITHUB_TOKEN }}

.github/workflows/test.yml

@@ -23,3 +23,31 @@ jobs:
       - run: pnpm run lint
       - run: pnpm run build
       - run: pnpm test
+
+      # Started as a step, not a service: `services:` cannot pass a command, and the
+      # minio image needs `server /data`. Without this the integration spec skips, and
+      # skip-by-default plus never-in-CI means it rots — which matters because mocked
+      # tests cannot catch an SDK behaviour change.
+      - name: Start MinIO
+        run: |
+          docker run -d --name ci-minio -p 9000:9000 \
+            -e MINIO_ROOT_USER=minioadmin \
+            -e MINIO_ROOT_PASSWORD=minioadmin \
+            minio/minio:latest server /data
+          for i in $(seq 1 30); do
+            if curl -sf http://127.0.0.1:9000/minio/health/live; then
+              echo "minio ready"; exit 0
+            fi
+            sleep 2
+          done
+          echo "minio did not become healthy" >&2
+          docker logs ci-minio >&2
+          exit 1
+
+      - run: pnpm run test:integration
+        env:
+          MINIO_TEST_ENDPOINT: http://127.0.0.1:9000
+
+      - name: Stop MinIO
+        if: always()
+        run: docker rm -f ci-minio || true

package.json

@@ -63,5 +63,8 @@
     "typescript": "^6.0.3",
     "typescript-eslint": "^8.67.0"
   },
-  "packageManager": "pnpm@10.13.1"
+  "packageManager": "pnpm@10.13.1",
+  "publishConfig": {
+    "registry": "https://npm.pkg.github.com/"
+  }
 }

src/file-store.azure.spec.ts

@@ -10,6 +10,7 @@ describe("AzureFileStore", () => {
   let fastify: ReturnType<typeof Fastify>;
   let mockBlobClient: any;
   let mockContainerClient: any;
+  let mockGetContainerClient: jest.Mock;
   let store: FileStore;
 
   beforeEach(async () => {
@@ -30,8 +31,9 @@ describe("AzureFileStore", () => {
       getBlockBlobClient: jest.fn(() => mockBlobClient),
     };
 
+    mockGetContainerClient = jest.fn(() => mockContainerClient);
     (BlobServiceClient as unknown as jest.Mock).mockImplementation(() => ({
-      getContainerClient: jest.fn(() => mockContainerClient),
+      getContainerClient: mockGetContainerClient,
     }));
 
     process.env.AZURE_STORAGE_ACCOUNT_URL =
@@ -58,6 +60,12 @@ describe("AzureFileStore", () => {
       expect(store).toBeDefined();
     });
 
+    // Nothing asserted the container name, so every operation could target the wrong
+    // container and the suite would stay green.
+    it("opens the container named by AZURE_STORAGE_CONTAINER", () => {
+      expect(mockGetContainerClient).toHaveBeenCalledWith("test-container");
+    });
+
     it("should throw when AZURE_STORAGE_ACCOUNT_URL is missing", async () => {
       delete process.env.AZURE_STORAGE_ACCOUNT_URL;
       const f = Fastify();
@@ -102,6 +110,27 @@ describe("AzureFileStore", () => {
       );
     });
 
+    // Without this, changing the utf8 encoding to ascii mangles every non-ASCII byte
+    // and the suite stays green — all other payload fixtures are ASCII.
+    it("encodes a unicode payload as utf8", async () => {
+      mockBlobClient.uploadData.mockResolvedValueOnce({});
+
+      await store.save("u.txt", "text/plain", "héllo→世界");
+
+      expect(mockBlobClient.uploadData).toHaveBeenCalledWith(
+        Buffer.from("héllo→世界", "utf8"),
+        { blobHTTPHeaders: { blobContentType: "text/plain" } },
+      );
+    });
+
+    it("propagates a rejected upload, not just an errorCode", async () => {
+      mockBlobClient.uploadData.mockRejectedValueOnce(new Error("AuthFailure"));
+
+      await expect(store.save("a.txt", "text/plain", "x")).rejects.toThrow(
+        "AuthFailure",
+      );
+    });
+
     it("should pass a Buffer payload through untouched", async () => {
       mockBlobClient.uploadData.mockResolvedValueOnce({});
       const data = Buffer.from([1, 2, 3]);
@@ -140,6 +169,24 @@ describe("AzureFileStore", () => {
     });
   });
 
+  describe("write failures", () => {
+    it("propagates a rejected uploadFile", async () => {
+      mockBlobClient.uploadFile.mockRejectedValueOnce(new Error("io error"));
+
+      await expect(
+        store.copyFromLocalFile("a.txt", "text/plain", "/tmp/x"),
+      ).rejects.toThrow("io error");
+    });
+
+    it("propagates a rejected uploadStream", async () => {
+      mockBlobClient.uploadStream.mockRejectedValueOnce(new Error("aborted"));
+
+      await expect(
+        store.copyFromStream("a.txt", "text/plain", Readable.from(["x"])),
+      ).rejects.toThrow("aborted");
+    });
+  });
+
   describe("copyFromLocalFile", () => {
     it("should upload the local file", async () => {
       mockBlobClient.uploadFile.mockResolvedValueOnce({});

src/file-store.gcs.spec.ts

@@ -88,6 +88,24 @@ describe("GCPFileStore", () => {
         contentType: "text/plain",
       });
     });
+
+    it("propagates a failed save", async () => {
+      mockFile.save.mockRejectedValueOnce(new Error("quota exceeded"));
+
+      await expect(
+        store.save("test.txt", "text/plain", "content"),
+      ).rejects.toThrow("quota exceeded");
+    });
+
+    it("round-trips a unicode payload unchanged", async () => {
+      mockFile.save.mockResolvedValueOnce(undefined);
+
+      await store.save("u.txt", "text/plain", "héllo→世界");
+
+      expect(mockFile.save).toHaveBeenCalledWith("héllo→世界", {
+        contentType: "text/plain",
+      });
+    });
   });
 
   describe("getAsBuffer", () => {
@@ -113,15 +131,35 @@ describe("GCPFileStore", () => {
         destination: "dest.txt",
       });
     });
+
+    it("propagates a failed upload", async () => {
+      mockBucket.upload.mockRejectedValueOnce(new Error("upload failed"));
+
+      await expect(
+        store.copyFromLocalFile("dest.txt", "text/plain", "/tmp/src.txt"),
+      ).rejects.toThrow("upload failed");
+    });
   });
 
   describe("getAsStream", () => {
     it("should return the read stream", async () => {
       const rs = Readable.from(["data"]);
+      mockFile.exists.mockResolvedValueOnce([true]);
       mockFile.createReadStream.mockReturnValueOnce(rs);
 
       expect(await store.getAsStream("test.txt")).toBe(rs);
     });
+
+    // createReadStream does no I/O, so without an existence check a missing object
+    // resolves and fails later as a stream 'error'. Every other provider rejects.
+    it("rejects for a missing object instead of returning a doomed stream", async () => {
+      mockFile.exists.mockResolvedValueOnce([false]);
+
+      await expect(store.getAsStream("missing.txt")).rejects.toThrow(
+        "File not found: missing.txt",
+      );
+      expect(mockFile.createReadStream).not.toHaveBeenCalled();
+    });
   });
 
   describe("copyFromStream", () => {
@@ -147,6 +185,19 @@ describe("GCPFileStore", () => {
       });
       expect(Buffer.concat(chunks).toString()).toBe("piped content");
     });
+
+    it("propagates a write-stream failure", async () => {
+      const ws = new Writable({
+        write(_chunk, _enc, cb) {
+          cb(new Error("disk full"));
+        },
+      });
+      mockFile.createWriteStream.mockReturnValueOnce(ws);
+
+      await expect(
+        store.copyFromStream("test.txt", "text/plain", Readable.from(["x"])),
+      ).rejects.toThrow("disk full");
+    });
   });
 
   describe("getInfo", () => {

src/file-store.local.spec.ts

@@ -75,6 +75,19 @@ describe("LocalFileStore", () => {
         fastify.register(FileStorePlugin, { type: "nope" as any }),
       ).rejects.toThrow("Unknown storage type: nope");
     });
+
+    // A plain lookup finds Object.prototype.toString and registers with no FileStore
+    // decorated — a typo becoming a silent no-op boot instead of a failure.
+    it.each(["toString", "constructor", "valueOf", "hasOwnProperty"])(
+      "rejects the inherited Object.prototype name %s",
+      async (type) => {
+        const f = Fastify({ logger: false });
+        await expect(
+          f.register(FileStorePlugin, { type: type as never }),
+        ).rejects.toThrow(`Unknown storage type: ${type}`);
+        await f.close();
+      },
+    );
   });
 
   describe("exists", () => {
@@ -165,12 +178,11 @@ describe("LocalFileStore", () => {
       expect((await store.getAsBuffer("read.txt")).toString()).toBe("buffered");
     });
 
-    it("throws File not found when stat yields nothing", async () => {
+    it("rejects with ENOENT for a missing file", async () => {
       const store = await register();
-      jest.spyOn(fs.promises, "stat").mockResolvedValue(undefined);
-      await expect(store.getAsBuffer("ghost.txt")).rejects.toThrow(
-        `File not found: ${path.join(tempDir, "ghost.txt")}`,
-      );
+      await expect(store.getAsBuffer("ghost.txt")).rejects.toMatchObject({
+        code: "ENOENT",
+      });
     });
   });
 
@@ -182,12 +194,12 @@ describe("LocalFileStore", () => {
       expect((await streamToBuffer(rs)).toString()).toBe("streamed");
     });
 
-    it("throws File not found when stat yields nothing", async () => {
+    // Rejects rather than deferring to a stream 'error': createReadStream is lazy.
+    it("rejects with ENOENT for a missing file", async () => {
       const store = await register();
-      jest.spyOn(fs.promises, "stat").mockResolvedValue(undefined);
-      await expect(store.getAsStream("ghost.txt")).rejects.toThrow(
-        `File not found: ${path.join(tempDir, "ghost.txt")}`,
-      );
+      await expect(store.getAsStream("ghost.txt")).rejects.toMatchObject({
+        code: "ENOENT",
+      });
     });
   });
 
@@ -215,6 +227,19 @@ describe("LocalFileStore", () => {
         await fs.promises.readFile(path.join(tempDir, "x/y/z.txt"), "utf8"),
       ).toBe("piped");
     });
+
+    it("propagates a source stream error", async () => {
+      const store = await register();
+      const bad = new Readable({
+        read() {
+          this.destroy(new Error("source failed"));
+        },
+      });
+
+      await expect(
+        store.copyFromStream("bad.txt", "text/plain", bad),
+      ).rejects.toThrow("source failed");
+    });
   });
 });
 

src/file-store.minio.integration.spec.ts

@@ -64,6 +64,26 @@ describeIf("MinIO integration", () => {
     expect(await store.getInfo(`missing-${Date.now()}.txt`)).toBeNull();
   });
 
+  // The real GetObject 404 path. Mocks cannot catch a change in this error shape —
+  // the NoSuchKey/NotFound mismatch fixed in #9 was invisible to them.
+  it("rejects getAsBuffer for a missing key", async () => {
+    await expect(
+      store.getAsBuffer(`missing-${Date.now()}.txt`),
+    ).rejects.toBeDefined();
+  });
+
+  it("rejects getAsStream for a missing key", async () => {
+    await expect(
+      store.getAsStream(`missing-${Date.now()}.txt`),
+    ).rejects.toBeDefined();
+  });
+
+  it("round-trips a unicode payload and key", async () => {
+    const key = `unicode-héllo-世界-${Date.now()}.txt`;
+    await store.save(key, "text/plain", "héllo→世界");
+    expect((await store.getAsBuffer(key)).toString()).toBe("héllo→世界");
+  });
+
   it("round-trips a string payload", async () => {
     const key = `round-trip-${Date.now()}.txt`;
     await store.save(key, "text/plain", "hello minio");