test: add files plugin upload + error-handling coverage (50 tests) (#319)

* test: add files plugin upload + error-handling coverage (50 tests)

Adds a focused test file covering FilesPlugin paths that were lightly
exercised: the upload streaming path, error mapping in _handleApiError,
cache invalidation around mutations, raw/download response headers,
shutdown behavior, volume discovery merging, path validation, and the
clientConfig surface.

Originally proposed in PR #256 alongside the analytics ARROW_STREAM
work, but the file shipped with three classes of failures and was
removed from that PR to unblock its CI:

1. Missing per-volume policy on test configs. The default policy is
   publicRead(), which denies upload/mkdir/delete and surfaced as 403
   instead of the SDK-error status codes the tests asserted. Fixed by
   setting policy: policy.allowAll() on every test volume.
2. vi.spyOn(connector, "upload") inferred a stricter signature than
   the test's three-arg async mock. Switched the mock to spread args
   with a typed cast.
3. AuthenticationError test asserted "token" in the error message,
   but _extractUser actually surfaces the missing-x-forwarded-user
   path. Updated the assertion to match the real behavior.

Co-authored-by: Isaac
Signed-off-by: James Broadhead <jamesbroadhead@gmail.com>

* test: split upload-and-write.test.ts into focused per-feature files

The previous single file name implied upload + write coverage, but the
file actually held tests for ~10 separate concerns (download, raw, path
validation, shutdown, volume discovery, etc). Split into focused files:

- error-handling.test.ts   _handleApiError + _sendStatusError
- upload.test.ts            upload streaming + cache invalidation
- raw-endpoint.test.ts      /raw security headers
- download-endpoint.test.ts /download Content-Disposition
- delete.test.ts            delete + cache invalidation
- mkdir.test.ts             mkdir + cache invalidation
- shutdown.test.ts          shutdown + trackWrite
- volume-config.test.ts     discoverVolumes + clientConfig
- path-validation.test.ts   null-byte / 4096-cap / required-path

Shared pure helpers (mockReq/mockRes/getRouteHandler/etc) extracted to
_test-helpers.ts. The vi.hoisted/vi.mock block stays inlined per file
to match the convention in plugin.test.ts. No test logic changes:
50/50 still passing in the files plugin (1242 across appkit).

Co-authored-by: Isaac
Signed-off-by: James Broadhead <jamesbroadhead@gmail.com>

* test: address ACE multi-model review findings

Three test-quality fixes flagged by GPT 5.4 + Gemini 3.1 Pro review:

- shutdown.test.ts: assert the shutdown promise stays unresolved while
  inflightWrites > 0 — the previous test would silently pass even if
  shutdown() returned immediately (the final inflightWrites === 0 check
  is trivially true regardless of waiting).
- path-validation.test.ts: assert the SDK connector was not called when
  the handler rejects the input. Previously a regression that hit the
  SDK *and* returned 400 would still pass.
- _test-helpers.ts: lowercase override.headers keys in mockReq so a
  caller passing "Content-Type" matches the case-insensitive
  req.header() lookup.

Co-authored-by: Isaac
Signed-off-by: James Broadhead <jamesbroadhead@gmail.com>

---------

Signed-off-by: James Broadhead <jamesbroadhead@gmail.com>

Author: jamesbroadhead

packages/appkit/src/plugins/files/tests/_test-helpers.ts

@@ -0,0 +1,152 @@
+import { Readable } from "node:stream";
+import { mockServiceContext, setupDatabricksEnv } from "@tools/test-helpers";
+import { vi } from "vitest";
+import { ServiceContext } from "../../../context/service-context";
+import type { FilesPlugin } from "../plugin";
+import { policy } from "../policy";
+
+export const VOLUMES_CONFIG = {
+  volumes: {
+    uploads: { maxUploadSize: 100_000_000, policy: policy.allowAll() },
+    exports: { policy: policy.allowAll() },
+  },
+};
+
+/**
+ * Get a registered route handler from a FilesPlugin by HTTP method and path
+ * suffix. Useful when a test wants to invoke a single route in isolation.
+ */
+export function getRouteHandler(
+  plugin: FilesPlugin,
+  method: "get" | "post" | "delete",
+  pathSuffix: string,
+) {
+  const mockRouter = {
+    use: vi.fn(),
+    get: vi.fn(),
+    post: vi.fn(),
+    put: vi.fn(),
+    delete: vi.fn(),
+    patch: vi.fn(),
+  } as any;
+
+  plugin.injectRoutes(mockRouter);
+
+  const call = mockRouter[method].mock.calls.find(
+    (c: unknown[]) =>
+      typeof c[0] === "string" && (c[0] as string).endsWith(pathSuffix),
+  );
+  if (!call) throw new Error(`No route found for ${method} ...${pathSuffix}`);
+  return call[call.length - 1] as (req: any, res: any) => Promise<void>;
+}
+
+export function mockRes() {
+  const res: any = {
+    headersSent: false,
+  };
+  res.status = vi.fn().mockReturnValue(res);
+  res.json = vi.fn().mockReturnValue(res);
+  res.type = vi.fn().mockReturnValue(res);
+  res.send = vi.fn().mockReturnValue(res);
+  res.setHeader = vi.fn().mockReturnValue(res);
+  res.write = vi.fn().mockReturnValue(true);
+  res.destroy = vi.fn();
+  res.end = vi.fn();
+  res.on = vi.fn().mockReturnValue(res);
+  res.once = vi.fn().mockReturnValue(res);
+  res.emit = vi.fn().mockReturnValue(true);
+  res.removeListener = vi.fn().mockReturnValue(res);
+  res.pipe = vi.fn().mockReturnValue(res);
+  return res;
+}
+
+export function mockReq(
+  volumeKey: string,
+  overrides: Record<string, any> = {},
+): any {
+  // Lowercase override header keys so `req.header(name)` (case-insensitive
+  // via toLowerCase) matches them regardless of how callers cased the keys.
+  const lowercased: Record<string, string> = {};
+  for (const [k, v] of Object.entries(overrides.headers ?? {})) {
+    lowercased[k.toLowerCase()] = v as string;
+  }
+  const headers: Record<string, string> = {
+    "x-forwarded-access-token": "test-token",
+    "x-forwarded-user": "test-user",
+    ...lowercased,
+  };
+
+  const req: any = {
+    params: { volumeKey },
+    query: {},
+    ...overrides,
+    headers,
+    header: (name: string) => headers[name.toLowerCase()],
+  };
+
+  return req;
+}
+
+/**
+ * Mock Express request that behaves as a Node Readable stream — needed by the
+ * upload handler which calls Readable.toWeb(req).
+ */
+export function mockUploadReq(
+  volumeKey: string,
+  bodyChunks: Buffer[],
+  overrides: Record<string, any> = {},
+): any {
+  const headers: Record<string, string> = {
+    "x-forwarded-access-token": "test-token",
+    "x-forwarded-user": "test-user",
+    ...(overrides.headers ?? {}),
+  };
+
+  let chunkIndex = 0;
+  const stream = new Readable({
+    read() {
+      if (chunkIndex < bodyChunks.length) {
+        this.push(bodyChunks[chunkIndex++]);
+      } else {
+        this.push(null);
+      }
+    },
+  });
+
+  (stream as any).params = { volumeKey };
+  (stream as any).query = overrides.query ?? {};
+  (stream as any).headers = headers;
+  (stream as any).header = (name: string) => headers[name.toLowerCase()];
+  (stream as any).body = overrides.body;
+
+  return stream;
+}
+
+export function makeStreamResponse(content: string) {
+  const stream = new ReadableStream<Uint8Array>({
+    start(controller) {
+      controller.enqueue(new TextEncoder().encode(content));
+      controller.close();
+    },
+  });
+  return { contents: stream };
+}
+
+export async function setupTestEnv() {
+  vi.clearAllMocks();
+  setupDatabricksEnv();
+  ServiceContext.reset();
+  process.env.DATABRICKS_VOLUME_UPLOADS = "/Volumes/catalog/schema/uploads";
+  process.env.DATABRICKS_VOLUME_EXPORTS = "/Volumes/catalog/schema/exports";
+  return mockServiceContext();
+}
+
+export function teardownTestEnv(
+  serviceContextMock:
+    | Awaited<ReturnType<typeof mockServiceContext>>
+    | undefined,
+) {
+  serviceContextMock?.restore();
+  delete process.env.DATABRICKS_VOLUME_UPLOADS;
+  delete process.env.DATABRICKS_VOLUME_EXPORTS;
+}

packages/appkit/src/plugins/files/tests/delete.test.ts

@@ -0,0 +1,133 @@
+import { afterEach, beforeEach, describe, expect, test, vi } from "vitest";
+import { FilesPlugin } from "../plugin";
+import {
+  getRouteHandler,
+  mockReq,
+  mockRes,
+  setupTestEnv,
+  teardownTestEnv,
+  VOLUMES_CONFIG,
+} from "./_test-helpers";
+
+const { mockClient, MockApiError, mockCacheInstance } = vi.hoisted(() => {
+  const mockFilesApi = {
+    listDirectoryContents: vi.fn(),
+    download: vi.fn(),
+    getMetadata: vi.fn(),
+    upload: vi.fn(),
+    createDirectory: vi.fn(),
+    delete: vi.fn(),
+  };
+  const mockClient = {
+    files: mockFilesApi,
+    config: {
+      host: "https://test.databricks.com",
+      authenticate: vi.fn(),
+    },
+  };
+  class MockApiError extends Error {
+    statusCode: number;
+    constructor(message: string, statusCode: number) {
+      super(message);
+      this.name = "ApiError";
+      this.statusCode = statusCode;
+    }
+  }
+  const mockCacheInstance = {
+    get: vi.fn(),
+    set: vi.fn(),
+    delete: vi.fn(),
+    getOrExecute: vi.fn(async (_key: unknown[], fn: () => Promise<unknown>) =>
+      fn(),
+    ),
+    generateKey: vi.fn((...args: unknown[]) => JSON.stringify(args)),
+  };
+  return { mockClient, MockApiError, mockCacheInstance };
+});
+
+vi.mock("@databricks/sdk-experimental", () => ({
+  WorkspaceClient: vi.fn(() => mockClient),
+  ApiError: MockApiError,
+}));
+
+vi.mock("../../../context", async (importOriginal) => {
+  const actual = await importOriginal<typeof import("../../../context")>();
+  return {
+    ...actual,
+    getWorkspaceClient: vi.fn(() => mockClient),
+    isInUserContext: vi.fn(() => true),
+  };
+});
+
+vi.mock("../../../cache", () => ({
+  CacheManager: {
+    getInstanceSync: vi.fn(() => mockCacheInstance),
+  },
+}));
+
+describe("FilesPlugin delete", () => {
+  let serviceContextMock: Awaited<ReturnType<typeof setupTestEnv>>;
+
+  beforeEach(async () => {
+    serviceContextMock = await setupTestEnv();
+  });
+
+  afterEach(() => {
+    teardownTestEnv(serviceContextMock);
+  });
+
+  test("successful delete invalidates list cache", async () => {
+    const plugin = new FilesPlugin(VOLUMES_CONFIG);
+    const handler = getRouteHandler(plugin, "delete", "");
+    const res = mockRes();
+
+    mockClient.files.delete.mockResolvedValue(undefined);
+
+    await handler(
+      mockReq("uploads", {
+        query: { path: "/Volumes/catalog/schema/uploads/dir/file.txt" },
+      }),
+      res,
+    );
+
+    expect(res.json).toHaveBeenCalledWith(
+      expect.objectContaining({ success: true }),
+    );
+    expect(mockCacheInstance.generateKey).toHaveBeenCalled();
+    expect(mockCacheInstance.delete).toHaveBeenCalled();
+  });
+
+  test("delete without path returns 400", async () => {
+    const plugin = new FilesPlugin(VOLUMES_CONFIG);
+    const handler = getRouteHandler(plugin, "delete", "");
+    const res = mockRes();
+
+    await handler(mockReq("uploads", { query: {} }), res);
+
+    expect(res.status).toHaveBeenCalledWith(400);
+    expect(res.json).toHaveBeenCalledWith(
+      expect.objectContaining({ error: "path is required" }),
+    );
+  });
+
+  test("delete that throws ApiError returns proper status", async () => {
+    const plugin = new FilesPlugin(VOLUMES_CONFIG);
+    const handler = getRouteHandler(plugin, "delete", "");
+    const res = mockRes();
+
+    mockClient.files.delete.mockRejectedValue(
+      new MockApiError("Not found", 404),
+    );
+
+    await handler(
+      mockReq("uploads", {
+        query: { path: "/Volumes/catalog/schema/uploads/missing.txt" },
+      }),
+      res,
+    );
+
+    // SDK errors go through execute() which returns {ok: false, status: 404}
+    // then _sendStatusError is called with STATUS_CODES[404] = "Not Found"
+    expect(res.status).toHaveBeenCalledWith(404);
+  });
+});