Rename SDK to sdk-typescript, add e2e download test#7
Conversation
Rename the directory to match sdk-python naming convention. The npm package name (@nimblebrain/mpak-sdk) is unchanged. Add an integration test that downloads a real bundle from the CDN, verifies SHA256 integrity, extracts the zip, and validates the manifest.
JoeCardoso13
left a comment
JoeCardoso13
left a comment
There was a problem hiding this comment.
Findings
The rename is thorough across all reference points (docs, lock file, package.json, pnpm workspace), no problems there. The new e2e test has a few minor issues. The most important of which is a test hanging with a cryptic timeout message, meaning they're all small.
MINOR-1 — 6 unused imports added to the integration test
File: packages/sdk-typescript/tests/client.integration.test.ts
These 6 imports are new but none are used in the new test:
import { mkdtemp, rm, readFile, readdir } from "fs/promises"; // unused
import { tmpdir } from "os"; // unused
import { join } from "path"; // unusedOnly createHash (from crypto) and JSZip are actually used. These look like leftovers from an initial disk-based implementation draft that was replaced with the current in-memory JSZip approach. The PR claims pnpm lint — 5/5 packages pass, so the linter apparently isn't catching unused imports in test files — worth checking the ESLint config.
MINOR-2 — No explicit timeout on fetch(download.url)
File: packages/sdk-typescript/tests/client.integration.test.ts
const response = await fetch(download.url);If the CDN stalls (not just a connection timeout but a hung response), this awaits indefinitely until vitest's global test timeout fires — producing a generic "test timed out" error rather than a clear "download failed" message. A simple AbortSignal.timeout() would make failures more diagnostic:
const response = await fetch(download.url, { signal: AbortSignal.timeout(30_000) });MINOR-3 (cross-PR) — PRODUCT.md not updated
File: PRODUCT.md (introduced by PR #6, currently open)
PRODUCT.md shows the monorepo layout with sdk/ and python-sdk/, neither of which match the actual directory names after both PRs land. One of the two PRs should own fixing this before merging.
- Remove unused imports from integration test (MINOR-1) - Add AbortSignal.timeout to CDN fetch in e2e test (MINOR-2) - Extend lint scope to include tests/ - Add 404 handling to searchBundles/searchSkills (fixes #8) - Add sdk-typescript-ci.yml (lint, format, typecheck, test) - Add sdk-typescript-publish.yml (tag-based OIDC publish to npm) - Update README with badges and release docs
|
closes #8 |
JoeCardoso13
left a comment
JoeCardoso13
left a comment
There was a problem hiding this comment.
Re-review
All previous findings addressed. Issue #8 bugs fixed with proper 404 handling in searchBundles and searchSkills. LGTM.
Status
| Item | Status |
|---|---|
| MINOR-1 (unused imports) | Fixed |
| MINOR-2 (fetch timeout) | Fixed (AbortSignal.timeout(30_000)) |
| MINOR-3 (PRODUCT.md) | Fixed in PR #6 |
Issue #8 Bug 1 (searchBundles 404 → wrong error class) |
Fixed (explicit 404 check before generic !response.ok) |
Issue #8 Bug 2 (getBundle 404 for echo bundle) |
Fixed (registry-side + integration test passing) |
| Lint scope extended to tests/ | Good |
| CI + publish workflows | Good |
2 participants
Summary
packages/sdktopackages/sdk-typescriptto matchsdk-pythonnaming convention (npm package name@nimblebrain/mpak-sdkis unchanged).mcpbbundle from CDN, verifies SHA256 integrity, extracts the zip, and validates the manifestTest plan
pnpm --filter @nimblebrain/mpak-sdk test— 44 unit tests passpnpm --filter @nimblebrain/mpak-sdk test:integration— 14 integration tests pass (including new e2e download test)pnpm typecheck— 7/7 packages passpnpm lint— 5/5 packages passcc @shwetank-dev @JoeCardoso13