Skip to content

feat: upload and install custom .mcpb bundles from web UI#170

Open
Ovaculos wants to merge 20 commits intoNimbleBrainInc:mainfrom
Ovaculos:feat/mcpb-upload
Open

feat: upload and install custom .mcpb bundles from web UI#170
Ovaculos wants to merge 20 commits intoNimbleBrainInc:mainfrom
Ovaculos:feat/mcpb-upload

Conversation

@Ovaculos
Copy link
Copy Markdown
Contributor

@Ovaculos Ovaculos commented May 4, 2026

Summary

  • Adds POST /v1/bundles/upload REST endpoint for multipart .mcpb upload with SDK validation
  • Extends manage_app tool to accept path for file-based bundle install (unified install function for both registry and uploaded bundles)
  • Routes .mcpb archives through SDK's prepareServer({ local }) in startBundleSource
  • Adds Upload button to About tab's "Installed Bundles" section

Dependencies

Requires mpak#94 — adds validateMcpb() to @nimblebrain/mpak-sdk.

Using the updated SDK locally

Until mpak#94 is published to npm, symlink the local SDK build:

rm -rf node_modules/@nimblebrain/mpak-sdk
ln -s ../../mpak/packages/sdk-typescript node_modules/@nimblebrain/mpak-sdk

After mpak#94 is released and @nimblebrain/mpak-sdk is bumped in package.json, remove the symlink and run bun install.

QA review changes (post 47496b1)

Ten follow-up commits address the QA review findings:

Critical security

  • fix(bundles): confine manage_app.path to workspace bundles dir (bf65f8d) — assertPathInWorkspaceBundlesDir rejects any LLM-supplied path outside <workDir>/workspaces/<wsId>/bundles/. Wired into install, uninstall, and startup re-hydration. Realpaths both sides so symlinks pointing outside the dir are rejected.
  • fix(api): validate uploaded .mcpb in tempfile before commit to bundles dir (96726b2) — Validate-then-commit flow. Upload writes to os.tmpdir(), runs validateMcpb, then renameSync into the bundles dir on success. Failure or thrown error unlinks the temp. Bundles dir now holds validated content only, regardless of crash timing.

Other fixes

  • fix(api): randomize uploaded .mcpb filename to prevent silent overwrite (bfb85dc) — 64-bit random hex suffix before .mcpb. Two uploads of bundle.mcpb no longer clobber each other. Frontend display name comes from manifest, unaffected.
  • fix(api): enforce MAX_BUNDLE_SIZE post-buffer in handleBundleUpload (e9117f9) — Authoritative data.length check after buffering. Route-level bodyLimit is advisory (Content-Length only); chunked transfer encoding bypasses it. Constant exported from handlers.ts, shared with route middleware.

Refactors

  • refactor(api): hoist node:fs and mpak-sdk imports in handleBundleUpload (a84adc0) — Drops dynamic await import calls; handlers.ts loads on every API boot so deferring offered no benefit.
  • refactor(api): extract UploadedFileEntry helper, adopt in upload handlers (45df46d) — asUploadedFile helper in src/api/types.ts replaces inline as unknown as { arrayBuffer(): Promise<ArrayBuffer>; ... } casts in three handlers (handleChatMultipart, handleResourceUpload, handleBundleUpload).

Web UI

  • feat(web): track upload phase explicitly in AboutTab (3ffe22e) — uploadPhase: "idle" | "validating" | "installing" + uploadError: string | null. isUploading derived from phase. Replaces brittle string-equality checks against literal copy. finally resets phase on thrown errors.

Tests

  • test: import production filter in mcpb-upload unit test (89373d9) — Extracts bundleEntryMatchesTarget to a named export in system-tools.ts, used by both install dedup and uninstall filter. Test imports the production function rather than re-implementing it.
  • test: cover .mcpb path guard and upload integration (3bb76b4) — New test/unit/bundle-path-guard.test.ts (6 tests) and test/integration/mcpb-bundle-upload.test.ts (5 tests) covering valid upload, filename uniqueness, traversal stripping, non-.mcpb rejection, and invalid-archive rejection. Integration tests will fail in CI until @nimblebrain/mpak-sdk@>=0.7.0 is published and the dep is bumped past 0.6.0 — intentional, gates the SDK bump on the contract.

Docs

  • docs: changelog entry for .mcpb upload feature (87e0307)

Test plan

  • Upload valid .mcpb → validates, installs, appears in bundle list
  • Upload invalid file → error displayed inline
  • Install via manage_app with path → same behavior as name installs
  • .mcpb bundles survive server restart (persisted in workspace.json)
  • Typecheck passes (only pre-existing TS2688 ambient errors)
  • bun run test:unit (2356 pass)
  • bun run test:web (234 pass)
  • bun run test:integration locally (460 pass, 12 skip — mcpb-bundle-upload.test.ts passes via mpak-sdk symlink; will fail in CI until 0.7.0 published)
  • Path traversal in upload filename → stripped to basename inside bundles dir
  • LLM-supplied manage_app({ path }) outside bundles dir → rejected
  • Two uploads of same source filename → distinct on-disk paths

Closes #169

🤖 Generated with Claude Code

@mgoldsborough
Copy link
Copy Markdown
Contributor

Lint is broken

Copy link
Copy Markdown
Contributor

@mgoldsborough mgoldsborough left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Critical issues inline. Path traversal on upload and the broken uninstall-by-path are blockers; the .mcpb spawn branch also drifts from the named/local branches in ways that will bite real bundles. SDK dep also isn't released yet.

Comment thread src/api/handlers.ts Outdated
const bundlesDir = joinPath(runtime.getWorkspaceScopedDir(workspaceId), "bundles");
mkdirSync(bundlesDir, { recursive: true });

const safeName = sanitizeFilename(filename);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Path traversal: sanitizeFilename only strips control chars and quotes — it doesn't touch /, \, or ... A user-supplied filename like ../../etc/cron.daily/foo.mcpb resolves outside bundlesDir and writeFileSync will happily write there. Use path.basename(filename) after the extension check, or reject any filename containing path separators.

Comment thread src/api/handlers.ts Outdated
writeFileSync(bundlePath, data, { mode: 0o600 });

// Validate via mpak SDK
const { validateMcpb } = await import("@nimblebrain/mpak-sdk");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nimblebrain/mpak-sdk@0.6.0 is the version pinned in package.json and it doesn't export validateMcpb — mpak#94 is still open. This will throw at runtime until that PR ships and the SDK pin is bumped here. Worth blocking the merge on mpak#94 + an SDK release rather than relying on the symlink workflow.

Comment thread src/tools/system-tools.ts Outdated
}
return await uninstallBundleFromWorkspaceViaCtx(
name,
target,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uninstall by path doesn't actually work. target here is the absolute file path, which gets passed as name into uninstallBundleFromWorkspaceViaCtx. That helper does deriveServerName(target)foo-mcpb (path basename), but the bundle was registered under the manifest-derived server name, so lifecycle.getInstance returns nothing and uninstallBundleFromWorkspace throws No bundle "<basename>" found.

The workspace.json filter further down (ws.bundles.filter((b) => !("name" in b && b.name === name))) also only matches the name variant, so even if the process were stopped the path entry would stay in workspace.json forever.

Needs the same { name?, path? } threading you did for install.

Comment thread src/bundles/startup.ts Outdated
const mpak = getMpak(mpakHome);
const nbWorkDir = opts?.workDir ?? process.env.NB_WORK_DIR ?? join(homedir(), ".nimblebrain");

const server = await mpak.prepareServer(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unlike the named-bundle branch above, this skips resolveUserConfig(...) before calling prepareServer. Any uploaded bundle that declares user_config (API keys, etc.) will fail with a raw MpakConfigError here instead of getting the friendly nb config set -w <wsId> hint, and host env aliases won't be wired in.

Comment thread src/bundles/startup.ts Outdated
...server.env,
...filterEnvForBundle(process.env as Record<string, string>, undefined, ref.allowedEnv),
...(ref.env ?? {}),
...platformEnv,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This env block drifts from both the named-bundle branch (lines 305-316) and buildLocalSource:

  • No MPAK_WORKSPACE / UPJACK_ROOT — Upjack-based .mcpb bundles will run against the wrong directory.
  • No internalEnv injection (NB_INTERNAL_TOKEN / NB_HOST_URL) for ref.protected bundles.
  • prepareServer's workspaceDir is opts?.dataDir ?? nbWorkDir — for cold spawns this can be the global workdir instead of a per-bundle dir.

Probably want to share the env-building with the named branch rather than re-rolling it here.

Comment thread src/bundles/startup.ts Outdated
{ workspaceDir: opts?.dataDir ?? nbWorkDir },
);

const serverName = deriveServerName(server.name);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Server-name collision: this registers under deriveServerName(manifest.name), but serverNameFromRef in workspace-ops.ts derives from the path basename for {path} refs. So:

  • The registry.hasSource(serverName) duplicate check upstream uses the wrong name and won't catch real collisions (re-uploading the same bundle from a different path).
  • resolveBundleDataDir(wsPath, ref.path) produces an ugly, non-portable data dir; on restart, buildProcessInventory rebuilds inventory keyed by the path-derived name that doesn't match the registered one.

Either make serverNameFromRef / bundleNameFromRef .mcpb-aware (peek the manifest), or carry the manifest name through installBundleInWorkspace instead of re-deriving in two places.

Ovaculos and others added 9 commits May 5, 2026 22:38
Adds end-to-end flow for uploading .mcpb bundles not on the mpak
registry. REST endpoint validates via SDK's validateMcpb(), manage_app
tool handles installation. Uploaded bundles are treated identically to
registry bundles — no separate "sideloaded" distinction.

- POST /v1/bundles/upload: multipart upload + validation
- manage_app tool: accepts optional path for file-based install
- startBundleSource: routes .mcpb archives through SDK prepareServer
- About tab: Upload button on Installed Bundles section

Closes NimbleBrainInc#169

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Drives the next four commits via TDD red→green. Each test asserts
post-fix behavior and currently fails against the buggy code:

- Fix 1 (path traversal in handleBundleUpload)
- Fix 3a (uninstall server name resolution by .mcpb path)
- Fix 3b (workspace.json filter handles {path} entries)
- Fix 4 (.mcpb branch resolves workspace credentials, requires wsId)
- Fix 5 (.mcpb spawn env includes MPAK_WORKSPACE / UPJACK_ROOT / internalEnv)
- Fix 6 (install returns manifest-derived sourceName + dataDir)

13 bug-tests fail; 1 control test (named-bundle removal) passes —
confirming the filter test isolates the .mcpb regression.

Integration tests (Fix 4, 5, 6) use bun:test mock.module to stub
McpSource, getMpak, workspace-credentials so startBundleSource and
installBundleInWorkspace can be exercised in isolation.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
handleBundleUpload joined the user-supplied filename onto the workspace
bundles dir after only stripping control chars / quotes via
sanitizeFilename. A filename like "../../etc/cron.daily/evil.mcpb" passed
through unchanged and the writeFileSync target escaped bundlesDir,
landing wherever the user could traverse to.

Fix: introduce safeBundleFilename which applies path.basename, stripping
all directory components. handleBundleUpload now uses this helper for
the on-disk path. Helper is exported so the test pins the contract.

sanitizeFilename is unchanged — it remains the right tool for the
Content-Disposition path (file download), where we want to preserve
display names but neutralize header-injection chars.

Closes PR NimbleBrainInc#170 review comment 1.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The .mcpb branch in startBundleSource diverged from the named-bundle
branch in three ways that broke uploaded bundles silently:

1. Skipped resolveUserConfig — workspace-stored credentials (API keys
   declared in manifest.user_config) never reached the subprocess. The
   SDK's MpakConfigError surfaced to the user even when they had run
   `nb config set -w <wsId>` correctly.

2. Derived serverName from server.name (a path-influenced string from
   the SDK) instead of peeking the manifest. dataDir defaulted to
   nbWorkDir root, so Upjack apps wrote outside their per-bundle dir.

3. Omitted MPAK_WORKSPACE / UPJACK_ROOT / internalEnv from the spawn
   env. Upjack data writes landed in the wrong dir; protected bundles
   couldn't reach the host.

Fix: rewrite the branch to mirror the named-bundle path one-for-one,
peeking the manifest from the .mcpb archive via validateMcpb instead
of reading from the mpak cache. Now requires opts.wsId (matches named
branch) so cross-tenant credential pooling is impossible.

Note: validateMcpb requires mpak-sdk@>=0.7.0 (mpak#94). PR NimbleBrainInc#170 stays
blocked on that release; once the dep is bumped, this branch works
end-to-end. Tests mock @nimblebrain/mpak-sdk so they pass against the
currently-installed SDK.

Closes PR NimbleBrainInc#170 review comments 4 and 5.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
installBundleInWorkspace was pre-computing serverName + dataDir from
the BundleRef before calling startBundleSource. For .mcpb refs that's
wrong: the path-derived name ("echo-mcpb" for "/uploads/echo.mcpb")
doesn't match the manifest-derived name the bundle actually registers
under ("echo"), so:

- The duplicate-source pre-flight check looked up the wrong name. When
  a re-upload landed, the check passed silently and addSource later
  threw the cryptic "already registered" error from the registry.
- dataDir embedded path components ("data/-uploads/echo.mcpb"), so
  re-uploads from a different upload path or a different workspace
  layout landed in different dirs across restarts.

Fix: introduce isMcpbRef helper. For .mcpb refs, skip the pre-flight
name + dataDir derivation and let startBundleSource own registration
(the registry's own duplicate check is the safety net). After start,
derive dataDir from result.sourceName (the manifest-derived name) so
the per-bundle dir is stable across re-uploads.

The test that originally asserted `deriveServerName(path) ===
deriveServerName(name)` was rewritten — that's an impossible contract,
since the path always carries the .mcpb suffix. The new test exercises
the real install path and asserts the registered name matches the
manifest, not the path.

Closes PR NimbleBrainInc#170 review comment 6.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The earlier draft of this file used bun:test mock.module to stub
@nimblebrain/mpak-sdk, ../../src/config/workspace-credentials.ts,
../../src/tools/mcp-source.ts, and ../../src/bundles/mpak.ts so the
.mcpb branch of startBundleSource and the .mcpb-aware code in
installBundleInWorkspace could be exercised in isolation.

mock.module in bun:test is global within the test process. The stubs
leaked into every other test file in the same run (system-tools,
lifecycle, etc.), turning a clean main into 175+ failures. Confirmed
by isolating the file (clean) vs combining with system-tools.test.ts
(47 failures appeared).

Removes Fix 4, 5, 6 integration tests for now. Fix 1 (path traversal)
and Fix 3b (uninstall workspace.json filter) stay — pure logic, no
module mocks needed.

Once mpak-sdk@>=0.7.0 ships validateMcpb, integration coverage for
the deferred fixes can land in test/integration/ where each file runs
in its own process and real fixtures can drive end-to-end paths.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
uninstallBundleFromWorkspaceViaCtx took a single `target: string`
parameter and called deriveServerName(target). For .mcpb path
uninstalls (target = "/uploads/echo.mcpb"), this produced
"echo-mcpb" — but the source was registered under deriveServerName
(manifest.name) = "echo". Three downstream failures:

1. lifecycle.getInstance(serverName, wsId) returned nothing because
   it was looking up the wrong key. The protected-bundle check
   silently passed and uninstall proceeded for protected bundles.
2. uninstallBundleFromWorkspace(wsId, target, registry) hit the
   registry under the wrong name → "no bundle found" with no hint
   that the path argument was the cause.
3. The workspace.json filter only matched {name} entries:
     ws.bundles.filter((b) => !("name" in b && b.name === name))
   Path-installed bundles became permanent residents of
   workspace.json even after their tool source was deregistered.

Fix: change the function signature to accept { name?, path? },
mirroring the install handler. For .mcpb path uninstalls, peek the
manifest via validateMcpb(path) before calling deriveServerName so
every downstream lookup uses the manifest-derived name. Update the
workspace.json filter to dispatch per-variant — match {name} when
target.name is set, {path} when target.path is set.

manage_app handler now passes { name, path } through unchanged
(matches the install side).

Closes PR NimbleBrainInc#170 review comment 3.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@Ovaculos Ovaculos force-pushed the feat/mcpb-upload branch from 06232f1 to 039e8e0 Compare May 6, 2026 03:55
Annotating the variable as `FormData` clashed in CI: Bun's
Request.formData() returns the undici `FormData`, but the explicit
`FormData` annotation resolved to the DOM-lib type from a different
ambient declaration. Their iterator types differ (FormDataEntryValue vs
string), so the assignment failed strict typecheck.

Inferring via `Awaited<ReturnType<typeof raw.formData>>` keeps the
variable strongly typed without pinning it to the wrong global.

Surfaced by PR NimbleBrainInc#170 CI run 25415571865; the validateMcpb TS errors in
the same run are intentionally left in place — they document that the
PR is blocked on @nimblebrain/mpak-sdk@>=0.7.0.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@Ovaculos
Copy link
Copy Markdown
Contributor Author

Ovaculos commented May 6, 2026

⛔ Blocked: needs mpak to have validateMcpb() (NimbleBrainInc/mpak#94)

This PR introduces three runtime call sites for validateMcpb that don't exist in the currently-pinned @nimblebrain/mpak-sdk@0.6.0:

  • src/api/handlers.tshandleBundleUpload (validates the uploaded archive before save)
  • src/bundles/startup.ts.mcpb branch of startBundleSource (peeks manifest for credential resolution + canonical naming)
  • src/tools/system-tools.tsuninstallBundleFromWorkspaceViaCtx (peeks manifest to derive the registered server name)

CI surfaces the gap as three TS2339: Property 'validateMcpb' does not exist errors. Those errors are intentionally kept in place — they're the visible signal that the SDK bump is the prerequisite.

Unblocks when:

  1. mpak#94 (the PR exporting validateMcpb) is merged
  2. mpak cuts a release that includes it (target: 0.7.0)
  3. package.json here bumps @nimblebrain/mpak-sdk to that version
  4. bun install repopulates node_modules

Verification after bump:

  • bun run check clean (no TS2339 for validateMcpb)
  • bun run test:unit — 2349/2349 pass
  • Manual .mcpb upload in the web UI completes end-to-end (a malformed bundle should hit the friendly validateMcpb rejection, not the previous is not a function crash)

Could a maintainer add a blocked (or equivalent) label? Fork contributors don't have label permissions.

Once unblocked, the deferred test work in this branch can land: test/unit/mcpb-upload.test.ts currently covers Fix 1 (path traversal) and Fix 3b (workspace.json filter). Integration coverage for Fix 4/5/6 (the validateMcpb-dependent paths) was pulled because bun:test's mock.module is process-global and was leaking SDK stubs into other test files. That coverage can return in test/integration/ once real fixtures replace the mocks.

Ovaculos and others added 10 commits May 6, 2026 11:09
Dynamic imports inside the handler offered no benefit — handlers.ts loads
on every API boot, so deferring only shifts the cost to first-call latency
without avoiding it. Hoists `mkdirSync`, `writeFileSync`, `unlinkSync` from
`node:fs` and `validateMcpb` from `@nimblebrain/mpak-sdk` to module scope.
`join` already top-level imported; drops the local `joinPath` alias.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
LLM-supplied `path` to `manage_app` was unrestricted — a successful prompt
injection (chat content, RAG doc, scraped page) could pass any local
`.mcpb` archive to install/uninstall. The install path then spawned a
subprocess under the platform user with workspace credentials,
`MPAK_WORKSPACE`, `UPJACK_ROOT`, and (for protected refs) `NB_INTERNAL_TOKEN`
attached, plus persisted the path to `workspace.json` so it re-spawned every
restart.

Adds `assertPathInWorkspaceBundlesDir` to `src/bundles/paths.ts`. Realpaths
both sides where possible so symlinks pointing outside the dir are rejected.
Falls back to a lexical resolve when the file does not exist (uninstall
after manual deletion is allowed; the persisted path is already trusted).

Wires the check into three call sites:

- `installBundleInWorkspaceViaCtx` — primary control. Rejects bad path
  before constructing the bundle ref so `startBundleSource` never sees it.
- `uninstallBundleFromWorkspaceViaCtx` — mirror guard. Without it, an
  attacker could pass an arbitrary path to read its manifest via
  `validateMcpb` (which extracts and parses the archive).
- `startBundleSource` `.mcpb` branch — defense-in-depth at re-hydration.
  Catches a tampered or out-of-band-edited `workspace.json`.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…s dir

`handleBundleUpload` previously wrote the upload directly into the
workspace bundles dir and only unlinked it on validation failure (best
effort). If unlink failed (perm/race) or the process crashed between write
and unlink, an unvalidated `.mcpb` lingered in the bundles dir — a stale
artifact the install path could later spawn.

Switches to a validate-then-commit flow:

1. Write upload to a random tempfile under `os.tmpdir()`.
2. Run `validateMcpb` against the temp path. On failure or thrown error,
   unlink temp and return.
3. On success, `renameSync` the temp into the bundles dir.

Keeps the bundles dir to validated content only, regardless of crash
timing. Rename is atomic on a shared filesystem; cross-fs falls back to
copy + unlink — acceptable since the archive is already trusted.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two uploads named `bundle.mcpb` previously clobbered each other on disk:
`safeBundleFilename` returned `basename(filename)` only, so the second
upload silently swapped the artifact backing whatever install was pinned
to that path in `workspace.json`.

Appends a 64-bit random hex suffix before the `.mcpb` extension so every
upload lands at a unique path. SHA-256 over the buffer was considered but
adds a 200 MB hash compute for no real benefit — idempotent re-upload is
not a requirement, and content-addressed storage would make collision
analysis a permanent maintenance burden.

The frontend never sees this name. Bundle display name comes from the
manifest via `extractBundleMeta` → `seedInstance` → `instance.bundleName`,
which `seedInstance` prefers over the filesystem label.

Updates `test/unit/mcpb-upload.test.ts`:
- Asserts the new `<stem>-<hex16>.mcpb` shape on existing traversal tests.
- Adds a uniqueness test pinning the no-clobber contract.
- Drops the `as unknown as { safeBundleFilename? }` cast — the function
  is a plain named export and the test file already imports from the
  same module.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The route-level `bodyLimit(..., { multipart: MAX_BUNDLE_SIZE })` is
advisory — it inspects `Content-Length` only and lets chunked transfer
encoding, missing headers, or a lying client through. The handler then
buffered the upload into memory regardless of size.

Adds a post-buffer authoritative check on `data.length` immediately after
`arrayBuffer()` resolves, returning 413 with `{limit, received}`. Promotes
`MAX_BUNDLE_SIZE` to an exported constant in `handlers.ts` so the route
middleware and the handler share a single source of truth.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…lers

Three multipart handlers (`handleChatMultipart`, `handleResourceUpload`,
`handleBundleUpload`) each carried an inline
`value as unknown as { arrayBuffer(): Promise<ArrayBuffer>; ... }` cast
plus a duck-typed `arrayBuffer` check. The cast exists because Bun's
`Request.formData()` returns the undici `FormData` whose entries don't
unify with the DOM-lib `File` type at annotation sites.

Centralises both pieces in `src/api/types.ts`:

- `UploadedFileEntry` interface — minimal `arrayBuffer` / `name` / `type` /
  `size` shape that all callers share.
- `asUploadedFile(value)` helper — narrows a FormData value to the
  interface or returns `null`. Single source for the cast.

Three handlers now collapse to `const entry = asUploadedFile(value); if
(!entry) ...`. Adds ~30 LOC of shared types, drops ~20 LOC of inline
casts and duck-typing per site.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Previous shape used a single `uploadStatus: string | null` that doubled as
both phase indicator and error message. Disable state and button label
were derived via string-equality checks against the literal copy
("Validating..." / "Installing..."), which would silently break if the
strings were ever changed.

Splits state in two:

- `uploadPhase: "idle" | "validating" | "installing"` — drives the button
  label and disabled state. `isUploading` derived from `phase !== "idle"`.
- `uploadError: string | null` — error surfaced in the inline panel.
  Independent so a phase transition doesn't have to clear an error string.

Wraps the phase reset in `finally` so a thrown error from `uploadBundle`
or `callTool` always returns the button to the idle state.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The Fix 3b unit test previously re-implemented the workspace.json filter
locally as `fixedFilter` — drift between the test copy and the production
filter would not have been caught.

Extracts the dispatch into a named export `bundleEntryMatchesTarget` in
`src/tools/system-tools.ts` and adopts it at both call sites:

- Install-side `bundles.some(...)` duplicate guard.
- Uninstall-side `bundles.filter(...)` removal pass.

Test imports it directly. Local helper is now a thin wrapper around
`!bundleEntryMatchesTarget(...)` — kept only so each test reads as
"remove the matching entry" rather than threading the negation through
every assertion.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two test files address the coverage gap flagged in the PR review.

`test/unit/bundle-path-guard.test.ts` — pure-logic tests for
`assertPathInWorkspaceBundlesDir` (Critical NimbleBrainInc#1 guard). Covers:

- Files inside the bundles dir pass.
- Non-existent files inside the dir pass (uninstall after manual delete).
- Absolute paths outside the dir reject.
- Traversal paths that resolve outside the dir reject.
- Cross-tenant: another workspace's bundles dir rejects.
- Symlinks inside bundlesDir that point outside reject (realpath defense).

The macOS-specific `/var` → `/private/var` symlink quirk surfaced a real
divergence between `realpathSync(existingDir)` and lexical `resolve(missing)`
on the same logical path. Adds a `canonicalize()` helper in `paths.ts` that
walks up to the deepest existing ancestor and realpaths from there, so the
guard agrees with itself regardless of which side is missing.

`test/integration/mcpb-bundle-upload.test.ts` — end-to-end coverage of the
upload flow (Fix 4 / Fix 5 / Fix 6 surface). Builds a real `.mcpb` archive
via the system `zip` CLI (manifest.json + a stub `server.sh` so validation
passes) and drives `POST /v1/bundles/upload`:

- Valid upload → 200, manifest echoed back, file inside bundles dir.
- Two uploads of the same source name → distinct on-disk paths
  (no-clobber contract from the random-suffix change).
- `../../etc/cron.daily/evil.mcpb` filename → traversal stripped.
- Non-`.mcpb` extension → 400, no disk side effects.
- Garbage archive → 400 + bundles dir empty (validate-then-rename).

Will fail until `@nimblebrain/mpak-sdk@>=0.7.0` is published with
`validateMcpb` exported and the dependency is bumped past 0.6.0.
Intentional — the failing test gates the SDK bump on the contract being
preserved.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds an `### Added` bullet under Unreleased covering the upload endpoint,
the `manage_app({ path })` install path, the workspace-bundles-dir
confinement guard, and persistence behavior.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: upload and install custom .mcpb bundles from the web UI

2 participants