From 3b7039803cf14cf973ca5bad27f854a4383c0157 Mon Sep 17 00:00:00 2001 From: Tiffany Trinh Date: Thu, 27 Aug 2026 12:42:41 -0400 Subject: [PATCH 01/14] feat(apps): wire local execution into the real dev server --- .../src/vite/dev-server.integration.test.ts | 137 ++++++++++ .../plugins/apps/src/vite/dev-server.test.ts | 249 ++++++++++++++++-- packages/plugins/apps/src/vite/dev-server.ts | 218 ++++++++++++--- packages/plugins/apps/src/vite/index.test.ts | 17 ++ packages/plugins/apps/src/vite/index.ts | 17 ++ .../apps/src/vite/local-execution.test.ts | 11 +- packages/tests/src/_jest/helpers/mocks.ts | 26 ++ 7 files changed, 614 insertions(+), 61 deletions(-) create mode 100644 packages/plugins/apps/src/vite/dev-server.integration.test.ts diff --git a/packages/plugins/apps/src/vite/dev-server.integration.test.ts b/packages/plugins/apps/src/vite/dev-server.integration.test.ts new file mode 100644 index 000000000..45fd02668 --- /dev/null +++ b/packages/plugins/apps/src/vite/dev-server.integration.test.ts @@ -0,0 +1,137 @@ +// Unless explicitly stated otherwise all files in this repository are licensed under the MIT License. +// This product includes software developed at Datadog (https://www.datadoghq.com/). +// Copyright 2019-Present Datadog, Inc. + +/** + * Real coverage for the local-execution path's module resolution: no mocked + * `viteBuild`/`loadModule`, no hand-written stand-in module. This spins up + * a real Vite dev server (`createServer`, middleware mode — no port bound) + * rooted at the same `apps_backend_project` fixture `backend/integration.test.ts` + * uses, and lets its real `ssrLoadModule` import a real `.backend.ts` file + * directly and execute it via the real `/__dd/executeAction` HTTP handler, + * including resolving `@datadog/apps-backend` from the fixture's own project + * root rather than build-plugins' own dependency tree. + * + * Does NOT register `getVitePlugin()`'s own transform hook on this server — + * `createServer` here has no `plugins:` array — so this does not exercise + * `vite/index.ts`'s `.backend.ts` → RPC-proxy transform or its interaction + * with `LOCAL_EXECUTION_LOAD_SUFFIX`; `index.test.ts` covers that hook + * directly instead. Registering the real plugin here (so this test also + * catches a regression in the plugin's own filter/handler wiring, not just + * the handler function in isolation) is a valuable, real follow-up. + * + * Uses `@datadog/apps-backend` (the fixture already has it as a real, + * locally-resolvable dependency — see `packages/tests/src/_jest/fixtures/ + * node_modules/@datadog/apps-backend`) rather than `@datadog/action-catalog` + * (no equivalent local fixture package exists yet for it). + * `local-execution.test.ts` already separately proves a raw + * `$.Actions.foo.bar(...)` call and an action-catalog typed-wrapper call — + * which reduce to the same injected `executeAction` under the hood — route + * correctly. Building a real local `@datadog/action-catalog` fixture package + * is a reasonable, cheap follow-up, not required for this coverage to be + * meaningful. + */ + +import { createDevServerMiddleware } from '@dd/apps-plugin/vite/dev-server'; +import { getMockLogger } from '@dd/tests/_jest/helpers/mocks'; +import { EventEmitter } from 'events'; +import type { IncomingMessage, ServerResponse } from 'http'; +import path from 'path'; +import { build, createServer, type ViteDevServer } from 'vite'; + +import { encodeQueryName } from '../backend/encodeQueryName'; +import type { BackendFunction } from '../backend/types'; + +const FIXTURE_ROOT = path.resolve( + __dirname, + '../../../../tests/src/_jest/fixtures/apps_backend_project', +); + +const getRuntimeUsersFunc: BackendFunction = { + relativePath: 'getRuntimeUsers', + name: 'getRuntimeUsers', + absolutePath: path.join(FIXTURE_ROOT, 'getRuntimeUsers.backend.ts'), + allowedConnectionIds: [], +}; + +function createMockRequest(url: string, body: Record): IncomingMessage { + const req = new EventEmitter() as unknown as IncomingMessage; + req.method = 'POST'; + req.url = url; + process.nextTick(() => { + (req as unknown as EventEmitter).emit('data', Buffer.from(JSON.stringify(body))); + (req as unknown as EventEmitter).emit('end'); + }); + return req; +} + +function createMockResponse() { + let body = ''; + let resolveDone: () => void; + const done = new Promise((resolve) => { + resolveDone = resolve; + }); + const res = { + statusCode: 200, + setHeader: jest.fn(), + end: jest.fn((data: string) => { + body = data || ''; + resolveDone(); + }), + getBody() { + return body; + }, + done, + }; + return res as typeof res & ServerResponse; +} + +describe('Dev Server Middleware — real end-to-end local execution', () => { + let server: ViteDevServer; + + beforeAll(async () => { + server = await createServer({ + configFile: false, + root: FIXTURE_ROOT, + logLevel: 'silent', + server: { middlewareMode: true, hmr: false }, + ssr: { noExternal: true }, + }); + }); + + afterAll(async () => { + await server.close(); + }); + + test('Should import a real backend function directly via the real Vite dev server and execute it locally, with a real @datadog/apps-backend typed import resolving $.Source correctly', async () => { + const middleware = createDevServerMiddleware( + build, + server.ssrLoadModule.bind(server), + () => [getRuntimeUsersFunc], + { site: 'datadoghq.com' }, + undefined, // no auth configured — this function never calls $.Actions + FIXTURE_ROOT, + getMockLogger(), + ); + + const req = createMockRequest('/__dd/executeAction', { + functionName: encodeQueryName(getRuntimeUsersFunc), + args: ['e2e-test'], + }); + const res = createMockResponse(); + + middleware(req, res, jest.fn()); + await res.done; + + expect(res.statusCode).toBe(200); + const body = JSON.parse(res.getBody()); + expect(body.success).toBe(true); + expect(body.result).toEqual({ + data: { + label: 'e2e-test', + executionUser: { id: 'local-dev', orgId: 'local-dev-org' }, + initiatingUser: { id: 'local-dev', orgId: 'local-dev-org' }, + }, + }); + }, 30000); +}); diff --git a/packages/plugins/apps/src/vite/dev-server.test.ts b/packages/plugins/apps/src/vite/dev-server.test.ts index 263df3e89..b1529f7ee 100644 --- a/packages/plugins/apps/src/vite/dev-server.test.ts +++ b/packages/plugins/apps/src/vite/dev-server.test.ts @@ -2,10 +2,12 @@ // This product includes software developed at Datadog (https://www.datadoghq.com/). // Copyright 2019-Present Datadog, Inc. +/* global globalThis */ + import { getAuthenticatedRequest } from '@dd/apps-plugin/auth'; import { createDevServerMiddleware } from '@dd/apps-plugin/vite/dev-server'; import type { AuthOptionsWithDefaults } from '@dd/core/types'; -import { getMockLogger } from '@dd/tests/_jest/helpers/mocks'; +import { getMockLogger, moduleResolverFor } from '@dd/tests/_jest/helpers/mocks'; import { EventEmitter } from 'events'; import type { IncomingMessage, ServerResponse } from 'http'; import nock from 'nock'; @@ -28,6 +30,13 @@ jest.mock('@dd/core/helpers/oauth-request', () => ({ const mockViteBuild = jest.fn(); +/** + * Stands in for the real `server.ssrLoadModule` — the local executeAction + * path no longer bundles, so tests exercising it configure this directly + * instead of `mockBuildWithParsedBackend`. + */ +const mockLoadModule = jest.fn(); + const DD_API_ORIGIN = 'https://api.datadoghq.com'; const mockFunctions: BackendFunction[] = [ @@ -147,10 +156,20 @@ function mockBuildWithParsedBackend(code = '// code') { }); } +/** + * Configures `mockLoadModule` to resolve `func`'s absolute path to a module + * exporting a single named function, matching what the real `ssrLoadModule` + * returns for a real backend-function file. + */ +function mockLoadModuleReturning(func: BackendFunction, fn: (...args: never[]) => unknown) { + mockLoadModule.mockImplementation(moduleResolverFor(func, { [func.name]: fn })); +} + describe('Dev Server Middleware', () => { beforeEach(() => { jest.clearAllMocks(); mockViteBuild.mockReset(); + mockLoadModule.mockReset(); }); afterEach(() => { @@ -160,6 +179,7 @@ describe('Dev Server Middleware', () => { describe('createDevServerMiddleware routing', () => { const middleware = createDevServerMiddleware( mockViteBuild, + mockLoadModule, () => mockFunctions, mockAuth, getApiKeyRequest(), @@ -208,7 +228,28 @@ describe('Dev Server Middleware', () => { expect(res.end).toHaveBeenCalled(); }); - test('Should handle /__dd/executeAction POST', async () => { + test('Should handle /__dd/executeAction POST by running the function directly, no bundling, no network call', async () => { + mockLoadModuleReturning(mockFunctions[0], (arg) => arg); + + const req = createMockRequest('/__dd/executeAction', { + functionName: encodeQueryName(mockFunctions[0]), + args: ['world'], + }); + const res = createMockResponse(); + const next = jest.fn(); + + middleware(req, res, next); + expect(next).not.toHaveBeenCalled(); + + await res.done; + + expect(res.statusCode).toBe(200); + const body = JSON.parse(res.getBody()); + expect(body.success).toBe(true); + expect(body.result).toEqual({ data: 'world' }); + }); + + test('Should handle /__dd/executeActionViaCloud POST', async () => { mockBuildWithParsedBackend(); // Mock the Datadog API via nock. @@ -225,7 +266,7 @@ describe('Dev Server Middleware', () => { }, }); - const req = createMockRequest('/__dd/executeAction', { + const req = createMockRequest('/__dd/executeActionViaCloud', { functionName: encodeQueryName(mockFunctions[0]), args: ['world'], }); @@ -248,6 +289,7 @@ describe('Dev Server Middleware', () => { describe('debugBundle handler', () => { const middleware = createDevServerMiddleware( mockViteBuild, + mockLoadModule, () => mockFunctions, mockAuth, getApiKeyRequest(), @@ -321,9 +363,10 @@ describe('Dev Server Middleware', () => { }); }); - describe('executeAction handler', () => { + describe('executeActionViaCloud handler', () => { const middleware = createDevServerMiddleware( mockViteBuild, + mockLoadModule, () => mockFunctions, mockAuth, getApiKeyRequest(), @@ -332,7 +375,7 @@ describe('Dev Server Middleware', () => { ); test('Should return 400 for missing functionRef', async () => { - const req = createMockRequest('/__dd/executeAction', {}); + const req = createMockRequest('/__dd/executeActionViaCloud', {}); const res = createMockResponse(); middleware(req, res, jest.fn()); @@ -342,7 +385,7 @@ describe('Dev Server Middleware', () => { }); test('Should return 404 for unknown function', async () => { - const req = createMockRequest('/__dd/executeAction', { + const req = createMockRequest('/__dd/executeActionViaCloud', { functionName: 'nonexistent.nonexistent', }); const res = createMockResponse(); @@ -369,7 +412,7 @@ describe('Dev Server Middleware', () => { .post('/api/v2/app-builder/queries/preview-async') .reply(403, 'Forbidden'); - const req = createMockRequest('/__dd/executeAction', { + const req = createMockRequest('/__dd/executeActionViaCloud', { functionName: encodeQueryName(mockFunctions[0]), args: [], }); @@ -421,7 +464,7 @@ describe('Dev Server Middleware', () => { data: { attributes: { done: true, outputs: { data: { value: 42 } } } }, }); - const req = createMockRequest('/__dd/executeAction', { + const req = createMockRequest('/__dd/executeActionViaCloud', { functionName: encodeQueryName(mockFunctions[0]), args: ['hello', 42], }); @@ -449,6 +492,7 @@ describe('Dev Server Middleware', () => { const oauthMiddleware = createDevServerMiddleware( mockViteBuild, + mockLoadModule, () => mockFunctions, mockOauthOnlyAuth, getOAuthRequest(), @@ -469,7 +513,7 @@ describe('Dev Server Middleware', () => { data: { attributes: { done: true, outputs: { data: { ok: true } } } }, }); - const req = createMockRequest('/__dd/executeAction', { + const req = createMockRequest('/__dd/executeActionViaCloud', { functionName: encodeQueryName(mockFunctions[0]), args: [], }); @@ -488,6 +532,7 @@ describe('Dev Server Middleware', () => { test('Should return 400 with auth guidance when explicit API-key auth is missing keys', async () => { const noKeyMiddleware = createDevServerMiddleware( mockViteBuild, + mockLoadModule, () => mockFunctions, mockOauthOnlyAuth, undefined, @@ -495,7 +540,7 @@ describe('Dev Server Middleware', () => { mockLog, ); - const req = createMockRequest('/__dd/executeAction', { + const req = createMockRequest('/__dd/executeActionViaCloud', { functionName: encodeQueryName(mockFunctions[0]), args: [], }); @@ -546,7 +591,7 @@ describe('Dev Server Middleware', () => { }); const trickyArgs = ["don't break", "'); alert(1); //", '😀']; - const req = createMockRequest('/__dd/executeAction', { + const req = createMockRequest('/__dd/executeActionViaCloud', { functionName: encodeQueryName(mockFunctions[0]), args: trickyArgs, }); @@ -575,6 +620,7 @@ describe('Dev Server Middleware', () => { ]; const middlewareWithAllowlist = createDevServerMiddleware( mockViteBuild, + mockLoadModule, () => functionsWithAllowlist, mockAuth, getApiKeyRequest(), @@ -605,7 +651,7 @@ describe('Dev Server Middleware', () => { data: { attributes: { done: true, outputs: { data: { ok: true } } } }, }); - const req = createMockRequest('/__dd/executeAction', { + const req = createMockRequest('/__dd/executeActionViaCloud', { functionName: encodeQueryName(functionsWithAllowlist[1]), args: [], }); @@ -660,7 +706,7 @@ describe('Dev Server Middleware', () => { data: { attributes: { done: true, outputs: { data: { ok: true } } } }, }); - const req = createMockRequest('/__dd/executeAction', { + const req = createMockRequest('/__dd/executeActionViaCloud', { functionName: encodeQueryName(mockFunctions[0]), args: [], }); @@ -687,7 +733,7 @@ describe('Dev Server Middleware', () => { errors: [{ title: 'ExecutionFailed', detail: 'Script threw an error' }], }); - const req = createMockRequest('/__dd/executeAction', { + const req = createMockRequest('/__dd/executeActionViaCloud', { functionName: encodeQueryName(mockFunctions[0]), args: [], }); @@ -715,7 +761,7 @@ describe('Dev Server Middleware', () => { data: { attributes: { done: true, outputs: { data: { ok: true } } } }, }); - const req = createMockRequest('/__dd/executeAction', { + const req = createMockRequest('/__dd/executeActionViaCloud', { functionName: encodeQueryName(mockFunctions[0]), args: [], }); @@ -732,11 +778,184 @@ describe('Dev Server Middleware', () => { }); }); + describe('executeAction handler (local)', () => { + const middleware = createDevServerMiddleware( + mockViteBuild, + mockLoadModule, + () => mockFunctions, + mockAuth, + getApiKeyRequest(), + '/project', + mockLog, + ); + + test('Should return 400 for missing functionRef', async () => { + const req = createMockRequest('/__dd/executeAction', {}); + const res = createMockResponse(); + + middleware(req, res, jest.fn()); + await res.done; + + expect(res.statusCode).toBe(400); + }); + + test('Should return 404 for unknown function', async () => { + const req = createMockRequest('/__dd/executeAction', { + functionName: 'nonexistent.nonexistent', + }); + const res = createMockResponse(); + + middleware(req, res, jest.fn()); + await res.done; + + expect(res.statusCode).toBe(404); + }); + + test('Should run the function directly in-process and return its result, with no bundling and no network call', async () => { + mockLoadModuleReturning(mockFunctions[0], (arg: number) => arg * 2); + + const req = createMockRequest('/__dd/executeAction', { + functionName: encodeQueryName(mockFunctions[0]), + args: [21], + }); + const res = createMockResponse(); + + middleware(req, res, jest.fn()); + await res.done; + + expect(res.statusCode).toBe(200); + const body = JSON.parse(res.getBody()); + expect(body.success).toBe(true); + expect(body.result).toEqual({ data: 42 }); + expect(mockViteBuild).not.toHaveBeenCalled(); + }); + + test('Should work with no auth configured at all, for a function that never calls $.Actions', async () => { + const noAuthMiddleware = createDevServerMiddleware( + mockViteBuild, + mockLoadModule, + () => mockFunctions, + mockOauthOnlyAuth, + undefined, + '/project', + mockLog, + ); + mockLoadModuleReturning(mockFunctions[0], () => 1); + + const req = createMockRequest('/__dd/executeAction', { + functionName: encodeQueryName(mockFunctions[0]), + args: [], + }); + const res = createMockResponse(); + + noAuthMiddleware(req, res, jest.fn()); + await res.done; + + expect(res.statusCode).toBe(200); + const body = JSON.parse(res.getBody()); + expect(body.success).toBe(true); + expect(body.result).toEqual({ data: 1 }); + }); + + test('Should return a clear error when a function calls $.Actions with no auth configured', async () => { + const noAuthMiddleware = createDevServerMiddleware( + mockViteBuild, + mockLoadModule, + () => mockFunctions, + mockOauthOnlyAuth, + undefined, + '/project', + mockLog, + ); + mockLoadModuleReturning(mockFunctions[0], () => + (globalThis as Record).$.Actions.slack.chat.postMessage({ + inputs: { text: 'hi' }, + }), + ); + + const req = createMockRequest('/__dd/executeAction', { + functionName: encodeQueryName(mockFunctions[0]), + args: [], + }); + const res = createMockResponse(); + + noAuthMiddleware(req, res, jest.fn()); + await res.done; + + expect(res.statusCode).toBe(500); + const body = JSON.parse(res.getBody()); + expect(body.success).toBe(false); + expect(body.error).toContain('Auth credentials not configured'); + }); + + test('Should route a real $.Actions call (including connectionId) through a direct single-action preview-async query, not the jsFunctionWithActions wrapper', async () => { + mockLoadModuleReturning(mockFunctions[0], () => + (globalThis as Record).$.Actions.slack.chat.postMessage({ + inputs: { text: 'hi' }, + connectionId: 'conn-1', + }), + ); + + type PreviewAsyncBody = { + data: { + attributes: { + query: { + properties: { + spec: { + fqn: string; + inputs: Record; + connectionId?: string; + }; + }; + }; + }; + }; + }; + let capturedBody: PreviewAsyncBody | undefined; + const apiScope = nock(DD_API_ORIGIN) + .post('/api/v2/app-builder/queries/preview-async', (body) => { + capturedBody = body as PreviewAsyncBody; + return true; + }) + .reply(200, { data: { id: 'receipt-action' } }) + .get('/api/v2/app-builder/queries/execution-long-polling/receipt-action') + .reply(200, { + data: { attributes: { done: true, outputs: { ok: true, ts: '123' } } }, + }); + + const req = createMockRequest('/__dd/executeAction', { + functionName: encodeQueryName(mockFunctions[0]), + args: [], + }); + const res = createMockResponse(); + + middleware(req, res, jest.fn()); + await res.done; + + expect(res.statusCode).toBe(200); + const body = JSON.parse(res.getBody()); + expect(body.success).toBe(true); + // The action's raw output ({ok, ts}, its own schema, not wrapped + // by preview-async itself) is what $.Actions.foo.bar() resolves + // to; the outer {data: ...} comes from the function's own return + // value going through executeScriptLocally's usual wrapping, not + // from anything action-specific. + expect(body.result).toEqual({ data: { ok: true, ts: '123' } }); + expect(apiScope.isDone()).toBe(true); + expect(capturedBody?.data.attributes.query.properties.spec).toEqual({ + fqn: 'com.datadoghq.slack.chat.postMessage', + inputs: { text: 'hi' }, + connectionId: 'conn-1', + }); + }); + }); + describe('dynamic discovery', () => { test('Should not find stale function after re-transform (HMR)', async () => { let currentFunctions: BackendFunction[] = [...mockFunctions]; const middleware = createDevServerMiddleware( mockViteBuild, + mockLoadModule, () => currentFunctions, mockAuth, getApiKeyRequest(), diff --git a/packages/plugins/apps/src/vite/dev-server.ts b/packages/plugins/apps/src/vite/dev-server.ts index f982275e9..801aaaf12 100644 --- a/packages/plugins/apps/src/vite/dev-server.ts +++ b/packages/plugins/apps/src/vite/dev-server.ts @@ -18,6 +18,8 @@ import { generateDevVirtualEntryContent } from '../backend/virtual-entry'; import { createBackendConnectionIdCollector } from './backend-connection-id-collector'; import { getBaseBackendBuildConfig } from './build-config'; +import type { ExecuteAction, LoadModule } from './local-execution'; +import { executeScriptLocally } from './local-execution'; interface BundleResult { func: BackendFunction; @@ -120,18 +122,20 @@ async function bundleBackendFunction( } /** - * Execute a script via Datadog's app-builder queries API. + * Submit a query to Datadog's app-builder `preview-async` endpoint and + * return its receipt ID. `querySpec` is the query's own `spec` object — + * either the `jsFunctionWithActions` wrapper (a whole script) or a single + * real action's own `{fqn, inputs}` directly (see `makeExecuteActionRemotely` + * below) — `submitQuery` itself doesn't care which. */ -async function executeScriptViaDatadog( - scriptBody: string, - func: BackendFunction, - args: unknown[], +async function submitQuery( + querySpec: Record, + displayName: string, auth: AuthConfig, doAuthenticatedRequest: DoAuthenticatedRequest, log: Logger, -): Promise { +): Promise { const endpoint = `https://api.${auth.site}/api/v2/app-builder/queries/preview-async`; - const displayName = formatRef(func); log.debug(`Calling Datadog API: ${endpoint}`); @@ -144,14 +148,7 @@ async function executeScriptViaDatadog( name: displayName, type: 'action', properties: { - spec: { - fqn: 'com.datadoghq.datatransformation.jsFunctionWithActions', - inputs: { - script: scriptBody, - allowedConnectionIds: func.allowedConnectionIds, - context: { backendFunctionArgs: args }, - }, - }, + spec: querySpec, onlyTriggerManually: true, }, }, @@ -178,36 +175,107 @@ async function executeScriptViaDatadog( log.debug(`Query execution started with receipt: ${receiptId}`); - return pollQueryExecution(receiptId, auth, doAuthenticatedRequest, log); + return receiptId; +} + +/** + * Execute a script via Datadog's app-builder queries API — the existing + * production round trip, unchanged. Wraps the whole script as a + * `jsFunctionWithActions` query. + */ +async function executeScriptViaDatadog( + scriptBody: string, + func: BackendFunction, + args: unknown[], + auth: AuthConfig, + doAuthenticatedRequest: DoAuthenticatedRequest, + log: Logger, +): Promise { + const displayName = formatRef(func); + + const receiptId = await submitQuery( + { + fqn: 'com.datadoghq.datatransformation.jsFunctionWithActions', + inputs: { + script: scriptBody, + allowedConnectionIds: func.allowedConnectionIds, + context: { backendFunctionArgs: args }, + }, + }, + displayName, + auth, + doAuthenticatedRequest, + log, + ); + + const outputs = await pollQueryExecution(receiptId, auth, doAuthenticatedRequest, log); + if (typeof outputs !== 'object' || outputs === null || !('data' in outputs)) { + throw new Error('Query execution completed without a "data" field in its outputs'); + } + return outputs; +} + +/** + * Build the real `$.Actions` implementation local execution injects: each + * call submits its own direct, single-action `preview-async` query — the + * action's own `{fqn, inputs, connectionId}`, not wrapped in a + * `jsFunctionWithActions` script — and polls it the same way the whole-script + * path does. This is the v1 mechanism decided in the RFC's Decisions and + * Trade-Offs: it needs nothing new from Action Platform and works today. No + * auth check happens until an action call is actually made — a script that + * never calls `$.Actions` runs locally with no auth configured at all. + */ +function makeExecuteActionRemotely( + auth: AuthConfig, + doAuthenticatedRequest: DoAuthenticatedRequest | undefined, + log: Logger, +): ExecuteAction { + return async ( + fqn: string, + inputs: unknown, + connectionId: string | undefined, + ): Promise => { + if (!doAuthenticatedRequest) { + throw new Error(`Auth credentials not configured. ${AUTH_GUIDANCE}`); + } + const receiptId = await submitQuery( + connectionId ? { fqn, inputs, connectionId } : { fqn, inputs }, + fqn, + auth, + doAuthenticatedRequest, + log, + ); + return pollQueryExecution(receiptId, auth, doAuthenticatedRequest, log); + }; } interface PollResult { - data?: { attributes?: { done?: boolean; outputs?: BackendOutputs } }; + data?: { attributes?: { done?: boolean; outputs?: unknown } }; errors?: Array<{ detail?: string; title?: string }>; } +/** + * Long-poll Datadog API until a submitted query's execution completes or + * times out. Returns the raw `outputs` value — shape varies by query type + * (a `jsFunctionWithActions` query wraps its result as `{data: }`; + * a direct single-action query's `outputs` is that action's own defined + * output schema) — callers interpret it accordingly. + * + * The server holds each poll connection open (~30s) and responds with + * done: true when the result is ready, or done: false when its long-poll + * window expires. This loop handles application-level re-polling + * (done: false), not HTTP retries — doRequest already retries transient + * HTTP failures (5xx, network errors) internally. + */ async function pollQueryExecution( receiptId: string, auth: AuthConfig, doAuthenticatedRequest: DoAuthenticatedRequest, log: Logger, -): Promise { +): Promise { const endpoint = `https://api.${auth.site}/api/v2/app-builder/queries/execution-long-polling/${receiptId}`; const maxRetries = 10; - /* - * Long-poll Datadog API until the query execution completes or times out. - * - * Executing an action works in two phases: - * 1. executeScriptViaDatadog sends a POST to preview-async, which starts the - * query and returns a receipt ID immediately. - * 2. This function polls the execution-long-polling endpoint with that receipt ID. - * The server holds the connection open (~30s) and responds with done: true when - * the result is ready, or done: false when its long-poll window expires. - * - * This loop handles application-level re-polling (done: false), not HTTP retries. - * doRequest already retries transient HTTP failures (5xx, network errors) internally. - */ for (let attempt = 0; attempt < maxRetries; attempt++) { log.debug(`Long-poll attempt ${attempt + 1}/${maxRetries}...`); @@ -226,7 +294,7 @@ async function pollQueryExecution( log.debug(`Long-poll response, done: ${attrs?.done}`); if (attrs?.done) { - if (!attrs.outputs) { + if (attrs.outputs === undefined) { throw new Error('Query execution completed without outputs'); } return attrs.outputs; @@ -303,9 +371,72 @@ async function handleDebugBundle( } /** - * Handle POST /__dd/executeAction — bundles a backend function and executes it via Datadog API. + * Parse the request body and look up the backend function by encoded query + * name — the same validation `validateAndBundle` does, minus the bundle step + * `handleExecuteAction` no longer needs. + */ +async function parseAndLookupFunction( + req: IncomingMessage, + functionsByName: Map, +): Promise<{ func: BackendFunction; args: unknown[] }> { + const { functionName, args = [] } = await parseRequestBody(req); + + if (!functionName || typeof functionName !== 'string') { + throw new HttpError(400, 'Missing or invalid functionName'); + } + + const func = functionsByName.get(functionName); + if (!func) { + throw new HttpError(404, `Backend function "${functionName}" not found`); + } + + return { func, args }; +} + +/** + * Handle POST /__dd/executeAction — imports a backend function's real file + * directly and executes it in-process (see local-execution.ts); no bundling + * on this path. Customer-facing default: no auth required upfront, since the + * script itself doesn't need it — only a real `$.Actions` call does, and + * that's checked lazily (see makeExecuteActionRemotely). */ async function handleExecuteAction( + req: IncomingMessage, + res: ServerResponse, + functionsByName: Map, + auth: AuthConfig, + doAuthenticatedRequest: DoAuthenticatedRequest | undefined, + loadModule: LoadModule, + log: Logger, +): Promise { + try { + const { func, args } = await parseAndLookupFunction(req, functionsByName); + const displayName = formatRef(func); + + log.debug(`Executing action locally: ${displayName} with args`); + + const executeAction = makeExecuteActionRemotely(auth, doAuthenticatedRequest, log); + const result = await executeScriptLocally(func, args, executeAction, loadModule, log); + + res.statusCode = 200; + res.setHeader('Content-Type', 'application/json'); + res.end(JSON.stringify({ success: true, result } satisfies ExecuteActionResponse)); + } catch (error: unknown) { + const statusCode = error instanceof HttpError ? error.statusCode : 500; + const message = error instanceof Error ? error.message : 'Internal server error'; + log.debug(`Error handling executeAction: ${message}`); + sendError(res, statusCode, message); + } +} + +/** + * Handle POST /__dd/executeActionViaCloud — bundles a backend function and + * executes it via the existing production round trip (queue + Deno + * subprocess). Same behavior as `/__dd/executeAction` before this project: + * kept as a distinctly-purposed command (`npm run dev:verify`, Milestone 3) + * for pre-publish parity checks, not a mode flag on the same endpoint. + */ +async function handleExecuteActionViaCloud( req: IncomingMessage, res: ServerResponse, functionsByName: Map, @@ -318,7 +449,7 @@ async function handleExecuteAction( const { func, code, args } = await validateAndBundle(req, functionsByName, bundle); const displayName = formatRef(func); - log.debug(`Executing action: ${displayName} with args`); + log.debug(`Executing action via cloud: ${displayName} with args`); const result = await executeScriptViaDatadog( code, @@ -335,7 +466,7 @@ async function handleExecuteAction( } catch (error: unknown) { const statusCode = error instanceof HttpError ? error.statusCode : 500; const message = error instanceof Error ? error.message : 'Internal server error'; - log.debug(`Error handling executeAction: ${message}`); + log.debug(`Error handling executeActionViaCloud: ${message}`); sendError(res, statusCode, message); } } @@ -357,6 +488,7 @@ function buildFunctionMap(backendFunctions: BackendFunction[]): Map BackendFunction[], auth: AuthConfig, doAuthenticatedRequest: DoAuthenticatedRequest | undefined, @@ -375,7 +507,7 @@ export function createDevServerMiddleware( if (!doAuthenticatedRequest) { log.warn( - `Auth credentials not configured. The /__dd/executeAction endpoint will be unavailable. ${AUTH_GUIDANCE}`, + `Auth credentials not configured. Backend functions that call $.Actions will fail; the /__dd/executeActionViaCloud endpoint will be unavailable. ${AUTH_GUIDANCE}`, ); } @@ -392,11 +524,23 @@ export function createDevServerMiddleware( sendError(res, 500, 'Unexpected error'); }); } else if (req.url === '/__dd/executeAction') { + handleExecuteAction( + req, + res, + functionsByName, + auth, + doAuthenticatedRequest, + loadModule, + log, + ).catch(() => { + sendError(res, 500, 'Unexpected error'); + }); + } else if (req.url === '/__dd/executeActionViaCloud') { if (!doAuthenticatedRequest) { sendError(res, 400, `Auth credentials not configured. ${AUTH_GUIDANCE}`); return; } - handleExecuteAction( + handleExecuteActionViaCloud( req, res, functionsByName, diff --git a/packages/plugins/apps/src/vite/index.test.ts b/packages/plugins/apps/src/vite/index.test.ts index 3b8e6b049..2f6e86df0 100644 --- a/packages/plugins/apps/src/vite/index.test.ts +++ b/packages/plugins/apps/src/vite/index.test.ts @@ -277,4 +277,21 @@ describe('Backend Functions - getVitePlugin', () => { value: expect.stringMatching(/[/\\]apps-runtime\.mjs$/), }); }); + + test('Should force @datadog/apps-backend and @datadog/action-catalog through the SSR transform pipeline instead of externalizing them', () => { + // These SDKs ship ESM-only. Vite's dev-server SSR mode externalizes + // node_modules by default (a plain require(), for speed), which + // throws "Cannot use import statement outside a module" for an + // ESM-only package -- ssr.noExternal is what the local executeAction + // path's server.ssrLoadModule call depends on to load them correctly. + const plugin = getVitePlugin(defaultOptions); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const config = (plugin as any).config(); + + expect(config).toEqual({ + ssr: { + noExternal: ['@datadog/apps-backend', '@datadog/action-catalog'], + }, + }); + }); }); diff --git a/packages/plugins/apps/src/vite/index.ts b/packages/plugins/apps/src/vite/index.ts index 6002db081..2a6360456 100644 --- a/packages/plugins/apps/src/vite/index.ts +++ b/packages/plugins/apps/src/vite/index.ts @@ -123,6 +123,22 @@ export const getVitePlugin = ({ const { setBackendFunctions, getBackendFunctions } = createBackendFunctionRegistry(); return { + // The dev server's local-execution path loads backend-function + // dependencies (e.g. @datadog/apps-backend, @datadog/action-catalog) + // via `server.ssrLoadModule`, which by default externalizes + // node_modules packages (a plain `require()`, for speed) rather than + // transforming them. Those two SDKs ship ESM-only, so an externalized + // `require()` throws "Cannot use import statement outside a module". + // `ssr.noExternal` forces Vite's SSR transform pipeline to handle + // them instead, matching how the production bundling path already + // inlines every dependency by default. + config() { + return { + ssr: { + noExternal: ['@datadog/apps-backend', '@datadog/action-catalog'], + }, + }; + }, transform: { filter: { id: { @@ -216,6 +232,7 @@ export const getVitePlugin = ({ server.middlewares.use( createDevServerMiddleware( bundler.build, + server.ssrLoadModule.bind(server), getBackendFunctions, auth, doAuthenticatedRequest, diff --git a/packages/plugins/apps/src/vite/local-execution.test.ts b/packages/plugins/apps/src/vite/local-execution.test.ts index 27f5360d3..f46e368c5 100644 --- a/packages/plugins/apps/src/vite/local-execution.test.ts +++ b/packages/plugins/apps/src/vite/local-execution.test.ts @@ -4,7 +4,7 @@ /* global globalThis, NodeJS */ -import { mockLogFn, mockLogger } from '@dd/tests/_jest/helpers/mocks'; +import { mockLogFn, mockLogger, moduleResolverFor } from '@dd/tests/_jest/helpers/mocks'; import * as shared from '../backend/shared'; import type { BackendFunction } from '../backend/types'; @@ -46,14 +46,7 @@ const stubExecuteAction: ExecuteAction = async (fqn) => ({ data: null, stub: tru /** A `loadModule` double that resolves the customer's function from a map and rejects anything else with a module-not-found error, matching the common case where neither optional package is installed. */ function loadModuleReturning(exports: Record): LoadModule { - return async (specifier: string) => { - if (specifier === func.absolutePath + LOCAL_EXECUTION_LOAD_SUFFIX) { - return exports; - } - const error: NodeJS.ErrnoException = new Error(`Cannot find module '${specifier}'`); - error.code = 'MODULE_NOT_FOUND'; - throw error; - }; + return moduleResolverFor(func, exports); } const ORDER_MARKER = '__ddLocalExecutionTestOrder'; diff --git a/packages/tests/src/_jest/helpers/mocks.ts b/packages/tests/src/_jest/helpers/mocks.ts index 907dfda97..b92a13683 100644 --- a/packages/tests/src/_jest/helpers/mocks.ts +++ b/packages/tests/src/_jest/helpers/mocks.ts @@ -2,6 +2,11 @@ // This product includes software developed at Datadog (https://www.datadoghq.com/). // Copyright 2019-Present Datadog, Inc. +/* global NodeJS */ + +import type { BackendFunction } from '@dd/apps-plugin/backend/types'; +import { LOCAL_EXECUTION_LOAD_SUFFIX } from '@dd/apps-plugin/constants'; +import type { LoadModule } from '@dd/apps-plugin/vite/local-execution'; import { DEFAULT_SITE } from '@dd/core/constants'; import { checkFile, @@ -120,6 +125,27 @@ export const getMockTimeLogger = (overrides: Partial = {}): TimeLogg return mockTimer; }; +/** + * Builds a `loadModule`-shaped resolver that returns `exports` for `func`'s + * absolute path (as requested by local execution's own suffixed specifier — + * see `LOCAL_EXECUTION_LOAD_SUFFIX`) and rejects any other specifier — + * matching what a real module loader returns when only the target module is + * actually resolvable. + */ +export const moduleResolverFor = ( + func: BackendFunction, + exports: Record, +): LoadModule => { + return async (specifier: string) => { + if (specifier === func.absolutePath + LOCAL_EXECUTION_LOAD_SUFFIX) { + return exports; + } + const error: NodeJS.ErrnoException = new Error(`Cannot find module '${specifier}'`); + error.code = 'MODULE_NOT_FOUND'; + throw error; + }; +}; + export const mockLogFn = jest.fn((text: any, level: LogLevel) => {}); export const getMockLogger = (overrides: Partial = {}): Logger => ({ getLogger: jest.fn(), From 6c11054490c15981f9b033f9aba28c27c1a6d0ee Mon Sep 17 00:00:00 2001 From: Tiffany Trinh Date: Thu, 27 Aug 2026 12:42:57 -0400 Subject: [PATCH 02/14] feat(apps): surface $.Actions result/error detail to the local console --- .../src/vite/dev-server.integration.test.ts | 72 ++++++++++++++++--- .../plugins/apps/src/vite/dev-server.test.ts | 69 +++++++++++++++++- packages/plugins/apps/src/vite/dev-server.ts | 30 +++++--- packages/plugins/apps/src/vite/index.ts | 43 +++++++++++ .../apps/src/vite/local-execution.test.ts | 48 +++++++++++++ .../plugins/apps/src/vite/local-execution.ts | 43 +++++++++-- .../nestedImport.backend.ts | 9 +++ 7 files changed, 289 insertions(+), 25 deletions(-) create mode 100644 packages/tests/src/_jest/fixtures/apps_backend_project/nestedImport.backend.ts diff --git a/packages/plugins/apps/src/vite/dev-server.integration.test.ts b/packages/plugins/apps/src/vite/dev-server.integration.test.ts index 45fd02668..d33c1de58 100644 --- a/packages/plugins/apps/src/vite/dev-server.integration.test.ts +++ b/packages/plugins/apps/src/vite/dev-server.integration.test.ts @@ -12,13 +12,11 @@ * including resolving `@datadog/apps-backend` from the fixture's own project * root rather than build-plugins' own dependency tree. * - * Does NOT register `getVitePlugin()`'s own transform hook on this server — - * `createServer` here has no `plugins:` array — so this does not exercise - * `vite/index.ts`'s `.backend.ts` → RPC-proxy transform or its interaction - * with `LOCAL_EXECUTION_LOAD_SUFFIX`; `index.test.ts` covers that hook - * directly instead. Registering the real plugin here (so this test also - * catches a regression in the plugin's own filter/handler wiring, not just - * the handler function in isolation) is a valuable, real follow-up. + * The nested-import test below registers the real `getVitePlugin()` hooks + * on this server (previous versions of this file didn't, and left that as a + * documented follow-up) — needed specifically to exercise `resolveId`'s + * `LOCAL_EXECUTION_LOAD_SUFFIX` propagation against Vite's own real module + * resolution, which a mocked `this.resolve()` can't reproduce. * * Uses `@datadog/apps-backend` (the fixture already has it as a real, * locally-resolvable dependency — see `packages/tests/src/_jest/fixtures/ @@ -33,11 +31,12 @@ */ import { createDevServerMiddleware } from '@dd/apps-plugin/vite/dev-server'; -import { getMockLogger } from '@dd/tests/_jest/helpers/mocks'; +import { getVitePlugin } from '@dd/apps-plugin/vite/index'; +import { getContextMock, getMockLogger } from '@dd/tests/_jest/helpers/mocks'; import { EventEmitter } from 'events'; import type { IncomingMessage, ServerResponse } from 'http'; import path from 'path'; -import { build, createServer, type ViteDevServer } from 'vite'; +import { build, createServer, type Plugin, type ViteDevServer } from 'vite'; import { encodeQueryName } from '../backend/encodeQueryName'; import type { BackendFunction } from '../backend/types'; @@ -86,16 +85,37 @@ function createMockResponse() { return res as typeof res & ServerResponse; } +const nestedImportFunc: BackendFunction = { + relativePath: 'nestedImport', + name: 'usesNestedImport', + absolutePath: path.join(FIXTURE_ROOT, 'nestedImport.backend.ts'), + allowedConnectionIds: [], +}; + describe('Dev Server Middleware — real end-to-end local execution', () => { let server: ViteDevServer; beforeAll(async () => { + const appsPlugin: Plugin = { + name: 'dd-apps-test', + ...getVitePlugin({ + bundler: { build }, + context: getContextMock({ buildRoot: FIXTURE_ROOT }), + options: { + authOverrides: { method: 'apiKey' }, + include: [], + dryRun: true, + }, + }), + }; + server = await createServer({ configFile: false, root: FIXTURE_ROOT, logLevel: 'silent', server: { middlewareMode: true, hmr: false }, ssr: { noExternal: true }, + plugins: [appsPlugin], }); }); @@ -134,4 +154,38 @@ describe('Dev Server Middleware — real end-to-end local execution', () => { }, }); }, 30000); + + // Real coverage for the resolveId propagation fix in vite/index.ts: + // nestedImport.backend.ts statically imports plainEcho from + // getRuntimeUsers.backend.ts. Without propagating + // LOCAL_EXECUTION_LOAD_SUFFIX onto that nested import, Vite would + // resolve it unsuffixed, the transform hook would replace it with the + // frontend RPC-proxy stub (calling globalThis.DD_APPS_RUNTIME, which + // doesn't exist server-side), and this would throw instead of returning + // the real value. + test('Should preserve real code for a nested *.backend.ts import, not swap it for the frontend RPC-proxy stub', async () => { + const middleware = createDevServerMiddleware( + build, + server.ssrLoadModule.bind(server), + () => [nestedImportFunc], + { site: 'datadoghq.com' }, + undefined, + FIXTURE_ROOT, + getMockLogger(), + ); + + const req = createMockRequest('/__dd/executeAction', { + functionName: encodeQueryName(nestedImportFunc), + args: ['nested-value'], + }); + const res = createMockResponse(); + + middleware(req, res, jest.fn()); + await res.done; + + expect(res.statusCode).toBe(200); + const body = JSON.parse(res.getBody()); + expect(body.success).toBe(true); + expect(body.result).toEqual({ data: { value: 'nested-value' } }); + }, 30000); }); diff --git a/packages/plugins/apps/src/vite/dev-server.test.ts b/packages/plugins/apps/src/vite/dev-server.test.ts index b1529f7ee..a0a2ca9ca 100644 --- a/packages/plugins/apps/src/vite/dev-server.test.ts +++ b/packages/plugins/apps/src/vite/dev-server.test.ts @@ -7,7 +7,7 @@ import { getAuthenticatedRequest } from '@dd/apps-plugin/auth'; import { createDevServerMiddleware } from '@dd/apps-plugin/vite/dev-server'; import type { AuthOptionsWithDefaults } from '@dd/core/types'; -import { getMockLogger, moduleResolverFor } from '@dd/tests/_jest/helpers/mocks'; +import { getMockLogger, mockLogFn, moduleResolverFor } from '@dd/tests/_jest/helpers/mocks'; import { EventEmitter } from 'events'; import type { IncomingMessage, ServerResponse } from 'http'; import nock from 'nock'; @@ -948,6 +948,73 @@ describe('Dev Server Middleware', () => { connectionId: 'conn-1', }); }); + + test("Should surface a successful $.Actions call's result to the local console", async () => { + mockLoadModuleReturning(mockFunctions[0], () => + (globalThis as Record).$.Actions.slack.chat.postMessage({ + inputs: { text: 'hi' }, + }), + ); + + nock(DD_API_ORIGIN) + .post('/api/v2/app-builder/queries/preview-async') + .reply(200, { data: { id: 'receipt-success' } }) + .get('/api/v2/app-builder/queries/execution-long-polling/receipt-success') + .reply(200, { + data: { attributes: { done: true, outputs: { ok: true } } }, + }); + + const req = createMockRequest('/__dd/executeAction', { + functionName: encodeQueryName(mockFunctions[0]), + args: [], + }); + const res = createMockResponse(); + + middleware(req, res, jest.fn()); + await res.done; + + expect(res.statusCode).toBe(200); + expect(mockLogFn).toHaveBeenCalledWith( + expect.stringContaining('com.datadoghq.slack.chat.postMessage'), + 'info', + ); + expect(mockLogFn).toHaveBeenCalledWith(expect.stringContaining('"ok":true'), 'info'); + }); + + test("Should surface a failed $.Actions call's error detail to the local console", async () => { + mockLoadModuleReturning(mockFunctions[0], () => + (globalThis as Record).$.Actions.slack.chat.postMessage({ + inputs: { text: 'hi' }, + }), + ); + + nock(DD_API_ORIGIN) + .post('/api/v2/app-builder/queries/preview-async') + .reply(200, { data: { id: 'receipt-failure' } }) + .get('/api/v2/app-builder/queries/execution-long-polling/receipt-failure') + .reply(200, { + errors: [{ detail: 'Connection is not authorized for this action' }], + }); + + const req = createMockRequest('/__dd/executeAction', { + functionName: encodeQueryName(mockFunctions[0]), + args: [], + }); + const res = createMockResponse(); + + middleware(req, res, jest.fn()); + await res.done; + + expect(res.statusCode).toBe(500); + expect(mockLogFn).toHaveBeenCalledWith( + expect.stringContaining('com.datadoghq.slack.chat.postMessage'), + 'error', + ); + expect(mockLogFn).toHaveBeenCalledWith( + expect.stringContaining('Connection is not authorized for this action'), + 'error', + ); + }); }); describe('dynamic discovery', () => { diff --git a/packages/plugins/apps/src/vite/dev-server.ts b/packages/plugins/apps/src/vite/dev-server.ts index 801aaaf12..a7bcd2670 100644 --- a/packages/plugins/apps/src/vite/dev-server.ts +++ b/packages/plugins/apps/src/vite/dev-server.ts @@ -224,6 +224,12 @@ async function executeScriptViaDatadog( * Trade-Offs: it needs nothing new from Action Platform and works today. No * auth check happens until an action call is actually made — a script that * never calls `$.Actions` runs locally with no auth configured at all. + * + * Logs the resolved result or error detail at info/error level — the + * Telemetry milestone's primary deliverable. Production's own equivalent + * signal only reaches Datadog's backend; a developer watching `npm run dev` + * would otherwise see no result at all for an action call beyond the + * `log.debug` breadcrumbs `submitQuery`/`pollQueryExecution` already emit. */ function makeExecuteActionRemotely( auth: AuthConfig, @@ -238,14 +244,22 @@ function makeExecuteActionRemotely( if (!doAuthenticatedRequest) { throw new Error(`Auth credentials not configured. ${AUTH_GUIDANCE}`); } - const receiptId = await submitQuery( - connectionId ? { fqn, inputs, connectionId } : { fqn, inputs }, - fqn, - auth, - doAuthenticatedRequest, - log, - ); - return pollQueryExecution(receiptId, auth, doAuthenticatedRequest, log); + try { + const receiptId = await submitQuery( + connectionId ? { fqn, inputs, connectionId } : { fqn, inputs }, + fqn, + auth, + doAuthenticatedRequest, + log, + ); + const result = await pollQueryExecution(receiptId, auth, doAuthenticatedRequest, log); + log.info(`$.Actions call to "${fqn}" succeeded: ${JSON.stringify(result)}`); + return result; + } catch (error: unknown) { + const message = error instanceof Error ? error.message : String(error); + log.error(`$.Actions call to "${fqn}" failed: ${message}`); + throw error; + } }; } diff --git a/packages/plugins/apps/src/vite/index.ts b/packages/plugins/apps/src/vite/index.ts index 2a6360456..b94f446ce 100644 --- a/packages/plugins/apps/src/vite/index.ts +++ b/packages/plugins/apps/src/vite/index.ts @@ -139,6 +139,49 @@ export const getVitePlugin = ({ }, }; }, + // Propagates the LOCAL_EXECUTION_LOAD_SUFFIX marker through the + // backend-file dependency graph: `ssrLoadModule` only tags the one + // entry module it's called with, so without this hook, a + // `.backend.ts` file statically importing another `.backend.ts` file + // would resolve that nested import unsuffixed, hitting transform's + // "not suffixed" branch below and getting replaced with the frontend + // RPC-proxy stub — breaking local execution for a multi-backend-file + // import graph. Only propagates when the importer itself was + // suffixed (this is local execution's own module graph, not a + // regular frontend import) and only onto another `.backend.ts` file + // (a plain helper module never hits the proxy-vs-real-code branching + // this marker exists to disambiguate, so it needs no suffix). + resolveId: { + // Must run before Vite's own built-in resolver: a plain relative + // specifier like `./other.backend` is fully resolvable by Vite's + // internal filesystem-based resolution alone, which — running at + // its default, unenforced order — would resolve and short-circuit + // the hook chain before this plugin's own resolveId ever saw it. + // `pre` guarantees this hook gets first look at every id. + order: 'pre', + async handler(source, importer, resolveOptions) { + if (!importer || !importer.endsWith(LOCAL_EXECUTION_LOAD_SUFFIX)) { + return null; + } + + const resolved = await this.resolve(source, importer, { + ...resolveOptions, + skipSelf: true, + }); + if (!resolved || resolved.external) { + return resolved; + } + + if ( + BACKEND_FILE_RE.test(resolved.id) && + !resolved.id.endsWith(LOCAL_EXECUTION_LOAD_SUFFIX) + ) { + return { ...resolved, id: resolved.id + LOCAL_EXECUTION_LOAD_SUFFIX }; + } + + return resolved; + }, + }, transform: { filter: { id: { diff --git a/packages/plugins/apps/src/vite/local-execution.test.ts b/packages/plugins/apps/src/vite/local-execution.test.ts index f46e368c5..5a8e32cd0 100644 --- a/packages/plugins/apps/src/vite/local-execution.test.ts +++ b/packages/plugins/apps/src/vite/local-execution.test.ts @@ -538,7 +538,55 @@ describe('local-execution — executeScriptLocally', () => { ); }); + // executeAction stands in for dev-server.ts's real makeExecuteActionRemotely, whose long-poll can legitimately outlast a short hang-detection timeout — that's network wait time, not a hung customer function. + test('Should not time out while a real $.Actions call is still legitimately in flight, even past the configured timeout', async () => { + const slowExecuteAction: ExecuteAction = () => + new Promise((resolve) => setTimeout(() => resolve({ ok: true }), 80)); + + const result = await executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + slowExecuteAction, + loadModuleReturning({ + example: () => + (globalThis as Record).$.Actions.slack.chat.postMessage({ + inputs: { text: 'hi' }, + }), + }), + mockLogger, + 50, // shorter than slowExecuteAction's own 80ms + ); + + expect(result).toEqual({ data: { ok: true } }); + }); + + test('Should still time out a function that hangs with no $.Actions call in flight, even after an earlier call in the same run completed', async () => { + const executeAction: ExecuteAction = async () => ({ ok: true }); + + await expect( + executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + executeAction, + loadModuleReturning({ + example: async () => { + await (globalThis as Record).$.Actions.slack.chat.postMessage({ + inputs: { text: 'hi' }, + }); + // Hangs with no further $.Actions call — the fresh timeout window from the completed call above must still expire normally. + return new Promise(() => {}); + }, + }), + mockLogger, + 50, + ), + ).rejects.toThrow(/timed out after 50ms/); + }); + // Asserts $'s exact key set, since a token added inside globalThis.$ wouldn't be caught by the weaker top-level check below. + test('Should never expose an auth token to the customer module — only backendFunctionArgs, Actions, and Source are visible on globalThis.$', async () => { const result = await executeScriptLocally( func, diff --git a/packages/plugins/apps/src/vite/local-execution.ts b/packages/plugins/apps/src/vite/local-execution.ts index 996e65f60..d4a97675a 100644 --- a/packages/plugins/apps/src/vite/local-execution.ts +++ b/packages/plugins/apps/src/vite/local-execution.ts @@ -475,7 +475,30 @@ async function runScriptLocally( // A timed-out execution is abandoned, not canceled — its fn() may keep running and must not act under a newer execution's identity. isCurrent() gates both this execution's own captured `$.Actions` closure and the shared adapters, which resolve the calling execution's dispatch from AsyncLocalStorage rather than whichever registration is currently live. const scope = executionEpoch.start(); - const guardedExecuteAction: ExecuteAction = (fqn, inputs, connectionId) => { + // `executeAction`'s own long-poll (dev-server.ts's pollQueryExecution) + // can legitimately take far longer than `timeoutMs` on its own — that's + // time spent waiting on a real network round trip, not evidence the + // customer function itself has hung. Pausing the hang-detection timer + // while at least one call is in flight, and giving it a fresh + // `timeoutMs` window once every in-flight call has settled, means a + // function that keeps making real progress via `$.Actions` is never + // penalized for it, while a function that genuinely hangs (with no + // `$.Actions` call in flight) still times out at the same `timeoutMs` + // it always did. + let timer: ReturnType | undefined; + let rejectTimeout: ((error: Error) => void) | undefined; + let pendingActionCalls = 0; + + const scheduleTimeout = () => { + timer = setTimeout(() => { + concludeExecution(); + rejectTimeout?.( + new Error(`Local execution of "${func.name}" timed out after ${timeoutMs}ms`), + ); + }, timeoutMs); + }; + + const guardedExecuteAction: ExecuteAction = async (fqn, inputs, connectionId) => { if (!scope.isCurrent()) { // A concluded scope stays concluded forever, not just "not the latest" — the wording stays conclusion-neutral rather than claiming a timeout that may not have happened. return Promise.reject( @@ -485,7 +508,16 @@ async function runScriptLocally( ), ); } - return executeAction(fqn, inputs, connectionId); + pendingActionCalls += 1; + clearTimeout(timer); + try { + return await executeAction(fqn, inputs, connectionId); + } finally { + pendingActionCalls -= 1; + if (pendingActionCalls === 0 && scope.isCurrent()) { + scheduleTimeout(); + } + } }; const concludeExecution = () => { @@ -552,12 +584,9 @@ async function runScriptLocally( ); }; - let timer: ReturnType | undefined; const timeout = new Promise((_resolve, reject) => { - timer = setTimeout(() => { - concludeExecution(); - reject(new Error(`Local execution of "${func.name}" timed out after ${timeoutMs}ms`)); - }, timeoutMs); + rejectTimeout = reject; + scheduleTimeout(); }); // Racing the timeout only stops the caller from waiting — run() keeps executing afterward, so a resumed customer function can still fire real $.Actions side effects; true cancellation would need a Worker thread, not possible in-process. diff --git a/packages/tests/src/_jest/fixtures/apps_backend_project/nestedImport.backend.ts b/packages/tests/src/_jest/fixtures/apps_backend_project/nestedImport.backend.ts new file mode 100644 index 000000000..bdeb98437 --- /dev/null +++ b/packages/tests/src/_jest/fixtures/apps_backend_project/nestedImport.backend.ts @@ -0,0 +1,9 @@ +// Unless explicitly stated otherwise all files in this repository are licensed under the MIT License. +// This product includes software developed at Datadog (https://www.datadoghq.com/). +// Copyright 2019-Present Datadog, Inc. + +import { plainEcho } from './getRuntimeUsers.backend'; + +export async function usesNestedImport(value: string) { + return plainEcho(value); +} From 361607b7a81be50a51a011bd32533efed3238c70 Mon Sep 17 00:00:00 2001 From: Tiffany Trinh Date: Thu, 27 Aug 2026 12:43:06 -0400 Subject: [PATCH 03/14] fix(apps): address dev-server wiring review findings --- packages/plugins/apps/package.json | 4 +- .../apps/src/vite/dev-server-module-graph.ts | 110 ++++++++++++++ .../src/vite/dev-server.integration.test.ts | 135 +++++++++++++++++- .../plugins/apps/src/vite/dev-server.test.ts | 61 ++++++-- packages/plugins/apps/src/vite/dev-server.ts | 108 +++++++------- packages/plugins/apps/src/vite/index.test.ts | 4 +- packages/plugins/apps/src/vite/index.ts | 82 ++++++++--- .../apps/src/vite/local-execution.test.ts | 15 +- .../published/esbuild-plugin/package.json | 1 + packages/published/rollup-plugin/package.json | 1 + packages/published/rspack-plugin/package.json | 1 + packages/published/vite-plugin/package.json | 1 + .../published/webpack-plugin/package.json | 1 + .../fixtures/apps_backend_project/helper.ts | 9 ++ .../apps_backend_project/viaHelper.backend.ts | 9 ++ 15 files changed, 453 insertions(+), 89 deletions(-) create mode 100644 packages/plugins/apps/src/vite/dev-server-module-graph.ts create mode 100644 packages/tests/src/_jest/fixtures/apps_backend_project/helper.ts create mode 100644 packages/tests/src/_jest/fixtures/apps_backend_project/viaHelper.backend.ts diff --git a/packages/plugins/apps/package.json b/packages/plugins/apps/package.json index a09626633..0470a0e08 100644 --- a/packages/plugins/apps/package.json +++ b/packages/plugins/apps/package.json @@ -35,12 +35,12 @@ "eslint-scope": "7.2.2", "glob": "11.1.0", "jszip": "3.10.1", - "pretty-bytes": "5.6.0" + "pretty-bytes": "5.6.0", + "rollup": "4.45.1" }, "devDependencies": { "@types/eslint-scope": "3.7.7", "@types/estree": "1.0.8", - "rollup": "4.45.1", "typescript": "5.4.3", "vite": "6.3.5" } diff --git a/packages/plugins/apps/src/vite/dev-server-module-graph.ts b/packages/plugins/apps/src/vite/dev-server-module-graph.ts new file mode 100644 index 000000000..3a8a29f3b --- /dev/null +++ b/packages/plugins/apps/src/vite/dev-server-module-graph.ts @@ -0,0 +1,110 @@ +// Unless explicitly stated otherwise all files in this repository are licensed under the MIT License. +// This product includes software developed at Datadog (https://www.datadoghq.com/). +// Copyright 2019-Present Datadog, Inc. + +import { parseAst } from 'rollup/parseAst'; +import type { ModuleNode, ViteDevServer } from 'vite'; + +import { + createParsedModuleRecord, + type ParsedModuleRecord, + shouldTraverseCollectedModule, + unsupportedModuleGraphDependency, +} from '../backend/ast-parsing/module-graph'; +import { LOCAL_EXECUTION_LOAD_SUFFIX } from '../constants'; + +/** + * Builds the same `ReadonlyMap` shape + * `createBackendModuleGraphCollector`'s `moduleParsed` hook produces during a + * real Rollup build — but for the dev server, where `moduleParsed` never + * fires at all (it's a Rollup-build-only hook; Vite's dev-server plugin + * container doesn't implement it). Instead, this walks Vite's own + * `server.moduleGraph`, which the dev server already populates as a side + * effect of `ssrLoadModule`: by the time an `await server.ssrLoadModule(id)` + * call resolves, the entry's `ModuleNode.importedModules` — and every + * imported module's own `importedModules` — already reflect the full + * transitive static-import graph, recursively, with no extra ticks needed. + * + * Call this only after `loadModule` has resolved for `bareEntryId + + * LOCAL_EXECUTION_LOAD_SUFFIX` in the same request — the graph it reads is a + * live side effect of that call, not independently maintained state. + * `bareEntryId` is the same unsuffixed id `extractConnectionIdsFromModuleGraph` + * needs to key into the returned map below; the suffix is appended here, + * internally, rather than left to each caller to remember — Vite keys the + * node it just loaded by the full resolved id (suffix included), since it + * treats each distinct query string as a logically distinct module. + */ +export function collectModuleGraphFromServer( + server: ViteDevServer, + bareEntryId: string, + buildRoot: string, +): ReadonlyMap { + const records = new Map(); + const visited = new Set(); + const pending: ModuleNode[] = []; + + const entryNode = server.moduleGraph.getModuleById(bareEntryId + LOCAL_EXECUTION_LOAD_SUFFIX); + if (entryNode) { + pending.push(entryNode); + } + + while (pending.length > 0) { + const node = pending.shift()!; + const moduleId = node.id ? normalizeViteModuleId(node.id) : undefined; + if (!moduleId || visited.has(moduleId)) { + continue; + } + visited.add(moduleId); + + if (!shouldTraverseCollectedModule(moduleId, buildRoot)) { + continue; + } + + // `transformResult` is the client/browser transform; SSR loads (what + // local execution always is, via server.ssrLoadModule) populate + // `ssrTransformResult` instead. Neither exists yet if Vite hasn't + // transformed this module — a local dependency the caller's own + // ssrLoadModule call never actually reached. + const transformResult = node.ssrTransformResult ?? node.transformResult; + if (typeof transformResult?.code !== 'string') { + continue; + } + + let ast; + try { + ast = parseAst(transformResult.code); + } catch (error) { + const reason = error instanceof Error ? error.message : String(error); + throw unsupportedModuleGraphDependency( + moduleId, + `unparseable module source (${reason})`, + ); + } + + // `deps` are this module's own static imports, already resolved to + // real module ids by Vite's import-analysis plugin — the SSR + // equivalent of `moduleParsed`'s `importedIds`/ + // `importedIdResolutions`. `dynamicDeps` (deliberately unused here) + // folds in `import()` calls; `module-graph.ts`'s own AST walk is what + // flags those as unsupported, so only static deps belong here. + const staticDependencyIds = (transformResult.deps ?? []).map(normalizeViteModuleId); + + const record = createParsedModuleRecord(moduleId, buildRoot, ast, staticDependencyIds); + if (record) { + records.set(record.id, record); + } + + for (const dependencyId of staticDependencyIds) { + const dependencyNode = server.moduleGraph.getModuleById(dependencyId); + if (dependencyNode) { + pending.push(dependencyNode); + } + } + } + + return records; +} + +function normalizeViteModuleId(id: string): string { + return id.split('?')[0]; +} diff --git a/packages/plugins/apps/src/vite/dev-server.integration.test.ts b/packages/plugins/apps/src/vite/dev-server.integration.test.ts index d33c1de58..814461a43 100644 --- a/packages/plugins/apps/src/vite/dev-server.integration.test.ts +++ b/packages/plugins/apps/src/vite/dev-server.integration.test.ts @@ -13,8 +13,7 @@ * root rather than build-plugins' own dependency tree. * * The nested-import test below registers the real `getVitePlugin()` hooks - * on this server (previous versions of this file didn't, and left that as a - * documented follow-up) — needed specifically to exercise `resolveId`'s + * on this server, needed specifically to exercise `resolveId`'s * `LOCAL_EXECUTION_LOAD_SUFFIX` propagation against Vite's own real module * resolution, which a mocked `this.resolve()` can't reproduce. * @@ -30,6 +29,7 @@ * meaningful. */ +import { collectModuleGraphFromServer } from '@dd/apps-plugin/vite/dev-server-module-graph'; import { createDevServerMiddleware } from '@dd/apps-plugin/vite/dev-server'; import { getVitePlugin } from '@dd/apps-plugin/vite/index'; import { getContextMock, getMockLogger } from '@dd/tests/_jest/helpers/mocks'; @@ -38,6 +38,7 @@ import type { IncomingMessage, ServerResponse } from 'http'; import path from 'path'; import { build, createServer, type Plugin, type ViteDevServer } from 'vite'; +import { extractConnectionIdsFromModuleGraph } from '../backend/ast-parsing/extract-connection-ids-from-module-graph'; import { encodeQueryName } from '../backend/encodeQueryName'; import type { BackendFunction } from '../backend/types'; @@ -92,6 +93,13 @@ const nestedImportFunc: BackendFunction = { allowedConnectionIds: [], }; +const viaHelperFunc: BackendFunction = { + relativePath: 'viaHelper', + name: 'usesHelper', + absolutePath: path.join(FIXTURE_ROOT, 'viaHelper.backend.ts'), + allowedConnectionIds: [], +}; + describe('Dev Server Middleware — real end-to-end local execution', () => { let server: ViteDevServer; @@ -114,7 +122,6 @@ describe('Dev Server Middleware — real end-to-end local execution', () => { root: FIXTURE_ROOT, logLevel: 'silent', server: { middlewareMode: true, hmr: false }, - ssr: { noExternal: true }, plugins: [appsPlugin], }); }); @@ -128,8 +135,10 @@ describe('Dev Server Middleware — real end-to-end local execution', () => { build, server.ssrLoadModule.bind(server), () => [getRuntimeUsersFunc], + () => [], { site: 'datadoghq.com' }, - undefined, // no auth configured — this function never calls $.Actions + // No auth configured — this function never calls $.Actions. + undefined, FIXTURE_ROOT, getMockLogger(), ); @@ -168,6 +177,7 @@ describe('Dev Server Middleware — real end-to-end local execution', () => { build, server.ssrLoadModule.bind(server), () => [nestedImportFunc], + () => [], { site: 'datadoghq.com' }, undefined, FIXTURE_ROOT, @@ -188,4 +198,121 @@ describe('Dev Server Middleware — real end-to-end local execution', () => { expect(body.success).toBe(true); expect(body.result).toEqual({ data: { value: 'nested-value' } }); }, 30000); + + // Real coverage for the multi-hop case the single-hop propagation above + // still misses: viaHelper.backend.ts imports helper.ts (a plain, + // non-backend module), which itself imports plainEcho from + // getRuntimeUsers.backend.ts. resolveId only appended the suffix when + // the DIRECT importer string ended with it, so helper.ts (reached + // through a suffixed importer, but never suffixed itself, since it + // isn't a *.backend.ts file) became an unsuffixed importer for its own + // import — silently dropping the marker one hop later than the direct + // backend-to-backend case above, and swapping getRuntimeUsers.backend.ts + // for its frontend RPC-proxy stub. + test('Should preserve real code for a *.backend.ts import reached through an intermediate non-backend module', async () => { + const middleware = createDevServerMiddleware( + build, + server.ssrLoadModule.bind(server), + () => [viaHelperFunc], + () => [], + { site: 'datadoghq.com' }, + undefined, + FIXTURE_ROOT, + getMockLogger(), + ); + + const req = createMockRequest('/__dd/executeAction', { + functionName: encodeQueryName(viaHelperFunc), + args: ['via-helper-value'], + }); + const res = createMockResponse(); + + middleware(req, res, jest.fn()); + await res.done; + + expect(res.statusCode).toBe(200); + const body = JSON.parse(res.getBody()); + expect(body.success).toBe(true); + expect(body.result).toEqual({ data: { value: 'via-helper-value' } }); + }, 30000); + + // Regression coverage for the real configureServer-installed middleware, + // not a hand-built one: every other test in this file constructs its own + // middleware via createDevServerMiddleware(..., () => [], ...), which + // bypasses getAllowedConnectionIds' real wiring entirely (a hardcoded + // () => [] never exercises collectModuleGraphFromServer at all). Sending + // the request through server.middlewares — the real Connect stack + // getVitePlugin's own configureServer hook installed when this file's + // createServer() call ran — is what actually proves the fix: before it, + // getAllowedConnectionIds threw "missing module record" for the entry + // module itself on every call, since moduleParsed (a Rollup-build-only + // hook) never fires on a real Vite dev server. + test('Should execute successfully through the real configureServer-installed middleware, walking a real multi-hop import graph', async () => { + // Registers viaHelperFunc in the real backend-function registry — + // configureServer's real middleware looks functions up there, and + // registration is itself a side effect of transforming the file as + // a normal (unsuffixed) frontend import, exactly like a real + // frontend entry point importing the generated client SDK would. + await server.ssrLoadModule(viaHelperFunc.absolutePath); + + const req = createMockRequest('/__dd/executeAction', { + functionName: encodeQueryName(viaHelperFunc), + args: ['real-middleware-value'], + }); + const res = createMockResponse(); + + server.middlewares(req, res, jest.fn()); + await res.done; + + expect(res.statusCode).toBe(200); + const body = JSON.parse(res.getBody()); + expect(body.success).toBe(true); + expect(body.result).toEqual({ data: { value: 'real-middleware-value' } }); + }, 30000); + + // Regression coverage for a real getAllowedConnectionIds wired exactly as + // vite/index.ts's configureServer builds it, on the very first request + // for an entry — no priming import beforehand. The test above still + // primes viaHelperFunc via an unsuffixed ssrLoadModule call first, which + // (before the entryId fix) left a stale, unsuffixed moduleGraph node + // behind that getModuleById happened to find, masking the real gap: on a + // cold entry, Vite only ever registers the node under the fully-resolved + // (suffixed) id handleExecuteAction's own loadModule call just produced, + // and collectModuleGraphFromServer was looking it up by the bare path. + test('Should compute allowed connection IDs on the very first request for an entry, with no prior priming import', async () => { + const loadModule = server.ssrLoadModule.bind(server); + // collectModuleGraphFromServer now appends LOCAL_EXECUTION_LOAD_SUFFIX internally, + // so this closure only ever handles the bare id — matching vite/index.ts's real wiring. + const getAllowedConnectionIds = (entryId: string) => + extractConnectionIdsFromModuleGraph( + entryId, + collectModuleGraphFromServer(server, entryId, FIXTURE_ROOT), + FIXTURE_ROOT, + ); + + const middleware = createDevServerMiddleware( + build, + loadModule, + () => [nestedImportFunc], + getAllowedConnectionIds, + { site: 'datadoghq.com' }, + undefined, + FIXTURE_ROOT, + getMockLogger(), + ); + + const req = createMockRequest('/__dd/executeAction', { + functionName: encodeQueryName(nestedImportFunc), + args: ['cold-entry-value'], + }); + const res = createMockResponse(); + + middleware(req, res, jest.fn()); + await res.done; + + expect(res.statusCode).toBe(200); + const body = JSON.parse(res.getBody()); + expect(body.success).toBe(true); + expect(body.result).toEqual({ data: { value: 'cold-entry-value' } }); + }, 30000); }); diff --git a/packages/plugins/apps/src/vite/dev-server.test.ts b/packages/plugins/apps/src/vite/dev-server.test.ts index a0a2ca9ca..a00c23350 100644 --- a/packages/plugins/apps/src/vite/dev-server.test.ts +++ b/packages/plugins/apps/src/vite/dev-server.test.ts @@ -28,11 +28,18 @@ jest.mock('@dd/core/helpers/oauth-request', () => ({ }), })); +/** + * Shape of the `$.Actions` dynamic proxy — an arbitrarily-nested property + * path (e.g. `$.Actions.slack.chat.postMessage`) that's callable at any + * depth. Used to type `globalThis.$` in tests without an `any` cast. + */ +type ActionsProxy = { [key: string]: ActionsProxy } & ((...args: unknown[]) => Promise); + const mockViteBuild = jest.fn(); /** * Stands in for the real `server.ssrLoadModule` — the local executeAction - * path no longer bundles, so tests exercising it configure this directly + * path doesn't bundle, so tests exercising it configure this directly * instead of `mockBuildWithParsedBackend`. */ const mockLoadModule = jest.fn(); @@ -162,7 +169,8 @@ function mockBuildWithParsedBackend(code = '// code') { * returns for a real backend-function file. */ function mockLoadModuleReturning(func: BackendFunction, fn: (...args: never[]) => unknown) { - mockLoadModule.mockImplementation(moduleResolverFor(func, { [func.name]: fn })); + const resolveModule = moduleResolverFor(func, { [func.name]: fn }); + mockLoadModule.mockImplementation(resolveModule); } describe('Dev Server Middleware', () => { @@ -181,6 +189,7 @@ describe('Dev Server Middleware', () => { mockViteBuild, mockLoadModule, () => mockFunctions, + () => [], mockAuth, getApiKeyRequest(), '/project', @@ -291,6 +300,7 @@ describe('Dev Server Middleware', () => { mockViteBuild, mockLoadModule, () => mockFunctions, + () => [], mockAuth, getApiKeyRequest(), '/project', @@ -368,6 +378,7 @@ describe('Dev Server Middleware', () => { mockViteBuild, mockLoadModule, () => mockFunctions, + () => [], mockAuth, getApiKeyRequest(), '/project', @@ -494,6 +505,7 @@ describe('Dev Server Middleware', () => { mockViteBuild, mockLoadModule, () => mockFunctions, + () => [], mockOauthOnlyAuth, getOAuthRequest(), '/project', @@ -534,6 +546,7 @@ describe('Dev Server Middleware', () => { mockViteBuild, mockLoadModule, () => mockFunctions, + () => [], mockOauthOnlyAuth, undefined, '/project', @@ -622,6 +635,7 @@ describe('Dev Server Middleware', () => { mockViteBuild, mockLoadModule, () => functionsWithAllowlist, + () => [], mockAuth, getApiKeyRequest(), '/project', @@ -783,6 +797,7 @@ describe('Dev Server Middleware', () => { mockViteBuild, mockLoadModule, () => mockFunctions, + () => [], mockAuth, getApiKeyRequest(), '/project', @@ -835,6 +850,7 @@ describe('Dev Server Middleware', () => { mockViteBuild, mockLoadModule, () => mockFunctions, + () => [], mockOauthOnlyAuth, undefined, '/project', @@ -862,13 +878,16 @@ describe('Dev Server Middleware', () => { mockViteBuild, mockLoadModule, () => mockFunctions, + () => [], mockOauthOnlyAuth, undefined, '/project', mockLog, ); mockLoadModuleReturning(mockFunctions[0], () => - (globalThis as Record).$.Actions.slack.chat.postMessage({ + ( + globalThis as typeof globalThis & { $: { Actions: ActionsProxy } } + ).$.Actions.slack.chat.postMessage({ inputs: { text: 'hi' }, }), ); @@ -882,15 +901,32 @@ describe('Dev Server Middleware', () => { noAuthMiddleware(req, res, jest.fn()); await res.done; - expect(res.statusCode).toBe(500); + expect(res.statusCode).toBe(400); const body = JSON.parse(res.getBody()); expect(body.success).toBe(false); expect(body.error).toContain('Auth credentials not configured'); }); test('Should route a real $.Actions call (including connectionId) through a direct single-action preview-async query, not the jsFunctionWithActions wrapper', async () => { - mockLoadModuleReturning(mockFunctions[0], () => - (globalThis as Record).$.Actions.slack.chat.postMessage({ + const funcWithConnection: BackendFunction = { + ...mockFunctions[0], + allowedConnectionIds: ['conn-1'], + }; + const middlewareWithConnection = createDevServerMiddleware( + mockViteBuild, + mockLoadModule, + () => [funcWithConnection, mockFunctions[1]], + (entryId: string) => + entryId === funcWithConnection.absolutePath ? ['conn-1'] : [], + mockAuth, + getApiKeyRequest(), + '/project', + mockLog, + ); + mockLoadModuleReturning(funcWithConnection, () => + ( + globalThis as typeof globalThis & { $: { Actions: ActionsProxy } } + ).$.Actions.slack.chat.postMessage({ inputs: { text: 'hi' }, connectionId: 'conn-1', }), @@ -924,12 +960,12 @@ describe('Dev Server Middleware', () => { }); const req = createMockRequest('/__dd/executeAction', { - functionName: encodeQueryName(mockFunctions[0]), + functionName: encodeQueryName(funcWithConnection), args: [], }); const res = createMockResponse(); - middleware(req, res, jest.fn()); + middlewareWithConnection(req, res, jest.fn()); await res.done; expect(res.statusCode).toBe(200); @@ -951,7 +987,9 @@ describe('Dev Server Middleware', () => { test("Should surface a successful $.Actions call's result to the local console", async () => { mockLoadModuleReturning(mockFunctions[0], () => - (globalThis as Record).$.Actions.slack.chat.postMessage({ + ( + globalThis as typeof globalThis & { $: { Actions: ActionsProxy } } + ).$.Actions.slack.chat.postMessage({ inputs: { text: 'hi' }, }), ); @@ -983,7 +1021,9 @@ describe('Dev Server Middleware', () => { test("Should surface a failed $.Actions call's error detail to the local console", async () => { mockLoadModuleReturning(mockFunctions[0], () => - (globalThis as Record).$.Actions.slack.chat.postMessage({ + ( + globalThis as typeof globalThis & { $: { Actions: ActionsProxy } } + ).$.Actions.slack.chat.postMessage({ inputs: { text: 'hi' }, }), ); @@ -1024,6 +1064,7 @@ describe('Dev Server Middleware', () => { mockViteBuild, mockLoadModule, () => currentFunctions, + () => [], mockAuth, getApiKeyRequest(), '/project', diff --git a/packages/plugins/apps/src/vite/dev-server.ts b/packages/plugins/apps/src/vite/dev-server.ts index a7bcd2670..a3ac6f952 100644 --- a/packages/plugins/apps/src/vite/dev-server.ts +++ b/packages/plugins/apps/src/vite/dev-server.ts @@ -15,6 +15,7 @@ import { encodeQueryName } from '../backend/encodeQueryName'; import type { ExecuteActionRequest, ExecuteActionResponse } from '../backend/protocol'; import type { BackendFunction, BackendOutputs } from '../backend/types'; import { generateDevVirtualEntryContent } from '../backend/virtual-entry'; +import { LOCAL_EXECUTION_LOAD_SUFFIX } from '../constants'; import { createBackendConnectionIdCollector } from './backend-connection-id-collector'; import { getBaseBackendBuildConfig } from './build-config'; @@ -215,22 +216,7 @@ async function executeScriptViaDatadog( return outputs; } -/** - * Build the real `$.Actions` implementation local execution injects: each - * call submits its own direct, single-action `preview-async` query — the - * action's own `{fqn, inputs, connectionId}`, not wrapped in a - * `jsFunctionWithActions` script — and polls it the same way the whole-script - * path does. This is the v1 mechanism decided in the RFC's Decisions and - * Trade-Offs: it needs nothing new from Action Platform and works today. No - * auth check happens until an action call is actually made — a script that - * never calls `$.Actions` runs locally with no auth configured at all. - * - * Logs the resolved result or error detail at info/error level — the - * Telemetry milestone's primary deliverable. Production's own equivalent - * signal only reaches Datadog's backend; a developer watching `npm run dev` - * would otherwise see no result at all for an action call beyond the - * `log.debug` breadcrumbs `submitQuery`/`pollQueryExecution` already emit. - */ +/** Submits a single-action `preview-async` query per `$.Actions` call (no auth needed until a call is actually made) and logs its result/error, since production's own equivalent signal only reaches Datadog's backend, not the developer's `npm run dev` console. */ function makeExecuteActionRemotely( auth: AuthConfig, doAuthenticatedRequest: DoAuthenticatedRequest | undefined, @@ -242,7 +228,7 @@ function makeExecuteActionRemotely( connectionId: string | undefined, ): Promise => { if (!doAuthenticatedRequest) { - throw new Error(`Auth credentials not configured. ${AUTH_GUIDANCE}`); + throw new HttpError(400, `Auth credentials not configured. ${AUTH_GUIDANCE}`); } try { const receiptId = await submitQuery( @@ -308,7 +294,7 @@ async function pollQueryExecution( log.debug(`Long-poll response, done: ${attrs?.done}`); if (attrs?.done) { - if (attrs.outputs === undefined) { + if (attrs.outputs === undefined || attrs.outputs === null) { throw new Error('Query execution completed without outputs'); } return attrs.outputs; @@ -339,14 +325,13 @@ class HttpError extends Error { } /** - * Shared request pipeline: parse body, validate functionName, look up - * the backend function by encoded query name, and bundle it. + * Parse the request body and look up the backend function by encoded query + * name. */ -async function validateAndBundle( +async function parseAndLookupFunction( req: IncomingMessage, functionsByName: Map, - bundle: BundleFn, -): Promise<{ func: BackendFunction; code: string; args: unknown[] }> { +): Promise<{ func: BackendFunction; args: unknown[] }> { const { functionName, args = [] } = await parseRequestBody(req); if (!functionName || typeof functionName !== 'string') { @@ -358,6 +343,19 @@ async function validateAndBundle( throw new HttpError(404, `Backend function "${functionName}" not found`); } + return { func, args }; +} + +/** + * Shared request pipeline: parse body, validate functionName, look up + * the backend function by encoded query name, and bundle it. + */ +async function validateAndBundle( + req: IncomingMessage, + functionsByName: Map, + bundle: BundleFn, +): Promise<{ func: BackendFunction; code: string; args: unknown[] }> { + const { func, args } = await parseAndLookupFunction(req, functionsByName); const bundled = await bundle(func); return { ...bundled, args }; } @@ -384,29 +382,6 @@ async function handleDebugBundle( } } -/** - * Parse the request body and look up the backend function by encoded query - * name — the same validation `validateAndBundle` does, minus the bundle step - * `handleExecuteAction` no longer needs. - */ -async function parseAndLookupFunction( - req: IncomingMessage, - functionsByName: Map, -): Promise<{ func: BackendFunction; args: unknown[] }> { - const { functionName, args = [] } = await parseRequestBody(req); - - if (!functionName || typeof functionName !== 'string') { - throw new HttpError(400, 'Missing or invalid functionName'); - } - - const func = functionsByName.get(functionName); - if (!func) { - throw new HttpError(404, `Backend function "${functionName}" not found`); - } - - return { func, args }; -} - /** * Handle POST /__dd/executeAction — imports a backend function's real file * directly and executes it in-process (see local-execution.ts); no bundling @@ -421,6 +396,8 @@ async function handleExecuteAction( auth: AuthConfig, doAuthenticatedRequest: DoAuthenticatedRequest | undefined, loadModule: LoadModule, + getAllowedConnectionIds: (entryId: string) => string[], + projectRoot: string, log: Logger, ): Promise { try { @@ -429,8 +406,35 @@ async function handleExecuteAction( log.debug(`Executing action locally: ${displayName} with args`); + // The registry's own `func.allowedConnectionIds` is always `[]` here + // — only the bundling collector populates it, and this path + // intentionally skips bundling. Loading the entry once first lets + // the module-graph collector observe Vite's `server.moduleGraph` for + // this entry (see collectModuleGraphFromServer), so the connection-ID + // allowlist reflects the function's actual imports instead of being + // silently empty. + const entrySpecifier = func.absolutePath + LOCAL_EXECUTION_LOAD_SUFFIX; + const primedModule = await loadModule(entrySpecifier); + const funcWithConnectionIds: BackendFunction = { + ...func, + allowedConnectionIds: getAllowedConnectionIds(func.absolutePath), + }; + + // executeScriptLocally loads this same entry specifier again + // internally; reuse the module already resolved above instead of + // making Vite re-run ssrLoadModule for it a second time. + const loadModuleReusingPrimedEntry: LoadModule = (specifier) => + specifier === entrySpecifier ? Promise.resolve(primedModule) : loadModule(specifier); + const executeAction = makeExecuteActionRemotely(auth, doAuthenticatedRequest, log); - const result = await executeScriptLocally(func, args, executeAction, loadModule, log); + const result = await executeScriptLocally( + funcWithConnectionIds, + projectRoot, + args, + executeAction, + loadModuleReusingPrimedEntry, + log, + ); res.statusCode = 200; res.setHeader('Content-Type', 'application/json'); @@ -446,9 +450,10 @@ async function handleExecuteAction( /** * Handle POST /__dd/executeActionViaCloud — bundles a backend function and * executes it via the existing production round trip (queue + Deno - * subprocess). Same behavior as `/__dd/executeAction` before this project: - * kept as a distinctly-purposed command (`npm run dev:verify`, Milestone 3) - * for pre-publish parity checks, not a mode flag on the same endpoint. + * subprocess), the same way `/__dd/executeAction` did before local + * execution existed. Kept as a distinctly-purposed command (`npm run + * dev:verify`, Milestone 3) for pre-publish parity checks, not a mode flag + * on the same endpoint. */ async function handleExecuteActionViaCloud( req: IncomingMessage, @@ -504,6 +509,7 @@ export function createDevServerMiddleware( viteBuild: typeof build, loadModule: LoadModule, getBackendFunctions: () => BackendFunction[], + getAllowedConnectionIds: (entryId: string) => string[], auth: AuthConfig, doAuthenticatedRequest: DoAuthenticatedRequest | undefined, projectRoot: string, @@ -545,6 +551,8 @@ export function createDevServerMiddleware( auth, doAuthenticatedRequest, loadModule, + getAllowedConnectionIds, + projectRoot, log, ).catch(() => { sendError(res, 500, 'Unexpected error'); diff --git a/packages/plugins/apps/src/vite/index.test.ts b/packages/plugins/apps/src/vite/index.test.ts index 2f6e86df0..adced4f6d 100644 --- a/packages/plugins/apps/src/vite/index.test.ts +++ b/packages/plugins/apps/src/vite/index.test.ts @@ -285,8 +285,8 @@ describe('Backend Functions - getVitePlugin', () => { // ESM-only package -- ssr.noExternal is what the local executeAction // path's server.ssrLoadModule call depends on to load them correctly. const plugin = getVitePlugin(defaultOptions); - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const config = (plugin as any).config(); + const configHook = plugin!.config as () => { ssr: { noExternal: string[] } }; + const config = configHook(); expect(config).toEqual({ ssr: { diff --git a/packages/plugins/apps/src/vite/index.ts b/packages/plugins/apps/src/vite/index.ts index b94f446ce..e1e3cf7b3 100644 --- a/packages/plugins/apps/src/vite/index.ts +++ b/packages/plugins/apps/src/vite/index.ts @@ -14,6 +14,7 @@ import { type DoAuthenticatedRequest, } from '../auth'; import { extractExportedFunctions } from '../backend/ast-parsing/extract-backend-functions'; +import { extractConnectionIdsFromModuleGraph } from '../backend/ast-parsing/extract-connection-ids-from-module-graph'; import { encodeQueryName } from '../backend/encodeQueryName'; import { generateProxyModule } from '../backend/proxy-codegen'; import type { BackendFunction } from '../backend/types'; @@ -26,6 +27,7 @@ import { import type { AppsOptionsWithDefaults } from '../types'; import { buildBackendFunctions } from './build-backend-functions'; +import { collectModuleGraphFromServer } from './dev-server-module-graph'; import { createDevServerMiddleware } from './dev-server'; import { handleUpload } from './handle-upload'; @@ -122,6 +124,16 @@ export const getVitePlugin = ({ const { setBackendFunctions, getBackendFunctions } = createBackendFunctionRegistry(); + // Non-backend module IDs reached transitively from a suffixed backend + // entry point. A plain helper module's own id can't carry + // LOCAL_EXECUTION_LOAD_SUFFIX (it's never ambiguous between a proxy and + // real code, so it needs no suffix), but it still needs to be recognized + // as an importer belonging to the suffixed subgraph — otherwise a + // *.backend.ts file reached through it (rather than directly from + // another *.backend.ts file) would lose the marker one hop later than a + // direct backend-to-backend import does. + const suffixedSubgraphImporters = new Set(); + return { // The dev server's local-execution path loads backend-function // dependencies (e.g. @datadog/apps-backend, @datadog/action-catalog) @@ -146,11 +158,14 @@ export const getVitePlugin = ({ // would resolve that nested import unsuffixed, hitting transform's // "not suffixed" branch below and getting replaced with the frontend // RPC-proxy stub — breaking local execution for a multi-backend-file - // import graph. Only propagates when the importer itself was - // suffixed (this is local execution's own module graph, not a - // regular frontend import) and only onto another `.backend.ts` file - // (a plain helper module never hits the proxy-vs-real-code branching - // this marker exists to disambiguate, so it needs no suffix). + // import graph. Propagates whenever the importer itself was suffixed + // OR is a previously-seen non-backend module reached from within the + // suffixed subgraph (see `suffixedSubgraphImporters` above) — this is + // local execution's own module graph either way, not a regular + // frontend import — and only appends the suffix onto another + // `.backend.ts` file (a plain helper module never hits the + // proxy-vs-real-code branching this marker exists to disambiguate, + // so it needs no suffix of its own). resolveId: { // Must run before Vite's own built-in resolver: a plain relative // specifier like `./other.backend` is fully resolvable by Vite's @@ -160,7 +175,19 @@ export const getVitePlugin = ({ // `pre` guarantees this hook gets first look at every id. order: 'pre', async handler(source, importer, resolveOptions) { - if (!importer || !importer.endsWith(LOCAL_EXECUTION_LOAD_SUFFIX)) { + // Scoped to `resolveOptions.ssr`: local execution's own + // traversal is always an SSR resolution (it runs through + // `server.ssrLoadModule`), so a helper's id recorded here + // must only count for a later SSR-context resolution too — + // otherwise the same helper subsequently reached from the + // ordinary (non-SSR) client graph would inherit the marker + // and serve real backend code to the browser instead of the + // frontend RPC-proxy stub. + const isPartOfSuffixedSubgraph = + !!importer && + (importer.endsWith(LOCAL_EXECUTION_LOAD_SUFFIX) || + (resolveOptions.ssr === true && suffixedSubgraphImporters.has(importer))); + if (!isPartOfSuffixedSubgraph) { return null; } @@ -172,10 +199,12 @@ export const getVitePlugin = ({ return resolved; } - if ( - BACKEND_FILE_RE.test(resolved.id) && - !resolved.id.endsWith(LOCAL_EXECUTION_LOAD_SUFFIX) - ) { + if (!BACKEND_FILE_RE.test(resolved.id)) { + suffixedSubgraphImporters.add(resolved.id); + return resolved; + } + + if (!resolved.id.endsWith(LOCAL_EXECUTION_LOAD_SUFFIX)) { return { ...resolved, id: resolved.id + LOCAL_EXECUTION_LOAD_SUFFIX }; } @@ -272,17 +301,32 @@ export const getVitePlugin = ({ } } - server.middlewares.use( - createDevServerMiddleware( - bundler.build, - server.ssrLoadModule.bind(server), - getBackendFunctions, - auth, - doAuthenticatedRequest, + const loadModule = server.ssrLoadModule.bind(server); + // Call only after `loadModule` has resolved for this same entryId (plus its + // LOCAL_EXECUTION_LOAD_SUFFIX) — the graph read below is a live side effect of + // that call, not independently maintained state (moduleParsed, the mechanism the + // production bundling path uses instead, never fires during a real Vite dev + // server — it's a Rollup-build-only hook). collectModuleGraphFromServer owns + // appending the suffix internally, so this closure only ever handles the bare + // backend-file path — the same shape extractConnectionIdsFromModuleGraph needs + // to key into the returned records map. + const getAllowedConnectionIds = (entryId: string) => + extractConnectionIdsFromModuleGraph( + entryId, + collectModuleGraphFromServer(server, entryId, context.buildRoot), context.buildRoot, - log, - ), + ); + const middleware = createDevServerMiddleware( + bundler.build, + loadModule, + getBackendFunctions, + getAllowedConnectionIds, + auth, + doAuthenticatedRequest, + context.buildRoot, + log, ); + server.middlewares.use(middleware); }, }; }; diff --git a/packages/plugins/apps/src/vite/local-execution.test.ts b/packages/plugins/apps/src/vite/local-execution.test.ts index 5a8e32cd0..09b53e73a 100644 --- a/packages/plugins/apps/src/vite/local-execution.test.ts +++ b/packages/plugins/apps/src/vite/local-execution.test.ts @@ -42,6 +42,13 @@ beforeEach(() => { jest.spyOn(shared, 'isDatadogAppsBackendInstalled').mockReturnValue(false); }); +/** + * Shape of the `$.Actions` dynamic proxy — an arbitrarily-nested property + * path (e.g. `$.Actions.slack.chat.postMessage`) that's callable at any + * depth. Used to type `globalThis.$` in tests without an `any` cast. + */ +type ActionsProxy = { [key: string]: ActionsProxy } & ((...args: unknown[]) => Promise); + const stubExecuteAction: ExecuteAction = async (fqn) => ({ data: null, stub: true, fqn }); /** A `loadModule` double that resolves the customer's function from a map and rejects anything else with a module-not-found error, matching the common case where neither optional package is installed. */ @@ -550,7 +557,9 @@ describe('local-execution — executeScriptLocally', () => { slowExecuteAction, loadModuleReturning({ example: () => - (globalThis as Record).$.Actions.slack.chat.postMessage({ + ( + globalThis as typeof globalThis & { $: { Actions: ActionsProxy } } + ).$.Actions.slack.chat.postMessage({ inputs: { text: 'hi' }, }), }), @@ -572,7 +581,9 @@ describe('local-execution — executeScriptLocally', () => { executeAction, loadModuleReturning({ example: async () => { - await (globalThis as Record).$.Actions.slack.chat.postMessage({ + await ( + globalThis as typeof globalThis & { $: { Actions: ActionsProxy } } + ).$.Actions.slack.chat.postMessage({ inputs: { text: 'hi' }, }); // Hangs with no further $.Actions call — the fresh timeout window from the completed call above must still expire normally. diff --git a/packages/published/esbuild-plugin/package.json b/packages/published/esbuild-plugin/package.json index 3047f1f7c..b1d19e619 100644 --- a/packages/published/esbuild-plugin/package.json +++ b/packages/published/esbuild-plugin/package.json @@ -63,6 +63,7 @@ "outdent": "0.8.0", "p-queue": "6.6.2", "pretty-bytes": "5.6.0", + "rollup": "4.45.1", "simple-git": "3.36.0", "unplugin": "2.3.11" }, diff --git a/packages/published/rollup-plugin/package.json b/packages/published/rollup-plugin/package.json index d05a63ae0..810ee006d 100644 --- a/packages/published/rollup-plugin/package.json +++ b/packages/published/rollup-plugin/package.json @@ -66,6 +66,7 @@ "outdent": "0.8.0", "p-queue": "6.6.2", "pretty-bytes": "5.6.0", + "rollup": "4.45.1", "simple-git": "3.36.0", "unplugin": "2.3.11" }, diff --git a/packages/published/rspack-plugin/package.json b/packages/published/rspack-plugin/package.json index 85a14619a..9db8727f6 100644 --- a/packages/published/rspack-plugin/package.json +++ b/packages/published/rspack-plugin/package.json @@ -63,6 +63,7 @@ "outdent": "0.8.0", "p-queue": "6.6.2", "pretty-bytes": "5.6.0", + "rollup": "4.45.1", "simple-git": "3.36.0", "unplugin": "2.3.11" }, diff --git a/packages/published/vite-plugin/package.json b/packages/published/vite-plugin/package.json index 05c805e65..5371a5927 100644 --- a/packages/published/vite-plugin/package.json +++ b/packages/published/vite-plugin/package.json @@ -63,6 +63,7 @@ "outdent": "0.8.0", "p-queue": "6.6.2", "pretty-bytes": "5.6.0", + "rollup": "4.45.1", "simple-git": "3.36.0", "unplugin": "2.3.11" }, diff --git a/packages/published/webpack-plugin/package.json b/packages/published/webpack-plugin/package.json index 148e40ffe..35da8ffbf 100644 --- a/packages/published/webpack-plugin/package.json +++ b/packages/published/webpack-plugin/package.json @@ -63,6 +63,7 @@ "outdent": "0.8.0", "p-queue": "6.6.2", "pretty-bytes": "5.6.0", + "rollup": "4.45.1", "simple-git": "3.36.0", "unplugin": "2.3.11" }, diff --git a/packages/tests/src/_jest/fixtures/apps_backend_project/helper.ts b/packages/tests/src/_jest/fixtures/apps_backend_project/helper.ts new file mode 100644 index 000000000..260ef5b6e --- /dev/null +++ b/packages/tests/src/_jest/fixtures/apps_backend_project/helper.ts @@ -0,0 +1,9 @@ +// Unless explicitly stated otherwise all files in this repository are licensed under the MIT License. +// This product includes software developed at Datadog (https://www.datadoghq.com/). +// Copyright 2019-Present Datadog, Inc. + +import { plainEcho } from './getRuntimeUsers.backend'; + +export async function helperEcho(value: string) { + return plainEcho(value); +} diff --git a/packages/tests/src/_jest/fixtures/apps_backend_project/viaHelper.backend.ts b/packages/tests/src/_jest/fixtures/apps_backend_project/viaHelper.backend.ts new file mode 100644 index 000000000..68e728635 --- /dev/null +++ b/packages/tests/src/_jest/fixtures/apps_backend_project/viaHelper.backend.ts @@ -0,0 +1,9 @@ +// Unless explicitly stated otherwise all files in this repository are licensed under the MIT License. +// This product includes software developed at Datadog (https://www.datadoghq.com/). +// Copyright 2019-Present Datadog, Inc. + +import { helperEcho } from './helper'; + +export async function usesHelper(value: string) { + return helperEcho(value); +} From 92cb344f93217ce230c363becf05d898ba7b141a Mon Sep 17 00:00:00 2001 From: Tiffany Trinh Date: Thu, 27 Aug 2026 18:37:07 -0400 Subject: [PATCH 04/14] fix(tools): externalize deep imports of a bundled dependency correctly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit rollupConfig.mjs's `external` list only matched a dependency's bare specifier exactly, so `rollup/parseAst` (needed by the new dev-server module-graph collector) got bundled instead of externalized despite `rollup` being declared as a dependency. The bundled copy pulls in Rollup's own native-binary platform loader, which throws through `@rollup/plugin-commonjs`'s dynamic-require interop the moment a consumer's real dev server calls it — breaking every published plugin package for real users, not just this repo's own tests. Switched the `external` option to a function that also matches subpath imports (`id === name || id.startsWith(name + '/')`) for every declared dependency and peer dependency. Also drops a stray blank line introduced between a comment and the test it documents in local-execution.test.ts. --- .../src/vite/dev-server.integration.test.ts | 26 +++++--- .../apps/src/vite/local-execution.test.ts | 51 +++++++++++++- .../plugins/apps/src/vite/local-execution.ts | 19 ++++-- packages/tools/src/rollupConfig.mjs | 66 +++++++++++-------- 4 files changed, 121 insertions(+), 41 deletions(-) diff --git a/packages/plugins/apps/src/vite/dev-server.integration.test.ts b/packages/plugins/apps/src/vite/dev-server.integration.test.ts index 814461a43..9bc8db310 100644 --- a/packages/plugins/apps/src/vite/dev-server.integration.test.ts +++ b/packages/plugins/apps/src/vite/dev-server.integration.test.ts @@ -100,6 +100,15 @@ const viaHelperFunc: BackendFunction = { allowedConnectionIds: [], }; +// Never referenced by another test in this file — the cold-entry test below +// needs a module its shared beforeAll server has genuinely never loaded. +const noSdkFunc: BackendFunction = { + relativePath: 'noSdk', + name: 'noSdkFunction', + absolutePath: path.join(FIXTURE_ROOT, 'noSdk.backend.ts'), + allowedConnectionIds: [], +}; + describe('Dev Server Middleware — real end-to-end local execution', () => { let server: ViteDevServer; @@ -272,10 +281,11 @@ describe('Dev Server Middleware — real end-to-end local execution', () => { // Regression coverage for a real getAllowedConnectionIds wired exactly as // vite/index.ts's configureServer builds it, on the very first request - // for an entry — no priming import beforehand. The test above still - // primes viaHelperFunc via an unsuffixed ssrLoadModule call first, which - // (before the entryId fix) left a stale, unsuffixed moduleGraph node - // behind that getModuleById happened to find, masking the real gap: on a + // for an entry — no priming import beforehand. Uses noSdkFunc, which no + // earlier test in this file touches: reusing nestedImportFunc or + // viaHelperFunc here would already have a warm moduleGraph node (and + // module-runner cache) left over from an earlier test against this same + // shared beforeAll server, masking the real gap this test guards — on a // cold entry, Vite only ever registers the node under the fully-resolved // (suffixed) id handleExecuteAction's own loadModule call just produced, // and collectModuleGraphFromServer was looking it up by the bare path. @@ -293,7 +303,7 @@ describe('Dev Server Middleware — real end-to-end local execution', () => { const middleware = createDevServerMiddleware( build, loadModule, - () => [nestedImportFunc], + () => [noSdkFunc], getAllowedConnectionIds, { site: 'datadoghq.com' }, undefined, @@ -302,8 +312,8 @@ describe('Dev Server Middleware — real end-to-end local execution', () => { ); const req = createMockRequest('/__dd/executeAction', { - functionName: encodeQueryName(nestedImportFunc), - args: ['cold-entry-value'], + functionName: encodeQueryName(noSdkFunc), + args: [], }); const res = createMockResponse(); @@ -313,6 +323,6 @@ describe('Dev Server Middleware — real end-to-end local execution', () => { expect(res.statusCode).toBe(200); const body = JSON.parse(res.getBody()); expect(body.success).toBe(true); - expect(body.result).toEqual({ data: { value: 'cold-entry-value' } }); + expect(body.result).toEqual({ data: { ok: true } }); }, 30000); }); diff --git a/packages/plugins/apps/src/vite/local-execution.test.ts b/packages/plugins/apps/src/vite/local-execution.test.ts index 09b53e73a..006fa9598 100644 --- a/packages/plugins/apps/src/vite/local-execution.test.ts +++ b/packages/plugins/apps/src/vite/local-execution.test.ts @@ -570,6 +570,56 @@ describe('local-execution — executeScriptLocally', () => { expect(result).toEqual({ data: { ok: true } }); }); + // Guards against the hang-detection timer staying paused forever: without + // a bound on the $.Actions call itself, a stalled network request (no + // abort signal/deadline of its own) would wedge this execution — and, + // since local executions are serialized via `enqueue`, every request + // queued behind it — indefinitely. + test('Should eventually time out an in-flight $.Actions call that never settles, and not wedge subsequently queued executions', async () => { + jest.useFakeTimers(); + try { + const neverSettlingExecuteAction: ExecuteAction = () => new Promise(() => {}); + + const hungExecution = executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + neverSettlingExecuteAction, + loadModuleReturning({ + example: () => + ( + globalThis as typeof globalThis & { $: { Actions: ActionsProxy } } + ).$.Actions.slack.chat.postMessage({ + inputs: { text: 'hi' }, + }), + }), + mockLogger, + 50, + ); + // Enqueued behind hungExecution — if the fix didn't bound the + // stalled $.Actions call, this would never get a turn either. + const queuedNext = executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + loadModuleReturning({ example: () => 'next' }), + mockLogger, + ); + + const hungAssertion = expect(hungExecution).rejects.toThrow( + /\$\.Actions call to "com\.datadoghq\.slack\.chat\.postMessage" timed out/, + ); + + await jest.runAllTimersAsync(); + await hungAssertion; + + expect(await queuedNext).toEqual({ data: 'next' }); + } finally { + jest.useRealTimers(); + } + }); + test('Should still time out a function that hangs with no $.Actions call in flight, even after an earlier call in the same run completed', async () => { const executeAction: ExecuteAction = async () => ({ ok: true }); @@ -597,7 +647,6 @@ describe('local-execution — executeScriptLocally', () => { }); // Asserts $'s exact key set, since a token added inside globalThis.$ wouldn't be caught by the weaker top-level check below. - test('Should never expose an auth token to the customer module — only backendFunctionArgs, Actions, and Source are visible on globalThis.$', async () => { const result = await executeScriptLocally( func, diff --git a/packages/plugins/apps/src/vite/local-execution.ts b/packages/plugins/apps/src/vite/local-execution.ts index d4a97675a..3e404c825 100644 --- a/packages/plugins/apps/src/vite/local-execution.ts +++ b/packages/plugins/apps/src/vite/local-execution.ts @@ -112,6 +112,9 @@ function isIndexableRecord(value: unknown): value is Record { const DEFAULT_TIMEOUT_MS = 10_000; +/** Bounds a single `$.Actions` call while it's exempt from the hang-detection timer above (see `guardedExecuteAction`). `doRequest` attaches no abort signal or deadline of its own, so an in-flight call that never settles would otherwise wedge this execution — and, since local executions are serialized, every request queued behind it — forever. Set generously past `pollQueryExecution`'s own worst-case long-poll budget (10 retries at up to ~30s each) so a legitimate slow action is never cut off. */ +const MAX_ACTION_CALL_TIMEOUT_MS = 10 * 60_000; + /** Loads a module by specifier, resolved against the customer's own project rather than build-plugins' dependency tree — the dev server passes its Vite instance's `ssrLoadModule` here. */ export type LoadModule = (specifier: string) => Promise>; @@ -203,11 +206,11 @@ function makeActionsProxy( }); } -/** Bounds a registration's `loadModule` call so a load that never settles (a broken/circular module graph) rejects instead of leaving its cache entry pending forever — eviction-on-rejection below only fires once a promise settles. Can't cancel the underlying promise, so a load that eventually settles still runs its side effects late; see the registration functions for why that's harmless. */ -function withTimeout(promise: Promise, timeoutMs: number, what: string): Promise { +/** Bounds a promise that would otherwise be able to hang forever — a registration's underlying `loadModule` call (a broken/circular module graph, not just a slow one), or a `$.Actions` call whose transport attaches no deadline of its own — so it rejects instead of leaving its caller waiting indefinitely. Doesn't cancel the underlying promise (not possible for a plain `Promise`), so it still runs its side effects late if it eventually does settle; see each call site's own doc comment for why that's harmless there. `label` is the full, already-attributed subject of the timeout message (e.g. `` `Loading ${specifier}` ``), not appended to a fixed prefix, so it reads naturally for both a load and an action call. */ +function withTimeout(promise: Promise, timeoutMs: number, label: string): Promise { return new Promise((resolve, reject) => { const timer = setTimeout(() => { - reject(new Error(`Loading ${what} timed out after ${timeoutMs}ms`)); + reject(new Error(`${label} timed out after ${timeoutMs}ms`)); }, timeoutMs); promise.then( (value) => { @@ -270,7 +273,7 @@ async function registerActionCatalogOnce(loadModule: LoadModule, timeoutMs: numb const mod = await withTimeout( loadPromise, timeoutMs, - '@datadog/action-catalog/action-execution', + 'Loading @datadog/action-catalog/action-execution', ); const setExecuteActionImplementation = mod.setExecuteActionImplementation; if (typeof setExecuteActionImplementation !== 'function') { @@ -327,7 +330,7 @@ async function registerBackendRuntimeOnce( const [jsFunctionWithActionsModule, runtimeModule] = await withTimeout( loadPromise, timeoutMs, - '@datadog/apps-backend/runtime', + 'Loading @datadog/apps-backend/runtime', ); const buildRuntimeFromJsFunctionWithActions = jsFunctionWithActionsModule.buildRuntimeFromJsFunctionWithActions; @@ -511,7 +514,11 @@ async function runScriptLocally( pendingActionCalls += 1; clearTimeout(timer); try { - return await executeAction(fqn, inputs, connectionId); + return await withTimeout( + executeAction(fqn, inputs, connectionId), + MAX_ACTION_CALL_TIMEOUT_MS, + `$.Actions call to "${fqn}"`, + ); } finally { pendingActionCalls -= 1; if (pendingActionCalls === 0 && scope.isCurrent()) { diff --git a/packages/tools/src/rollupConfig.mjs b/packages/tools/src/rollupConfig.mjs index fadfd2a5e..04db8dca5 100644 --- a/packages/tools/src/rollupConfig.mjs +++ b/packages/tools/src/rollupConfig.mjs @@ -50,10 +50,8 @@ const BUNDLER_NAME_RX = /^@datadog\/(.+)-plugin$/g; * @param {RollupOptions} config * @returns {RollupOptions} */ -export const bundle = (packageJson, config) => ({ - input: 'src/index.ts', - ...config, - external: [ +export const bundle = (packageJson, config) => { + const externalPackageNames = [ // All peer dependencies are external dependencies. ...Object.keys(packageJson.peerDependencies), // All dependencies are external dependencies. @@ -61,28 +59,44 @@ export const bundle = (packageJson, config) => ({ // These should be internal only and never be anywhere published. '@dd/tools', '@dd/tests', - // We never want to include Node.js built-in modules in the bundle. - ...modulePackage.builtinModules, - ...(config.external || []), - ], - onwarn(warning, warn) { - // Ignore warnings about undefined `this`. - if (warning.code === 'THIS_IS_UNDEFINED') { - return; - } - warn(warning); - }, - plugins: [ - babel({ - babelHelpers: 'bundled', - include: ['src/**/*'], - }), - json(), - commonjs(), - nodeResolve({ preferBuiltins: true }), - ...(config.plugins || []), - ], -}); + ]; + + return { + input: 'src/index.ts', + ...config, + // A plain string in Rollup's `external` array only matches an id + // exactly (see Rollup's own `getIdMatcher`) — it does not also match + // a deep import of that package (e.g. declaring `rollup` external + // doesn't cover `rollup/parseAst`). Once `@rollup/plugin-node-resolve` + // resolves a deep import to its real absolute file path, the + // exact-match check no longer sees the original bare specifier at + // all, so a package's own subpath export would otherwise get inlined + // despite being declared as a dependency specifically so it stays + // external. + external: (id) => + // We never want to include Node.js built-in modules in the bundle. + modulePackage.builtinModules.includes(id) || + externalPackageNames.some((name) => id === name || id.startsWith(`${name}/`)) || + (config.external || []).includes(id), + onwarn(warning, warn) { + // Ignore warnings about undefined `this`. + if (warning.code === 'THIS_IS_UNDEFINED') { + return; + } + warn(warning); + }, + plugins: [ + babel({ + babelHelpers: 'bundled', + include: ['src/**/*'], + }), + json(), + commonjs(), + nodeResolve({ preferBuiltins: true }), + ...(config.plugins || []), + ], + }; +}; /** * Returns the base configuration for the build plugin in the context of this project. From 714e448063d0664ab2a08027f182948846251370 Mon Sep 17 00:00:00 2001 From: Tiffany Trinh Date: Thu, 27 Aug 2026 20:19:05 -0400 Subject: [PATCH 05/14] fix(apps): bound the priming module load in handleExecuteAction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The priming loadModule call (needed to populate the module graph for connection-ID collection before the real execution starts) evaluates the entry's real top-level code and runs before executeScriptLocally installs its own hang-detection timeout — so it had no bound of its own. A customer module with a hanging top-level await would wedge the request forever. Wraps it in the same withTimeout helper the rest of this file already uses, now exported for this cross-module use. --- .../plugins/apps/src/vite/dev-server.test.ts | 37 +++++++++++++++++++ packages/plugins/apps/src/vite/dev-server.ts | 14 +++++-- .../plugins/apps/src/vite/local-execution.ts | 4 +- 3 files changed, 50 insertions(+), 5 deletions(-) diff --git a/packages/plugins/apps/src/vite/dev-server.test.ts b/packages/plugins/apps/src/vite/dev-server.test.ts index a00c23350..46deabada 100644 --- a/packages/plugins/apps/src/vite/dev-server.test.ts +++ b/packages/plugins/apps/src/vite/dev-server.test.ts @@ -1055,6 +1055,43 @@ describe('Dev Server Middleware', () => { 'error', ); }); + + // Guards the priming loadModule call (see handleExecuteAction) — it + // evaluates the entry's real top-level code before + // executeScriptLocally's own hang-detection timeout is installed, so + // a customer module with a hanging top-level await would otherwise + // wedge this request forever with no bound at all. + test('Should eventually time out and return a clear error when the priming load never settles', async () => { + jest.useFakeTimers(); + try { + mockLoadModule.mockImplementation( + () => new Promise(() => {}), // never settles + ); + + const req = createMockRequest('/__dd/executeAction', { + functionName: encodeQueryName(mockFunctions[0]), + args: [], + }); + const res = createMockResponse(); + + middleware(req, res, jest.fn()); + const doneAssertion = res.done; + + // createMockRequest emits the body via a real process.nextTick, + // which fake timers don't advance — drain it first so the + // priming load's own setTimeout is actually scheduled before + // runAllTimersAsync tries to advance past it. + await jest.advanceTimersByTimeAsync(0); + await jest.runAllTimersAsync(); + await doneAssertion; + + expect(res.statusCode).toBe(500); + const body = JSON.parse(res.getBody()); + expect(body.error).toMatch(/timed out after 10000ms/); + } finally { + jest.useRealTimers(); + } + }); }); describe('dynamic discovery', () => { diff --git a/packages/plugins/apps/src/vite/dev-server.ts b/packages/plugins/apps/src/vite/dev-server.ts index a3ac6f952..902827f14 100644 --- a/packages/plugins/apps/src/vite/dev-server.ts +++ b/packages/plugins/apps/src/vite/dev-server.ts @@ -20,7 +20,7 @@ import { LOCAL_EXECUTION_LOAD_SUFFIX } from '../constants'; import { createBackendConnectionIdCollector } from './backend-connection-id-collector'; import { getBaseBackendBuildConfig } from './build-config'; import type { ExecuteAction, LoadModule } from './local-execution'; -import { executeScriptLocally } from './local-execution'; +import { DEFAULT_TIMEOUT_MS, executeScriptLocally, withTimeout } from './local-execution'; interface BundleResult { func: BackendFunction; @@ -412,9 +412,17 @@ async function handleExecuteAction( // the module-graph collector observe Vite's `server.moduleGraph` for // this entry (see collectModuleGraphFromServer), so the connection-ID // allowlist reflects the function's actual imports instead of being - // silently empty. + // silently empty. This priming load runs before executeScriptLocally + // installs its own hang-detection timeout below, and it evaluates the + // entry's real top-level code (ssrLoadModule, not a parse-only step) + // — so it needs its own bound, or a customer module with a hanging + // top-level await would wedge this request forever. const entrySpecifier = func.absolutePath + LOCAL_EXECUTION_LOAD_SUFFIX; - const primedModule = await loadModule(entrySpecifier); + const primedModule = await withTimeout( + loadModule(entrySpecifier), + DEFAULT_TIMEOUT_MS, + `Loading "${displayName}"`, + ); const funcWithConnectionIds: BackendFunction = { ...func, allowedConnectionIds: getAllowedConnectionIds(func.absolutePath), diff --git a/packages/plugins/apps/src/vite/local-execution.ts b/packages/plugins/apps/src/vite/local-execution.ts index 3e404c825..eb703cf58 100644 --- a/packages/plugins/apps/src/vite/local-execution.ts +++ b/packages/plugins/apps/src/vite/local-execution.ts @@ -110,7 +110,7 @@ function isIndexableRecord(value: unknown): value is Record { return typeof value === 'object' && value !== null; } -const DEFAULT_TIMEOUT_MS = 10_000; +export const DEFAULT_TIMEOUT_MS = 10_000; /** Bounds a single `$.Actions` call while it's exempt from the hang-detection timer above (see `guardedExecuteAction`). `doRequest` attaches no abort signal or deadline of its own, so an in-flight call that never settles would otherwise wedge this execution — and, since local executions are serialized, every request queued behind it — forever. Set generously past `pollQueryExecution`'s own worst-case long-poll budget (10 retries at up to ~30s each) so a legitimate slow action is never cut off. */ const MAX_ACTION_CALL_TIMEOUT_MS = 10 * 60_000; @@ -207,7 +207,7 @@ function makeActionsProxy( } /** Bounds a promise that would otherwise be able to hang forever — a registration's underlying `loadModule` call (a broken/circular module graph, not just a slow one), or a `$.Actions` call whose transport attaches no deadline of its own — so it rejects instead of leaving its caller waiting indefinitely. Doesn't cancel the underlying promise (not possible for a plain `Promise`), so it still runs its side effects late if it eventually does settle; see each call site's own doc comment for why that's harmless there. `label` is the full, already-attributed subject of the timeout message (e.g. `` `Loading ${specifier}` ``), not appended to a fixed prefix, so it reads naturally for both a load and an action call. */ -function withTimeout(promise: Promise, timeoutMs: number, label: string): Promise { +export function withTimeout(promise: Promise, timeoutMs: number, label: string): Promise { return new Promise((resolve, reject) => { const timer = setTimeout(() => { reject(new Error(`${label} timed out after ${timeoutMs}ms`)); From a50a7f6f6f06c11bc5d82d8ffe9b94e6d0f3cadb Mon Sep 17 00:00:00 2001 From: Tiffany Trinh Date: Thu, 27 Aug 2026 21:18:41 -0400 Subject: [PATCH 06/14] fix(apps): collect connection-ID-scoped module graph from real dev-server output The SSR transform Vite actually runs for a dev-server-only load rewrites every import into a __vite_ssr_import__(...) call and resolves specifiers to absolute paths, which the plain-ImportDeclaration AST search built for a real Rollup build can't parse. Read each module's original source from disk and strip TS/JSX with esbuild in isolation instead, so the parser sees the same untransformed import syntax the production build path already trusts. --- packages/plugins/apps/package.json | 1 + .../src/backend/ast-parsing/module-graph.ts | 3 +- .../apps/src/vite/dev-server-module-graph.ts | 94 +++++++--- .../src/vite/dev-server.integration.test.ts | 174 ++++++++++++++++-- .../plugins/apps/src/vite/dev-server.test.ts | 62 +++++-- packages/plugins/apps/src/vite/dev-server.ts | 13 +- packages/plugins/apps/src/vite/index.ts | 4 +- .../published/esbuild-plugin/package.json | 1 + packages/published/rollup-plugin/package.json | 1 + packages/published/rspack-plugin/package.json | 1 + packages/published/vite-plugin/package.json | 1 + .../published/webpack-plugin/package.json | 1 + .../action-execution.js | 13 ++ .../fixtures/action_catalog_project/index.js | 13 ++ .../action_catalog_project/package.json | 14 ++ .../actionCatalogCall.backend.ts | 9 + .../mixedImports.backend.ts | 19 ++ .../apps_backend_project/package.json | 1 + .../tests/src/_jest/fixtures/package.json | 3 +- packages/tests/src/_jest/fixtures/yarn.lock | 13 ++ yarn.lock | 1 + 21 files changed, 385 insertions(+), 57 deletions(-) create mode 100644 packages/tests/src/_jest/fixtures/action_catalog_project/action-execution.js create mode 100644 packages/tests/src/_jest/fixtures/action_catalog_project/index.js create mode 100644 packages/tests/src/_jest/fixtures/action_catalog_project/package.json create mode 100644 packages/tests/src/_jest/fixtures/apps_backend_project/actionCatalogCall.backend.ts create mode 100644 packages/tests/src/_jest/fixtures/apps_backend_project/mixedImports.backend.ts diff --git a/packages/plugins/apps/package.json b/packages/plugins/apps/package.json index 0470a0e08..f648cef15 100644 --- a/packages/plugins/apps/package.json +++ b/packages/plugins/apps/package.json @@ -32,6 +32,7 @@ "dependencies": { "@dd/core": "workspace:*", "chalk": "2.3.1", + "esbuild": "0.25.8", "eslint-scope": "7.2.2", "glob": "11.1.0", "jszip": "3.10.1", diff --git a/packages/plugins/apps/src/backend/ast-parsing/module-graph.ts b/packages/plugins/apps/src/backend/ast-parsing/module-graph.ts index 171f90066..b3621c8b2 100644 --- a/packages/plugins/apps/src/backend/ast-parsing/module-graph.ts +++ b/packages/plugins/apps/src/backend/ast-parsing/module-graph.ts @@ -190,7 +190,8 @@ function collectStaticModuleDependencies( })); } -function getStaticModuleSources(ast: Program): string[] { +// Exported so callers that must derive dependency ids by resolving each static specifier individually (the dev server has no build-time Rollup ModuleInfo to read them from) extract the exact same specifier list this module zips them against — not a second, independently-written AST walk that could drift from this one. +export function getStaticModuleSources(ast: Program): string[] { return ast.body.flatMap((node) => { if ( (node.type === 'ImportDeclaration' || diff --git a/packages/plugins/apps/src/vite/dev-server-module-graph.ts b/packages/plugins/apps/src/vite/dev-server-module-graph.ts index 3a8a29f3b..c567aeed0 100644 --- a/packages/plugins/apps/src/vite/dev-server-module-graph.ts +++ b/packages/plugins/apps/src/vite/dev-server-module-graph.ts @@ -2,11 +2,16 @@ // This product includes software developed at Datadog (https://www.datadoghq.com/). // Copyright 2019-Present Datadog, Inc. +/* eslint-disable no-await-in-loop */ + +import { transform } from 'esbuild'; +import { readFile } from 'node:fs/promises'; import { parseAst } from 'rollup/parseAst'; import type { ModuleNode, ViteDevServer } from 'vite'; import { createParsedModuleRecord, + getStaticModuleSources, type ParsedModuleRecord, shouldTraverseCollectedModule, unsupportedModuleGraphDependency, @@ -25,6 +30,17 @@ import { LOCAL_EXECUTION_LOAD_SUFFIX } from '../constants'; * imported module's own `importedModules` — already reflect the full * transitive static-import graph, recursively, with no extra ticks needed. * + * Parses each module's own source read fresh from disk (via `ModuleNode.file`), + * stripped of TS/JSX syntax by `esbuild.transform` in isolation, rather than + * either of Vite's own transform results: the client transform + * (`transformResult`) doesn't run for an SSR-only load, and the SSR transform + * (`ssrTransformResult`) rewrites every `import` into a `__vite_ssr_import__(...)` + * call and resolves bare specifiers to absolute paths — neither shape + * `collectActionCatalogImports`'s plain-`ImportDeclaration` search (built for + * the untransformed syntax a real Rollup build sees) can parse. `esbuild.transform` + * in isolation only strips types/JSX; it doesn't touch import specifiers at all, + * so this reads exactly the same syntax the production build path already trusts. + * * Call this only after `loadModule` has resolved for `bareEntryId + * LOCAL_EXECUTION_LOAD_SUFFIX` in the same request — the graph it reads is a * live side effect of that call, not independently maintained state. @@ -34,11 +50,11 @@ import { LOCAL_EXECUTION_LOAD_SUFFIX } from '../constants'; * node it just loaded by the full resolved id (suffix included), since it * treats each distinct query string as a logically distinct module. */ -export function collectModuleGraphFromServer( +export async function collectModuleGraphFromServer( server: ViteDevServer, bareEntryId: string, buildRoot: string, -): ReadonlyMap { +): Promise> { const records = new Map(); const visited = new Set(); const pending: ModuleNode[] = []; @@ -51,7 +67,7 @@ export function collectModuleGraphFromServer( while (pending.length > 0) { const node = pending.shift()!; const moduleId = node.id ? normalizeViteModuleId(node.id) : undefined; - if (!moduleId || visited.has(moduleId)) { + if (!moduleId || visited.has(moduleId) || !node.file) { continue; } visited.add(moduleId); @@ -60,19 +76,24 @@ export function collectModuleGraphFromServer( continue; } - // `transformResult` is the client/browser transform; SSR loads (what - // local execution always is, via server.ssrLoadModule) populate - // `ssrTransformResult` instead. Neither exists yet if Vite hasn't - // transformed this module — a local dependency the caller's own - // ssrLoadModule call never actually reached. - const transformResult = node.ssrTransformResult ?? node.transformResult; - if (typeof transformResult?.code !== 'string') { - continue; + let source: string; + try { + source = await readFile(node.file, 'utf-8'); + } catch (error) { + const reason = error instanceof Error ? error.message : String(error); + throw unsupportedModuleGraphDependency( + moduleId, + `unreadable module source (${reason})`, + ); } let ast; try { - ast = parseAst(transformResult.code); + const stripped = await transform(source, { + loader: loaderForModuleId(moduleId), + format: 'esm', + }); + ast = parseAst(stripped.code); } catch (error) { const reason = error instanceof Error ? error.message : String(error); throw unsupportedModuleGraphDependency( @@ -81,30 +102,55 @@ export function collectModuleGraphFromServer( ); } - // `deps` are this module's own static imports, already resolved to - // real module ids by Vite's import-analysis plugin — the SSR - // equivalent of `moduleParsed`'s `importedIds`/ - // `importedIdResolutions`. `dynamicDeps` (deliberately unused here) - // folds in `import()` calls; `module-graph.ts`'s own AST walk is what - // flags those as unsupported, so only static deps belong here. - const staticDependencyIds = (transformResult.deps ?? []).map(normalizeViteModuleId); + // `node.importedModules` mixes static AND dynamic imports with no + // documented ordering guarantee, but `createParsedModuleRecord` + // zips dependency ids positionally against the AST's own static + // import/export declarations — so this list must contain ONLY + // static dependencies, in that same order. The dev server's plugin + // container doesn't populate Rollup-style `ModuleInfo.importedIds` + // outside a real build, so this resolves each of the AST's own + // static specifiers individually instead, via the same resolution + // Vite itself would use — guaranteeing a correct 1:1 correspondence + // by construction, since both sides are derived from this same ast. + const staticModuleSources = getStaticModuleSources(ast); + const importerFile = node.file; + const resolutions = await Promise.all( + staticModuleSources.map((moduleSource) => + server.pluginContainer.resolveId(moduleSource, importerFile ?? undefined, { + ssr: true, + }), + ), + ); + const staticDependencyIds = resolutions.map((resolved, index) => + resolved ? normalizeViteModuleId(resolved.id) : staticModuleSources[index], + ); const record = createParsedModuleRecord(moduleId, buildRoot, ast, staticDependencyIds); if (record) { records.set(record.id, record); } - for (const dependencyId of staticDependencyIds) { - const dependencyNode = server.moduleGraph.getModuleById(dependencyId); - if (dependencyNode) { - pending.push(dependencyNode); - } + for (const dependencyNode of node.importedModules) { + pending.push(dependencyNode); } } return records; } +function loaderForModuleId(moduleId: string): 'ts' | 'tsx' | 'jsx' | 'js' { + if (moduleId.endsWith('.tsx')) { + return 'tsx'; + } + if (moduleId.endsWith('.ts') || moduleId.endsWith('.mts') || moduleId.endsWith('.cts')) { + return 'ts'; + } + if (moduleId.endsWith('.jsx')) { + return 'jsx'; + } + return 'js'; +} + function normalizeViteModuleId(id: string): string { return id.split('?')[0]; } diff --git a/packages/plugins/apps/src/vite/dev-server.integration.test.ts b/packages/plugins/apps/src/vite/dev-server.integration.test.ts index 9bc8db310..d526db5ea 100644 --- a/packages/plugins/apps/src/vite/dev-server.integration.test.ts +++ b/packages/plugins/apps/src/vite/dev-server.integration.test.ts @@ -17,24 +17,23 @@ * `LOCAL_EXECUTION_LOAD_SUFFIX` propagation against Vite's own real module * resolution, which a mocked `this.resolve()` can't reproduce. * - * Uses `@datadog/apps-backend` (the fixture already has it as a real, - * locally-resolvable dependency — see `packages/tests/src/_jest/fixtures/ - * node_modules/@datadog/apps-backend`) rather than `@datadog/action-catalog` - * (no equivalent local fixture package exists yet for it). - * `local-execution.test.ts` already separately proves a raw - * `$.Actions.foo.bar(...)` call and an action-catalog typed-wrapper call — - * which reduce to the same injected `executeAction` under the hood — route - * correctly. Building a real local `@datadog/action-catalog` fixture package - * is a reasonable, cheap follow-up, not required for this coverage to be - * meaningful. + * `@datadog/apps-backend` and `@datadog/action-catalog` are both real, + * locally-resolvable fixture packages — see `packages/tests/src/_jest/ + * fixtures/node_modules/@datadog/apps-backend` and `.../@datadog/action-catalog` + * — rather than mocked modules, so the connection-ID coverage below exercises + * Vite's actual SSR transform output, not a hand-crafted AST shape a real + * transform would never produce. */ +import { getAuthenticatedRequest } from '@dd/apps-plugin/auth'; import { collectModuleGraphFromServer } from '@dd/apps-plugin/vite/dev-server-module-graph'; import { createDevServerMiddleware } from '@dd/apps-plugin/vite/dev-server'; import { getVitePlugin } from '@dd/apps-plugin/vite/index'; +import type { AuthOptionsWithDefaults } from '@dd/core/types'; import { getContextMock, getMockLogger } from '@dd/tests/_jest/helpers/mocks'; import { EventEmitter } from 'events'; import type { IncomingMessage, ServerResponse } from 'http'; +import nock from 'nock'; import path from 'path'; import { build, createServer, type Plugin, type ViteDevServer } from 'vite'; @@ -109,6 +108,20 @@ const noSdkFunc: BackendFunction = { allowedConnectionIds: [], }; +const actionCatalogCallFunc: BackendFunction = { + relativePath: 'actionCatalogCall', + name: 'postMessage', + absolutePath: path.join(FIXTURE_ROOT, 'actionCatalogCall.backend.ts'), + allowedConnectionIds: [], +}; + +const mixedImportsFunc: BackendFunction = { + relativePath: 'mixedImports', + name: 'usesMixedImports', + absolutePath: path.join(FIXTURE_ROOT, 'mixedImports.backend.ts'), + allowedConnectionIds: [], +}; + describe('Dev Server Middleware — real end-to-end local execution', () => { let server: ViteDevServer; @@ -132,6 +145,13 @@ describe('Dev Server Middleware — real end-to-end local execution', () => { logLevel: 'silent', server: { middlewareMode: true, hmr: false }, plugins: [appsPlugin], + // Local execution only ever goes through ssrLoadModule, never the + // client bundle — Vite's auto-crawl-and-pre-bundle step exists for + // the browser path this feature never uses, and a fixture-only + // dependency (like the fake @datadog/action-catalog package below) + // can trip it up in ways that have nothing to do with what this + // suite is actually testing. + optimizeDeps: { noDiscovery: true }, }); }); @@ -144,7 +164,7 @@ describe('Dev Server Middleware — real end-to-end local execution', () => { build, server.ssrLoadModule.bind(server), () => [getRuntimeUsersFunc], - () => [], + async () => [], { site: 'datadoghq.com' }, // No auth configured — this function never calls $.Actions. undefined, @@ -186,7 +206,7 @@ describe('Dev Server Middleware — real end-to-end local execution', () => { build, server.ssrLoadModule.bind(server), () => [nestedImportFunc], - () => [], + async () => [], { site: 'datadoghq.com' }, undefined, FIXTURE_ROOT, @@ -223,7 +243,7 @@ describe('Dev Server Middleware — real end-to-end local execution', () => { build, server.ssrLoadModule.bind(server), () => [viaHelperFunc], - () => [], + async () => [], { site: 'datadoghq.com' }, undefined, FIXTURE_ROOT, @@ -293,10 +313,10 @@ describe('Dev Server Middleware — real end-to-end local execution', () => { const loadModule = server.ssrLoadModule.bind(server); // collectModuleGraphFromServer now appends LOCAL_EXECUTION_LOAD_SUFFIX internally, // so this closure only ever handles the bare id — matching vite/index.ts's real wiring. - const getAllowedConnectionIds = (entryId: string) => + const getAllowedConnectionIds = async (entryId: string) => extractConnectionIdsFromModuleGraph( entryId, - collectModuleGraphFromServer(server, entryId, FIXTURE_ROOT), + await collectModuleGraphFromServer(server, entryId, FIXTURE_ROOT), FIXTURE_ROOT, ); @@ -325,4 +345,128 @@ describe('Dev Server Middleware — real end-to-end local execution', () => { expect(body.success).toBe(true); expect(body.result).toEqual({ data: { ok: true } }); }, 30000); + + // Regression coverage for the connection-ID collector against a REAL + // Vite SSR transform, not a hand-crafted AST fixture. Vite's SSR + // transform rewrites every `import` into a `__vite_ssr_import__(...)` + // call and resolves bare specifiers to absolute paths — neither shape + // the plain-`ImportDeclaration` search in collectActionCatalogImports + // (built for the untransformed syntax a real Rollup build sees) can + // parse. Before collectModuleGraphFromServer started reading each + // module's original source fresh from disk instead, this collector + // silently returned an empty allowlist for every locally-executed + // function that imports a typed action-catalog function — rejecting any + // real connectionId-scoped call with "not in this function's allowed + // connections: []", not because of a real access violation, but because + // the collector could never see the import that should have allowed it. + test('Should recognize a connectionId-scoped action-catalog call and allow it, not silently reject it', async () => { + const loadModule = server.ssrLoadModule.bind(server); + const getAllowedConnectionIds = async (entryId: string) => + extractConnectionIdsFromModuleGraph( + entryId, + await collectModuleGraphFromServer(server, entryId, FIXTURE_ROOT), + FIXTURE_ROOT, + ); + + const auth: AuthOptionsWithDefaults = { + apiKey: 'test-api-key', + appKey: 'test-app-key', + site: 'datadoghq.com', + }; + const middleware = createDevServerMiddleware( + build, + loadModule, + () => [actionCatalogCallFunc], + getAllowedConnectionIds, + auth, + getAuthenticatedRequest('apiKey', auth, getMockLogger()), + FIXTURE_ROOT, + getMockLogger(), + ); + + // The connection-ID collector is the thing under test here, not the + // real preview-async round trip (already covered by dev-server.test.ts + // and local-execution.test.ts) — this only needs the request to get + // past the allowedConnectionIds check, so a minimal reply is enough. + const apiScope = nock('https://api.datadoghq.com') + .post('/api/v2/app-builder/queries/preview-async') + .reply(200, { data: { id: 'receipt-action-catalog' } }) + .get('/api/v2/app-builder/queries/execution-long-polling/receipt-action-catalog') + .reply(200, { data: { attributes: { done: true, outputs: { ok: true } } } }); + + const req = createMockRequest('/__dd/executeAction', { + functionName: encodeQueryName(actionCatalogCallFunc), + args: [], + }); + const res = createMockResponse(); + + middleware(req, res, jest.fn()); + await res.done; + + expect(res.statusCode).toBe(200); + const body = JSON.parse(res.getBody()); + expect(body.success).toBe(true); + expect(body.result).toEqual({ data: { ok: true } }); + expect(apiScope.isDone()).toBe(true); + }, 30000); + + // Coverage for a module mixing a static import with a top-level dynamic + // import textually between two static ones. dev-server-module-graph.ts + // now resolves each static specifier individually against the AST + // instead of reading node.importedModules positionally (which mixes + // static and dynamic imports with no documented ordering guarantee) — + // this fixture exercises that resolution path directly. Note: verified + // against a real Vite dev server that node.importedModules for an + // SSR-loaded module doesn't actually include a non-local dynamic + // import's target at all, so the specific silent-misattribution failure + // this was meant to reproduce doesn't manifest on the old code either; + // the new resolution approach is kept regardless since it's correct by + // construction rather than relying on node.importedModules' undocumented + // behavior. + test('Should recognize a connectionId-scoped action-catalog call even when a top-level dynamic import sits between two static imports', async () => { + const loadModule = server.ssrLoadModule.bind(server); + const getAllowedConnectionIds = async (entryId: string) => + extractConnectionIdsFromModuleGraph( + entryId, + await collectModuleGraphFromServer(server, entryId, FIXTURE_ROOT), + FIXTURE_ROOT, + ); + + const auth: AuthOptionsWithDefaults = { + apiKey: 'test-api-key', + appKey: 'test-app-key', + site: 'datadoghq.com', + }; + const middleware = createDevServerMiddleware( + build, + loadModule, + () => [mixedImportsFunc], + getAllowedConnectionIds, + auth, + getAuthenticatedRequest('apiKey', auth, getMockLogger()), + FIXTURE_ROOT, + getMockLogger(), + ); + + const apiScope = nock('https://api.datadoghq.com') + .post('/api/v2/app-builder/queries/preview-async') + .reply(200, { data: { id: 'receipt-mixed-imports' } }) + .get('/api/v2/app-builder/queries/execution-long-polling/receipt-mixed-imports') + .reply(200, { data: { attributes: { done: true, outputs: { ok: true } } } }); + + const req = createMockRequest('/__dd/executeAction', { + functionName: encodeQueryName(mixedImportsFunc), + args: ['hello'], + }); + const res = createMockResponse(); + + middleware(req, res, jest.fn()); + await res.done; + + expect(res.statusCode).toBe(200); + const body = JSON.parse(res.getBody()); + expect(body.success).toBe(true); + expect(body.result).toEqual({ data: { ok: true } }); + expect(apiScope.isDone()).toBe(true); + }, 30000); }); diff --git a/packages/plugins/apps/src/vite/dev-server.test.ts b/packages/plugins/apps/src/vite/dev-server.test.ts index 46deabada..b926d4fd1 100644 --- a/packages/plugins/apps/src/vite/dev-server.test.ts +++ b/packages/plugins/apps/src/vite/dev-server.test.ts @@ -189,7 +189,7 @@ describe('Dev Server Middleware', () => { mockViteBuild, mockLoadModule, () => mockFunctions, - () => [], + async () => [], mockAuth, getApiKeyRequest(), '/project', @@ -300,7 +300,7 @@ describe('Dev Server Middleware', () => { mockViteBuild, mockLoadModule, () => mockFunctions, - () => [], + async () => [], mockAuth, getApiKeyRequest(), '/project', @@ -378,7 +378,7 @@ describe('Dev Server Middleware', () => { mockViteBuild, mockLoadModule, () => mockFunctions, - () => [], + async () => [], mockAuth, getApiKeyRequest(), '/project', @@ -505,7 +505,7 @@ describe('Dev Server Middleware', () => { mockViteBuild, mockLoadModule, () => mockFunctions, - () => [], + async () => [], mockOauthOnlyAuth, getOAuthRequest(), '/project', @@ -546,7 +546,7 @@ describe('Dev Server Middleware', () => { mockViteBuild, mockLoadModule, () => mockFunctions, - () => [], + async () => [], mockOauthOnlyAuth, undefined, '/project', @@ -635,7 +635,7 @@ describe('Dev Server Middleware', () => { mockViteBuild, mockLoadModule, () => functionsWithAllowlist, - () => [], + async () => [], mockAuth, getApiKeyRequest(), '/project', @@ -797,7 +797,7 @@ describe('Dev Server Middleware', () => { mockViteBuild, mockLoadModule, () => mockFunctions, - () => [], + async () => [], mockAuth, getApiKeyRequest(), '/project', @@ -850,7 +850,7 @@ describe('Dev Server Middleware', () => { mockViteBuild, mockLoadModule, () => mockFunctions, - () => [], + async () => [], mockOauthOnlyAuth, undefined, '/project', @@ -878,7 +878,7 @@ describe('Dev Server Middleware', () => { mockViteBuild, mockLoadModule, () => mockFunctions, - () => [], + async () => [], mockOauthOnlyAuth, undefined, '/project', @@ -916,7 +916,7 @@ describe('Dev Server Middleware', () => { mockViteBuild, mockLoadModule, () => [funcWithConnection, mockFunctions[1]], - (entryId: string) => + async (entryId: string) => entryId === funcWithConnection.absolutePath ? ['conn-1'] : [], mockAuth, getApiKeyRequest(), @@ -1092,6 +1092,46 @@ describe('Dev Server Middleware', () => { jest.useRealTimers(); } }); + + // Guards getAllowedConnectionIds — it reads every reachable module + // from disk and transforms it (dev-server-module-graph.ts), with no + // bound of its own, unlike the sibling priming load right next to it + // in handleExecuteAction which already has one. + test('Should eventually time out and return a clear error when getAllowedConnectionIds never settles', async () => { + jest.useFakeTimers(); + try { + mockLoadModuleReturning(mockFunctions[0], () => 'done'); + const hangingMiddleware = createDevServerMiddleware( + mockViteBuild, + mockLoadModule, + () => mockFunctions, + () => new Promise(() => {}), // never settles + mockAuth, + getApiKeyRequest(), + '/project', + mockLog, + ); + + const req = createMockRequest('/__dd/executeAction', { + functionName: encodeQueryName(mockFunctions[0]), + args: [], + }); + const res = createMockResponse(); + + hangingMiddleware(req, res, jest.fn()); + const doneAssertion = res.done; + + await jest.advanceTimersByTimeAsync(0); + await jest.runAllTimersAsync(); + await doneAssertion; + + expect(res.statusCode).toBe(500); + const body = JSON.parse(res.getBody()); + expect(body.error).toMatch(/timed out after 10000ms/); + } finally { + jest.useRealTimers(); + } + }); }); describe('dynamic discovery', () => { @@ -1101,7 +1141,7 @@ describe('Dev Server Middleware', () => { mockViteBuild, mockLoadModule, () => currentFunctions, - () => [], + async () => [], mockAuth, getApiKeyRequest(), '/project', diff --git a/packages/plugins/apps/src/vite/dev-server.ts b/packages/plugins/apps/src/vite/dev-server.ts index 902827f14..ac0213de9 100644 --- a/packages/plugins/apps/src/vite/dev-server.ts +++ b/packages/plugins/apps/src/vite/dev-server.ts @@ -396,7 +396,7 @@ async function handleExecuteAction( auth: AuthConfig, doAuthenticatedRequest: DoAuthenticatedRequest | undefined, loadModule: LoadModule, - getAllowedConnectionIds: (entryId: string) => string[], + getAllowedConnectionIds: (entryId: string) => Promise, projectRoot: string, log: Logger, ): Promise { @@ -423,9 +423,16 @@ async function handleExecuteAction( DEFAULT_TIMEOUT_MS, `Loading "${displayName}"`, ); + // Same reasoning as the priming load above: this reads every reachable module from disk and + // transforms it, with no bound of its own — a stalled file read or a hung esbuild.transform + // call would otherwise wedge this request forever. const funcWithConnectionIds: BackendFunction = { ...func, - allowedConnectionIds: getAllowedConnectionIds(func.absolutePath), + allowedConnectionIds: await withTimeout( + getAllowedConnectionIds(func.absolutePath), + DEFAULT_TIMEOUT_MS, + `Resolving allowed connections for "${displayName}"`, + ), }; // executeScriptLocally loads this same entry specifier again @@ -517,7 +524,7 @@ export function createDevServerMiddleware( viteBuild: typeof build, loadModule: LoadModule, getBackendFunctions: () => BackendFunction[], - getAllowedConnectionIds: (entryId: string) => string[], + getAllowedConnectionIds: (entryId: string) => Promise, auth: AuthConfig, doAuthenticatedRequest: DoAuthenticatedRequest | undefined, projectRoot: string, diff --git a/packages/plugins/apps/src/vite/index.ts b/packages/plugins/apps/src/vite/index.ts index e1e3cf7b3..8080e131e 100644 --- a/packages/plugins/apps/src/vite/index.ts +++ b/packages/plugins/apps/src/vite/index.ts @@ -310,10 +310,10 @@ export const getVitePlugin = ({ // appending the suffix internally, so this closure only ever handles the bare // backend-file path — the same shape extractConnectionIdsFromModuleGraph needs // to key into the returned records map. - const getAllowedConnectionIds = (entryId: string) => + const getAllowedConnectionIds = async (entryId: string) => extractConnectionIdsFromModuleGraph( entryId, - collectModuleGraphFromServer(server, entryId, context.buildRoot), + await collectModuleGraphFromServer(server, entryId, context.buildRoot), context.buildRoot, ); const middleware = createDevServerMiddleware( diff --git a/packages/published/esbuild-plugin/package.json b/packages/published/esbuild-plugin/package.json index b1d19e619..ae6a471dc 100644 --- a/packages/published/esbuild-plugin/package.json +++ b/packages/published/esbuild-plugin/package.json @@ -54,6 +54,7 @@ "@jridgewell/remapping": "2.3.5", "async-retry": "1.3.3", "chalk": "2.3.1", + "esbuild": "0.25.8", "eslint-scope": "7.2.2", "glob": "11.1.0", "json-stream-stringify": "3.1.6", diff --git a/packages/published/rollup-plugin/package.json b/packages/published/rollup-plugin/package.json index 810ee006d..14dc73e79 100644 --- a/packages/published/rollup-plugin/package.json +++ b/packages/published/rollup-plugin/package.json @@ -57,6 +57,7 @@ "@jridgewell/remapping": "2.3.5", "async-retry": "1.3.3", "chalk": "2.3.1", + "esbuild": "0.25.8", "eslint-scope": "7.2.2", "glob": "11.1.0", "json-stream-stringify": "3.1.6", diff --git a/packages/published/rspack-plugin/package.json b/packages/published/rspack-plugin/package.json index 9db8727f6..f8888e587 100644 --- a/packages/published/rspack-plugin/package.json +++ b/packages/published/rspack-plugin/package.json @@ -54,6 +54,7 @@ "@jridgewell/remapping": "2.3.5", "async-retry": "1.3.3", "chalk": "2.3.1", + "esbuild": "0.25.8", "eslint-scope": "7.2.2", "glob": "11.1.0", "json-stream-stringify": "3.1.6", diff --git a/packages/published/vite-plugin/package.json b/packages/published/vite-plugin/package.json index 5371a5927..3b11402a6 100644 --- a/packages/published/vite-plugin/package.json +++ b/packages/published/vite-plugin/package.json @@ -54,6 +54,7 @@ "@jridgewell/remapping": "2.3.5", "async-retry": "1.3.3", "chalk": "2.3.1", + "esbuild": "0.25.8", "eslint-scope": "7.2.2", "glob": "11.1.0", "json-stream-stringify": "3.1.6", diff --git a/packages/published/webpack-plugin/package.json b/packages/published/webpack-plugin/package.json index 35da8ffbf..0455d4dc2 100644 --- a/packages/published/webpack-plugin/package.json +++ b/packages/published/webpack-plugin/package.json @@ -54,6 +54,7 @@ "@jridgewell/remapping": "2.3.5", "async-retry": "1.3.3", "chalk": "2.3.1", + "esbuild": "0.25.8", "eslint-scope": "7.2.2", "glob": "11.1.0", "json-stream-stringify": "3.1.6", diff --git a/packages/tests/src/_jest/fixtures/action_catalog_project/action-execution.js b/packages/tests/src/_jest/fixtures/action_catalog_project/action-execution.js new file mode 100644 index 000000000..326bafb85 --- /dev/null +++ b/packages/tests/src/_jest/fixtures/action_catalog_project/action-execution.js @@ -0,0 +1,13 @@ +// Unless explicitly stated otherwise all files in this repository are licensed under the MIT License. +// This product includes software developed at Datadog (https://www.datadoghq.com/). +// Copyright 2019-Present Datadog, Inc. + +let implementation; + +export function setExecuteActionImplementation(fn) { + implementation = fn; +} + +export function getExecuteActionImplementation() { + return implementation; +} diff --git a/packages/tests/src/_jest/fixtures/action_catalog_project/index.js b/packages/tests/src/_jest/fixtures/action_catalog_project/index.js new file mode 100644 index 000000000..6476cbc32 --- /dev/null +++ b/packages/tests/src/_jest/fixtures/action_catalog_project/index.js @@ -0,0 +1,13 @@ +// Unless explicitly stated otherwise all files in this repository are licensed under the MIT License. +// This product includes software developed at Datadog (https://www.datadoghq.com/). +// Copyright 2019-Present Datadog, Inc. + +import { getExecuteActionImplementation } from './action-execution.js'; + +export async function sendSlackMessage(request) { + const implementation = getExecuteActionImplementation(); + if (!implementation) { + throw new Error('@datadog/action-catalog fixture: no execute-action implementation registered'); + } + return implementation('com.datadoghq.slack.chat.postMessage', request); +} diff --git a/packages/tests/src/_jest/fixtures/action_catalog_project/package.json b/packages/tests/src/_jest/fixtures/action_catalog_project/package.json new file mode 100644 index 000000000..678e69045 --- /dev/null +++ b/packages/tests/src/_jest/fixtures/action_catalog_project/package.json @@ -0,0 +1,14 @@ +{ + "name": "@datadog/action-catalog", + "version": "0.0.1", + "private": true, + "license": "MIT", + "author": "Datadog", + "type": "module", + "description": "Minimal local fixture standing in for the real @datadog/action-catalog package — only the pieces dev-server.integration.test.ts's connection-ID coverage actually exercises.", + "main": "index.js", + "exports": { + ".": "./index.js", + "./action-execution": "./action-execution.js" + } +} diff --git a/packages/tests/src/_jest/fixtures/apps_backend_project/actionCatalogCall.backend.ts b/packages/tests/src/_jest/fixtures/apps_backend_project/actionCatalogCall.backend.ts new file mode 100644 index 000000000..46a9b5daa --- /dev/null +++ b/packages/tests/src/_jest/fixtures/apps_backend_project/actionCatalogCall.backend.ts @@ -0,0 +1,9 @@ +// Unless explicitly stated otherwise all files in this repository are licensed under the MIT License. +// This product includes software developed at Datadog (https://www.datadoghq.com/). +// Copyright 2019-Present Datadog, Inc. + +import { sendSlackMessage } from '@datadog/action-catalog'; + +export async function postMessage() { + return sendSlackMessage({ inputs: { text: 'hi' }, connectionId: 'conn-1' }); +} diff --git a/packages/tests/src/_jest/fixtures/apps_backend_project/mixedImports.backend.ts b/packages/tests/src/_jest/fixtures/apps_backend_project/mixedImports.backend.ts new file mode 100644 index 000000000..450f8810f --- /dev/null +++ b/packages/tests/src/_jest/fixtures/apps_backend_project/mixedImports.backend.ts @@ -0,0 +1,19 @@ +// Unless explicitly stated otherwise all files in this repository are licensed under the MIT License. +// This product includes software developed at Datadog (https://www.datadoghq.com/). +// Copyright 2019-Present Datadog, Inc. + +import { helperEcho } from './helper'; + +// A non-local (npm-package) dynamic import specifically, not a local one — a +// local dynamic import is already caught by the fail-closed +// unsupportedDependencies check, which would mask the bug this fixture +// exists to exercise (silent misattribution of the static import textually +// after it, not a thrown error). +await import('chalk'); + +import { sendSlackMessage } from '@datadog/action-catalog'; + +export async function usesMixedImports(value: string) { + await helperEcho(value); + return sendSlackMessage({ inputs: { text: value }, connectionId: 'conn-1' }); +} diff --git a/packages/tests/src/_jest/fixtures/apps_backend_project/package.json b/packages/tests/src/_jest/fixtures/apps_backend_project/package.json index 988be3f0a..fabae5e0f 100644 --- a/packages/tests/src/_jest/fixtures/apps_backend_project/package.json +++ b/packages/tests/src/_jest/fixtures/apps_backend_project/package.json @@ -5,6 +5,7 @@ "author": "Datadog", "packageManager": "yarn@4.2.1", "dependencies": { + "@datadog/action-catalog": "portal:../action_catalog_project", "@datadog/apps-backend": "0.0.1" } } diff --git a/packages/tests/src/_jest/fixtures/package.json b/packages/tests/src/_jest/fixtures/package.json index df591e7ce..72bf7254e 100644 --- a/packages/tests/src/_jest/fixtures/package.json +++ b/packages/tests/src/_jest/fixtures/package.json @@ -7,6 +7,7 @@ "workspaces": [ "hard_project", "easy_project", - "apps_backend_project" + "apps_backend_project", + "action_catalog_project" ] } diff --git a/packages/tests/src/_jest/fixtures/yarn.lock b/packages/tests/src/_jest/fixtures/yarn.lock index 0a81c424b..79a91d93e 100644 --- a/packages/tests/src/_jest/fixtures/yarn.lock +++ b/packages/tests/src/_jest/fixtures/yarn.lock @@ -5,6 +5,18 @@ __metadata: version: 8 cacheKey: 10 +"@datadog/action-catalog@portal:../action_catalog_project::locator=%40tests%2Fapps_backend_project%40workspace%3Aapps_backend_project": + version: 0.0.0-use.local + resolution: "@datadog/action-catalog@portal:../action_catalog_project::locator=%40tests%2Fapps_backend_project%40workspace%3Aapps_backend_project" + languageName: node + linkType: soft + +"@datadog/action-catalog@workspace:action_catalog_project": + version: 0.0.0-use.local + resolution: "@datadog/action-catalog@workspace:action_catalog_project" + languageName: unknown + linkType: soft + "@datadog/apps-backend@npm:0.0.1": version: 0.0.1 resolution: "@datadog/apps-backend@npm:0.0.1" @@ -23,6 +35,7 @@ __metadata: version: 0.0.0-use.local resolution: "@tests/apps_backend_project@workspace:apps_backend_project" dependencies: + "@datadog/action-catalog": "portal:../action_catalog_project" "@datadog/apps-backend": "npm:0.0.1" languageName: unknown linkType: soft diff --git a/yarn.lock b/yarn.lock index 3d9228437..44fed7c02 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1976,6 +1976,7 @@ __metadata: "@types/eslint-scope": "npm:3.7.7" "@types/estree": "npm:1.0.8" chalk: "npm:2.3.1" + esbuild: "npm:0.25.8" eslint-scope: "npm:7.2.2" glob: "npm:11.1.0" jszip: "npm:3.10.1" From 568967e91f56de4c0f07709557615f25d5e4f543 Mon Sep 17 00:00:00 2001 From: Tiffany Trinh Date: Fri, 28 Aug 2026 01:14:11 -0400 Subject: [PATCH 07/14] fix(apps): fail closed on unresolved imports, require auth upfront, bound total execution time MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit collectModuleGraphFromServer silently fell back to a static import's raw specifier text when Vite's resolveId failed to resolve it, instead of failing closed like every sibling module-graph error path — a connectionId-scoped action call behind an unresolvable import would silently drop out of the allowlist instead of the request failing loudly. /__dd/executeAction ran a customer's real backend code with no auth check upfront, only lazily inside a $.Actions call — unlike production, which authenticates before any query/execution logic runs (app-builder-api's PreviewAsyncQueryHandler). A function that never calls $.Actions was a loophole around the same requirement production always enforces. Checked upfront as a local credential-presence check, not a network call, so it costs no latency on the local dev loop. guardedExecuteAction's hang-detection pause can't distinguish a customer function genuinely awaiting a slow $.Actions call from one that fired a call without awaiting it and then hung on something unrelated — an unawaited call masked a real hang for up to MAX_ACTION_CALL_TIMEOUT_MS (10 minutes). A second, independent absolute ceiling now bounds one execution's total wall-clock time regardless of pendingActionCalls, set just above pollQueryExecution's own ~300s worst-case long-poll budget so a legitimate slow call still always finishes. Also fixes a comment narrating this PR's own before/after history and an embedded milestone number, both against repo convention. --- .../src/vite/dev-server-module-graph.test.ts | 50 +++++++++++++++++++ .../apps/src/vite/dev-server-module-graph.ts | 13 +++-- .../src/vite/dev-server.integration.test.ts | 37 ++++++++++---- .../plugins/apps/src/vite/dev-server.test.ts | 26 +++++----- packages/plugins/apps/src/vite/dev-server.ts | 17 ++++--- .../apps/src/vite/local-execution.test.ts | 42 +++++++++++++++- .../plugins/apps/src/vite/local-execution.ts | 16 +++++- 7 files changed, 166 insertions(+), 35 deletions(-) create mode 100644 packages/plugins/apps/src/vite/dev-server-module-graph.test.ts diff --git a/packages/plugins/apps/src/vite/dev-server-module-graph.test.ts b/packages/plugins/apps/src/vite/dev-server-module-graph.test.ts new file mode 100644 index 000000000..26526fc1c --- /dev/null +++ b/packages/plugins/apps/src/vite/dev-server-module-graph.test.ts @@ -0,0 +1,50 @@ +// Unless explicitly stated otherwise all files in this repository are licensed under the MIT License. +// This product includes software developed at Datadog (https://www.datadoghq.com/). +// Copyright 2019-Present Datadog, Inc. + +import path from 'node:path'; + +import { LOCAL_EXECUTION_LOAD_SUFFIX } from '../constants'; + +import { collectModuleGraphFromServer } from './dev-server-module-graph'; + +const FIXTURE_ROOT = path.resolve( + __dirname, + '../../../../tests/src/_jest/fixtures/apps_backend_project', +); +const ENTRY_ID = path.join(FIXTURE_ROOT, 'helper.ts'); +const SUFFIXED_ENTRY_ID = ENTRY_ID + LOCAL_EXECUTION_LOAD_SUFFIX; + +function makeFakeServer(resolveId: (specifier: string) => Promise<{ id: string } | null>) { + return { + moduleGraph: { + getModuleById: (id: string) => + id === SUFFIXED_ENTRY_ID + ? { id: SUFFIXED_ENTRY_ID, file: ENTRY_ID, importedModules: new Set() } + : undefined, + }, + pluginContainer: { + resolveId: (specifier: string) => resolveId(specifier), + }, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + } as any; +} + +describe('dev-server-module-graph — collectModuleGraphFromServer', () => { + test('Should fail closed, not fall back to the raw specifier, when resolveId fails to resolve a static import', async () => { + const server = makeFakeServer(async () => null); + + await expect(collectModuleGraphFromServer(server, ENTRY_ID, FIXTURE_ROOT)).rejects.toThrow( + /unresolvable import specifier ".\/getRuntimeUsers\.backend"/, + ); + }); + + test('Should use the resolved id when resolveId succeeds', async () => { + const resolvedPath = path.join(FIXTURE_ROOT, 'getRuntimeUsers.backend.ts'); + const server = makeFakeServer(async () => ({ id: resolvedPath })); + + const records = await collectModuleGraphFromServer(server, ENTRY_ID, FIXTURE_ROOT); + + expect(records.has(ENTRY_ID)).toBe(true); + }); +}); diff --git a/packages/plugins/apps/src/vite/dev-server-module-graph.ts b/packages/plugins/apps/src/vite/dev-server-module-graph.ts index c567aeed0..40f649bb2 100644 --- a/packages/plugins/apps/src/vite/dev-server-module-graph.ts +++ b/packages/plugins/apps/src/vite/dev-server-module-graph.ts @@ -121,9 +121,16 @@ export async function collectModuleGraphFromServer( }), ), ); - const staticDependencyIds = resolutions.map((resolved, index) => - resolved ? normalizeViteModuleId(resolved.id) : staticModuleSources[index], - ); + const staticDependencyIds = resolutions.map((resolved, index) => { + if (!resolved) { + // Fail closed rather than trusting an incomplete allowlist — falling back to the raw specifier text would let a connectionId-scoped call behind an unresolvable import silently drop out of extractConnectionIdsFromModuleGraph's allowlist instead of the whole request failing loudly. + throw unsupportedModuleGraphDependency( + moduleId, + `unresolvable import specifier "${staticModuleSources[index]}"`, + ); + } + return normalizeViteModuleId(resolved.id); + }); const record = createParsedModuleRecord(moduleId, buildRoot, ast, staticDependencyIds); if (record) { diff --git a/packages/plugins/apps/src/vite/dev-server.integration.test.ts b/packages/plugins/apps/src/vite/dev-server.integration.test.ts index d526db5ea..ed7649224 100644 --- a/packages/plugins/apps/src/vite/dev-server.integration.test.ts +++ b/packages/plugins/apps/src/vite/dev-server.integration.test.ts @@ -160,14 +160,18 @@ describe('Dev Server Middleware — real end-to-end local execution', () => { }); test('Should import a real backend function directly via the real Vite dev server and execute it locally, with a real @datadog/apps-backend typed import resolving $.Source correctly', async () => { + const auth: AuthOptionsWithDefaults = { + apiKey: 'test-api-key', + appKey: 'test-app-key', + site: 'datadoghq.com', + }; const middleware = createDevServerMiddleware( build, server.ssrLoadModule.bind(server), () => [getRuntimeUsersFunc], async () => [], - { site: 'datadoghq.com' }, - // No auth configured — this function never calls $.Actions. - undefined, + auth, + getAuthenticatedRequest('apiKey', auth, getMockLogger()), FIXTURE_ROOT, getMockLogger(), ); @@ -202,13 +206,18 @@ describe('Dev Server Middleware — real end-to-end local execution', () => { // doesn't exist server-side), and this would throw instead of returning // the real value. test('Should preserve real code for a nested *.backend.ts import, not swap it for the frontend RPC-proxy stub', async () => { + const auth: AuthOptionsWithDefaults = { + apiKey: 'test-api-key', + appKey: 'test-app-key', + site: 'datadoghq.com', + }; const middleware = createDevServerMiddleware( build, server.ssrLoadModule.bind(server), () => [nestedImportFunc], async () => [], - { site: 'datadoghq.com' }, - undefined, + auth, + getAuthenticatedRequest('apiKey', auth, getMockLogger()), FIXTURE_ROOT, getMockLogger(), ); @@ -239,13 +248,18 @@ describe('Dev Server Middleware — real end-to-end local execution', () => { // backend-to-backend case above, and swapping getRuntimeUsers.backend.ts // for its frontend RPC-proxy stub. test('Should preserve real code for a *.backend.ts import reached through an intermediate non-backend module', async () => { + const auth: AuthOptionsWithDefaults = { + apiKey: 'test-api-key', + appKey: 'test-app-key', + site: 'datadoghq.com', + }; const middleware = createDevServerMiddleware( build, server.ssrLoadModule.bind(server), () => [viaHelperFunc], async () => [], - { site: 'datadoghq.com' }, - undefined, + auth, + getAuthenticatedRequest('apiKey', auth, getMockLogger()), FIXTURE_ROOT, getMockLogger(), ); @@ -320,13 +334,18 @@ describe('Dev Server Middleware — real end-to-end local execution', () => { FIXTURE_ROOT, ); + const auth: AuthOptionsWithDefaults = { + apiKey: 'test-api-key', + appKey: 'test-app-key', + site: 'datadoghq.com', + }; const middleware = createDevServerMiddleware( build, loadModule, () => [noSdkFunc], getAllowedConnectionIds, - { site: 'datadoghq.com' }, - undefined, + auth, + getAuthenticatedRequest('apiKey', auth, getMockLogger()), FIXTURE_ROOT, getMockLogger(), ); diff --git a/packages/plugins/apps/src/vite/dev-server.test.ts b/packages/plugins/apps/src/vite/dev-server.test.ts index b926d4fd1..31bb2361a 100644 --- a/packages/plugins/apps/src/vite/dev-server.test.ts +++ b/packages/plugins/apps/src/vite/dev-server.test.ts @@ -845,7 +845,7 @@ describe('Dev Server Middleware', () => { expect(mockViteBuild).not.toHaveBeenCalled(); }); - test('Should work with no auth configured at all, for a function that never calls $.Actions', async () => { + test('Should return a clear error when a function calls $.Actions with no auth configured', async () => { const noAuthMiddleware = createDevServerMiddleware( mockViteBuild, mockLoadModule, @@ -856,7 +856,13 @@ describe('Dev Server Middleware', () => { '/project', mockLog, ); - mockLoadModuleReturning(mockFunctions[0], () => 1); + mockLoadModuleReturning(mockFunctions[0], () => + ( + globalThis as typeof globalThis & { $: { Actions: ActionsProxy } } + ).$.Actions.slack.chat.postMessage({ + inputs: { text: 'hi' }, + }), + ); const req = createMockRequest('/__dd/executeAction', { functionName: encodeQueryName(mockFunctions[0]), @@ -867,13 +873,13 @@ describe('Dev Server Middleware', () => { noAuthMiddleware(req, res, jest.fn()); await res.done; - expect(res.statusCode).toBe(200); + expect(res.statusCode).toBe(400); const body = JSON.parse(res.getBody()); - expect(body.success).toBe(true); - expect(body.result).toEqual({ data: 1 }); + expect(body.success).toBe(false); + expect(body.error).toContain('Auth credentials not configured'); }); - test('Should return a clear error when a function calls $.Actions with no auth configured', async () => { + test('Should reject a request with no auth configured upfront, even for a function that never calls $.Actions — matching production, which authenticates before any backend code runs', async () => { const noAuthMiddleware = createDevServerMiddleware( mockViteBuild, mockLoadModule, @@ -884,13 +890,7 @@ describe('Dev Server Middleware', () => { '/project', mockLog, ); - mockLoadModuleReturning(mockFunctions[0], () => - ( - globalThis as typeof globalThis & { $: { Actions: ActionsProxy } } - ).$.Actions.slack.chat.postMessage({ - inputs: { text: 'hi' }, - }), - ); + mockLoadModuleReturning(mockFunctions[0], () => 'pure result, no $.Actions call'); const req = createMockRequest('/__dd/executeAction', { functionName: encodeQueryName(mockFunctions[0]), diff --git a/packages/plugins/apps/src/vite/dev-server.ts b/packages/plugins/apps/src/vite/dev-server.ts index ac0213de9..2bd8601df 100644 --- a/packages/plugins/apps/src/vite/dev-server.ts +++ b/packages/plugins/apps/src/vite/dev-server.ts @@ -385,9 +385,9 @@ async function handleDebugBundle( /** * Handle POST /__dd/executeAction — imports a backend function's real file * directly and executes it in-process (see local-execution.ts); no bundling - * on this path. Customer-facing default: no auth required upfront, since the - * script itself doesn't need it — only a real `$.Actions` call does, and - * that's checked lazily (see makeExecuteActionRemotely). + * on this path. Auth is required upfront (checked by the caller in + * createDevServerMiddleware before this is reached), matching production's + * own auth-before-execution ordering. */ async function handleExecuteAction( req: IncomingMessage, @@ -465,10 +465,8 @@ async function handleExecuteAction( /** * Handle POST /__dd/executeActionViaCloud — bundles a backend function and * executes it via the existing production round trip (queue + Deno - * subprocess), the same way `/__dd/executeAction` did before local - * execution existed. Kept as a distinctly-purposed command (`npm run - * dev:verify`, Milestone 3) for pre-publish parity checks, not a mode flag - * on the same endpoint. + * subprocess). Kept as a distinctly-purposed command (`npm run dev:verify`) + * for pre-publish parity checks, not a mode flag on the same endpoint. */ async function handleExecuteActionViaCloud( req: IncomingMessage, @@ -559,6 +557,11 @@ export function createDevServerMiddleware( sendError(res, 500, 'Unexpected error'); }); } else if (req.url === '/__dd/executeAction') { + // Matches production, which authenticates before any backend-function code runs (app-builder-api's PreviewAsyncQueryHandler checks the user first, before the query/execution path) — checked here upfront rather than only lazily inside a $.Actions call, so a function that never calls $.Actions isn't a loophole around the same requirement. A local presence check only (not a real credential-validation network call), so it costs no latency on the fast local dev loop. + if (!doAuthenticatedRequest) { + sendError(res, 400, `Auth credentials not configured. ${AUTH_GUIDANCE}`); + return; + } handleExecuteAction( req, res, diff --git a/packages/plugins/apps/src/vite/local-execution.test.ts b/packages/plugins/apps/src/vite/local-execution.test.ts index 006fa9598..4332cfd77 100644 --- a/packages/plugins/apps/src/vite/local-execution.test.ts +++ b/packages/plugins/apps/src/vite/local-execution.test.ts @@ -574,7 +574,11 @@ describe('local-execution — executeScriptLocally', () => { // a bound on the $.Actions call itself, a stalled network request (no // abort signal/deadline of its own) would wedge this execution — and, // since local executions are serialized via `enqueue`, every request - // queued behind it — indefinitely. + // queued behind it — indefinitely. The absolute execution ceiling + // (MAX_TOTAL_EXECUTION_TIMEOUT_MS, 6 minutes) now always fires before + // the per-call bound (MAX_ACTION_CALL_TIMEOUT_MS, 10 minutes) could, so + // that's the mechanism actually observed here — MAX_ACTION_CALL_TIMEOUT_MS + // remains a backstop for any path the ceiling doesn't cover. test('Should eventually time out an in-flight $.Actions call that never settles, and not wedge subsequently queued executions', async () => { jest.useFakeTimers(); try { @@ -608,7 +612,7 @@ describe('local-execution — executeScriptLocally', () => { ); const hungAssertion = expect(hungExecution).rejects.toThrow( - /\$\.Actions call to "com\.datadoghq\.slack\.chat\.postMessage" timed out/, + /exceeded the absolute 360000ms execution ceiling/, ); await jest.runAllTimersAsync(); @@ -620,6 +624,40 @@ describe('local-execution — executeScriptLocally', () => { } }); + // A fire-and-forget $.Actions call (not awaited by the customer function) increments pendingActionCalls the same as an awaited one, pausing the per-call hang-detection timer for as long as that call stays in flight — up to MAX_ACTION_CALL_TIMEOUT_MS (10 minutes) if the call never settles, even though the customer function itself moved on to something else entirely. The absolute execution ceiling below must still fire well before that. + test('Should eventually time out via an absolute execution ceiling, independent of any $.Actions call still in flight', async () => { + jest.useFakeTimers(); + try { + const neverSettlingExecuteAction: ExecuteAction = () => new Promise(() => {}); + + const execution = executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + neverSettlingExecuteAction, + loadModuleReturning({ + example: () => + ( + globalThis as typeof globalThis & { $: { Actions: ActionsProxy } } + ).$.Actions.slack.chat.postMessage({ + inputs: { text: 'hi' }, + }), + }), + mockLogger, + 50, + ); + + const assertion = expect(execution).rejects.toThrow( + /exceeded the absolute 360000ms execution ceiling/, + ); + + await jest.advanceTimersByTimeAsync(6 * 60_000); + await assertion; + } finally { + jest.useRealTimers(); + } + }); + test('Should still time out a function that hangs with no $.Actions call in flight, even after an earlier call in the same run completed', async () => { const executeAction: ExecuteAction = async () => ({ ok: true }); diff --git a/packages/plugins/apps/src/vite/local-execution.ts b/packages/plugins/apps/src/vite/local-execution.ts index eb703cf58..4ac3808d5 100644 --- a/packages/plugins/apps/src/vite/local-execution.ts +++ b/packages/plugins/apps/src/vite/local-execution.ts @@ -115,6 +115,9 @@ export const DEFAULT_TIMEOUT_MS = 10_000; /** Bounds a single `$.Actions` call while it's exempt from the hang-detection timer above (see `guardedExecuteAction`). `doRequest` attaches no abort signal or deadline of its own, so an in-flight call that never settles would otherwise wedge this execution — and, since local executions are serialized, every request queued behind it — forever. Set generously past `pollQueryExecution`'s own worst-case long-poll budget (10 retries at up to ~30s each) so a legitimate slow action is never cut off. */ const MAX_ACTION_CALL_TIMEOUT_MS = 10 * 60_000; +/** Absolute ceiling on one execution's total wall-clock time, independent of `pendingActionCalls`'s pause-and-extend mechanism (see `guardedExecuteAction`) — that mechanism can't distinguish a customer function genuinely awaiting a slow `$.Actions` call from one that fired a call without awaiting it and then hung on something unrelated, so an unawaited call currently masks a real hang for as long as MAX_ACTION_CALL_TIMEOUT_MS. Set just above `pollQueryExecution`'s own worst-case long-poll budget (10 retries at up to ~30s each, ~300s) so a legitimate single slow `$.Actions` call still always finishes — this bounds the masked-hang worst case to roughly 6 minutes instead of the full 10, not lower, since going lower would start killing real in-progress calls instead of just hangs. */ +const MAX_TOTAL_EXECUTION_TIMEOUT_MS = 6 * 60_000; + /** Loads a module by specifier, resolved against the customer's own project rather than build-plugins' dependency tree — the dev server passes its Vite instance's `ssrLoadModule` here. */ export type LoadModule = (specifier: string) => Promise>; @@ -596,7 +599,17 @@ async function runScriptLocally( scheduleTimeout(); }); - // Racing the timeout only stops the caller from waiting — run() keeps executing afterward, so a resumed customer function can still fire real $.Actions side effects; true cancellation would need a Worker thread, not possible in-process. + // Fires regardless of pendingActionCalls, unlike the pause-and-extend timeout above — bounds the worst case of a fire-and-forget $.Actions call masking an unrelated hang to MAX_TOTAL_EXECUTION_TIMEOUT_MS instead of the per-call MAX_ACTION_CALL_TIMEOUT_MS. + const absoluteTimeoutTimer = setTimeout(() => { + concludeExecution(); + rejectTimeout?.( + new Error( + `Local execution of "${func.name}" exceeded the absolute ${MAX_TOTAL_EXECUTION_TIMEOUT_MS}ms execution ceiling, regardless of any $.Actions call in flight.`, + ), + ); + }, MAX_TOTAL_EXECUTION_TIMEOUT_MS); + + // Racing against the timeout only stops the caller from waiting — run() keeps executing in-process afterward, so a customer function that resumes post-timeout can still fire real $.Actions side effects. True cancellation requires terminating a Worker thread, not possible for in-process execution. const runPromise = run(); // Set once the race settles, so the handler below can tell an abandoned rejection (caller already gone) from an ordinary one the caller is about to receive normally. let raceSettled = false; @@ -614,5 +627,6 @@ async function runScriptLocally( } finally { raceSettled = true; clearTimeout(timer); + clearTimeout(absoluteTimeoutTimer); } } From 95f45327dcbea8f9aa25f187071c4168bcc4c4de Mon Sep 17 00:00:00 2001 From: Tiffany Trinh Date: Fri, 28 Aug 2026 01:43:15 -0400 Subject: [PATCH 08/14] fix(apps): remove now-dead lazy auth check and its stale doc comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit makeExecuteActionRemotely's own inner auth check and doc comment ("no auth needed until a call is actually made") went stale once the caller started requiring auth upfront — its only caller is only ever reached after that upfront check already passed, making the inner check unreachable and the comment actively contradictory. Narrows both functions' doAuthenticatedRequest parameter to required, matching how the sibling /__dd/executeActionViaCloud path already types it. Removes the now-redundant "no auth + calls $.Actions" test, fully subsumed by the upfront-check test right after it — no test exercises the removed lazy check anymore since nothing can reach it. --- .../plugins/apps/src/vite/dev-server.test.ts | 34 ------------------- packages/plugins/apps/src/vite/dev-server.ts | 9 ++--- 2 files changed, 3 insertions(+), 40 deletions(-) diff --git a/packages/plugins/apps/src/vite/dev-server.test.ts b/packages/plugins/apps/src/vite/dev-server.test.ts index 31bb2361a..6192e753f 100644 --- a/packages/plugins/apps/src/vite/dev-server.test.ts +++ b/packages/plugins/apps/src/vite/dev-server.test.ts @@ -845,40 +845,6 @@ describe('Dev Server Middleware', () => { expect(mockViteBuild).not.toHaveBeenCalled(); }); - test('Should return a clear error when a function calls $.Actions with no auth configured', async () => { - const noAuthMiddleware = createDevServerMiddleware( - mockViteBuild, - mockLoadModule, - () => mockFunctions, - async () => [], - mockOauthOnlyAuth, - undefined, - '/project', - mockLog, - ); - mockLoadModuleReturning(mockFunctions[0], () => - ( - globalThis as typeof globalThis & { $: { Actions: ActionsProxy } } - ).$.Actions.slack.chat.postMessage({ - inputs: { text: 'hi' }, - }), - ); - - const req = createMockRequest('/__dd/executeAction', { - functionName: encodeQueryName(mockFunctions[0]), - args: [], - }); - const res = createMockResponse(); - - noAuthMiddleware(req, res, jest.fn()); - await res.done; - - expect(res.statusCode).toBe(400); - const body = JSON.parse(res.getBody()); - expect(body.success).toBe(false); - expect(body.error).toContain('Auth credentials not configured'); - }); - test('Should reject a request with no auth configured upfront, even for a function that never calls $.Actions — matching production, which authenticates before any backend code runs', async () => { const noAuthMiddleware = createDevServerMiddleware( mockViteBuild, diff --git a/packages/plugins/apps/src/vite/dev-server.ts b/packages/plugins/apps/src/vite/dev-server.ts index 2bd8601df..4c257f36a 100644 --- a/packages/plugins/apps/src/vite/dev-server.ts +++ b/packages/plugins/apps/src/vite/dev-server.ts @@ -216,10 +216,10 @@ async function executeScriptViaDatadog( return outputs; } -/** Submits a single-action `preview-async` query per `$.Actions` call (no auth needed until a call is actually made) and logs its result/error, since production's own equivalent signal only reaches Datadog's backend, not the developer's `npm run dev` console. */ +/** Submits a single-action `preview-async` query per `$.Actions` call and logs its result/error, since production's own equivalent signal only reaches Datadog's backend, not the developer's `npm run dev` console. Callers must already have confirmed auth is configured — see `handleExecuteAction`'s own upfront check. */ function makeExecuteActionRemotely( auth: AuthConfig, - doAuthenticatedRequest: DoAuthenticatedRequest | undefined, + doAuthenticatedRequest: DoAuthenticatedRequest, log: Logger, ): ExecuteAction { return async ( @@ -227,9 +227,6 @@ function makeExecuteActionRemotely( inputs: unknown, connectionId: string | undefined, ): Promise => { - if (!doAuthenticatedRequest) { - throw new HttpError(400, `Auth credentials not configured. ${AUTH_GUIDANCE}`); - } try { const receiptId = await submitQuery( connectionId ? { fqn, inputs, connectionId } : { fqn, inputs }, @@ -394,7 +391,7 @@ async function handleExecuteAction( res: ServerResponse, functionsByName: Map, auth: AuthConfig, - doAuthenticatedRequest: DoAuthenticatedRequest | undefined, + doAuthenticatedRequest: DoAuthenticatedRequest, loadModule: LoadModule, getAllowedConnectionIds: (entryId: string) => Promise, projectRoot: string, From fe24fa18f4938a021ae1c8f40c650b97fe5d8f0d Mon Sep 17 00:00:00 2001 From: Tiffany Trinh Date: Fri, 28 Aug 2026 02:13:40 -0400 Subject: [PATCH 09/14] fix(apps): scope the module-graph priming load's $ access like executeScriptLocally's own load MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit handleExecuteAction's priming load is the only place a customer module's top-level code actually runs (Vite caches the module, so executeScriptLocally's own load below just reuses the resolved object) — but it called loadModule directly instead of going through customerModuleLoadContext, so a customer module reaching for $ during its own top-level evaluation silently resolved to whatever $ a prior execution left behind instead of throwing the same way it does inside executeScriptLocally. Extracts the scoping into loadCustomerModuleEntry, shared by both call sites. Also corrects the startup warning logged when auth isn't configured: it still described only $.Actions calls as failing, but the earlier auth-upfront hardening rejects the whole /__dd/executeAction endpoint before any backend-function code runs, regardless of whether it calls $.Actions. --- .../plugins/apps/src/vite/dev-server.test.ts | 36 +++++++++++++++++++ packages/plugins/apps/src/vite/dev-server.ts | 23 +++++++++--- .../plugins/apps/src/vite/local-execution.ts | 15 ++++++-- 3 files changed, 67 insertions(+), 7 deletions(-) diff --git a/packages/plugins/apps/src/vite/dev-server.test.ts b/packages/plugins/apps/src/vite/dev-server.test.ts index 6192e753f..afc12c238 100644 --- a/packages/plugins/apps/src/vite/dev-server.test.ts +++ b/packages/plugins/apps/src/vite/dev-server.test.ts @@ -15,6 +15,7 @@ import { parseAst } from 'rollup/parseAst'; import { encodeQueryName } from '../backend/encodeQueryName'; import type { BackendFunction } from '../backend/types'; +import { LOCAL_EXECUTION_LOAD_SUFFIX } from '../constants'; jest.mock('@dd/core/helpers/oauth-request', () => ({ doOAuthRequest: jest.fn(async (opts) => { @@ -1022,6 +1023,41 @@ describe('Dev Server Middleware', () => { ); }); + // The priming loadModule call (see handleExecuteAction) is the only + // place the entry's top-level code actually runs — Vite caches the + // module, so executeScriptLocally's own load below just reuses this + // same resolved object — so it must carry the same $-scoping + // guarantee executeScriptLocally's load would otherwise provide. + test('Should throw when a customer module reaches for $ during its own top-level evaluation, even though the module-graph priming load runs before executeScriptLocally installs its own scoping', async () => { + let dollarDuringTopLevelLoad: unknown = 'not captured'; + mockLoadModule.mockImplementation(async (specifier: string) => { + if (specifier === mockFunctions[0].absolutePath + LOCAL_EXECUTION_LOAD_SUFFIX) { + try { + dollarDuringTopLevelLoad = (globalThis as Record).$; + } catch (error) { + dollarDuringTopLevelLoad = error; + } + return { [mockFunctions[0].name]: () => 'done' }; + } + throw new Error(`Cannot find module '${specifier}'`); + }); + + const req = createMockRequest('/__dd/executeAction', { + functionName: encodeQueryName(mockFunctions[0]), + args: [], + }); + const res = createMockResponse(); + + middleware(req, res, jest.fn()); + await res.done; + + expect(res.statusCode).toBe(200); + expect(dollarDuringTopLevelLoad).toBeInstanceOf(Error); + expect((dollarDuringTopLevelLoad as Error).message).toBe( + 'No active local execution to resolve $ under.', + ); + }); + // Guards the priming loadModule call (see handleExecuteAction) — it // evaluates the entry's real top-level code before // executeScriptLocally's own hang-detection timeout is installed, so diff --git a/packages/plugins/apps/src/vite/dev-server.ts b/packages/plugins/apps/src/vite/dev-server.ts index 4c257f36a..263cee24c 100644 --- a/packages/plugins/apps/src/vite/dev-server.ts +++ b/packages/plugins/apps/src/vite/dev-server.ts @@ -20,7 +20,12 @@ import { LOCAL_EXECUTION_LOAD_SUFFIX } from '../constants'; import { createBackendConnectionIdCollector } from './backend-connection-id-collector'; import { getBaseBackendBuildConfig } from './build-config'; import type { ExecuteAction, LoadModule } from './local-execution'; -import { DEFAULT_TIMEOUT_MS, executeScriptLocally, withTimeout } from './local-execution'; +import { + DEFAULT_TIMEOUT_MS, + executeScriptLocally, + loadCustomerModuleEntry, + withTimeout, +} from './local-execution'; interface BundleResult { func: BackendFunction; @@ -216,7 +221,7 @@ async function executeScriptViaDatadog( return outputs; } -/** Submits a single-action `preview-async` query per `$.Actions` call and logs its result/error, since production's own equivalent signal only reaches Datadog's backend, not the developer's `npm run dev` console. Callers must already have confirmed auth is configured — see `handleExecuteAction`'s own upfront check. */ +/** Submits a single-action `preview-async` query per `$.Actions` call and logs its result/error, since production's own equivalent signal only reaches Datadog's backend, not the developer's `npm run dev` console. Callers must already have confirmed auth is configured — see `createDevServerMiddleware`'s upfront check on this route. */ function makeExecuteActionRemotely( auth: AuthConfig, doAuthenticatedRequest: DoAuthenticatedRequest, @@ -413,10 +418,18 @@ async function handleExecuteAction( // installs its own hang-detection timeout below, and it evaluates the // entry's real top-level code (ssrLoadModule, not a parse-only step) // — so it needs its own bound, or a customer module with a hanging - // top-level await would wedge this request forever. + // top-level await would wedge this request forever. It's also the + // only place that top-level code actually runs (Vite caches the + // module, and executeScriptLocally below reuses this same resolved + // object rather than re-evaluating it), so it must go through + // loadCustomerModuleEntry's scoping rather than a raw loadModule call + // — otherwise a customer module reaching for $ during its own + // top-level evaluation would silently resolve to whatever `$` a + // prior execution happened to leave behind, instead of throwing the + // same way it does inside executeScriptLocally. const entrySpecifier = func.absolutePath + LOCAL_EXECUTION_LOAD_SUFFIX; const primedModule = await withTimeout( - loadModule(entrySpecifier), + loadCustomerModuleEntry(loadModule, entrySpecifier), DEFAULT_TIMEOUT_MS, `Loading "${displayName}"`, ); @@ -537,7 +550,7 @@ export function createDevServerMiddleware( if (!doAuthenticatedRequest) { log.warn( - `Auth credentials not configured. Backend functions that call $.Actions will fail; the /__dd/executeActionViaCloud endpoint will be unavailable. ${AUTH_GUIDANCE}`, + `Auth credentials not configured. Both the /__dd/executeAction and /__dd/executeActionViaCloud endpoints will be unavailable. ${AUTH_GUIDANCE}`, ); } diff --git a/packages/plugins/apps/src/vite/local-execution.ts b/packages/plugins/apps/src/vite/local-execution.ts index 4ac3808d5..27b623a2f 100644 --- a/packages/plugins/apps/src/vite/local-execution.ts +++ b/packages/plugins/apps/src/vite/local-execution.ts @@ -121,6 +121,16 @@ const MAX_TOTAL_EXECUTION_TIMEOUT_MS = 6 * 60_000; /** Loads a module by specifier, resolved against the customer's own project rather than build-plugins' dependency tree — the dev server passes its Vite instance's `ssrLoadModule` here. */ export type LoadModule = (specifier: string) => Promise>; +/** Loads a customer module's entry under the same top-level-evaluation scoping `runScriptLocally`'s own load uses (see `customerModuleLoadContext`'s doc comment above) — for callers that need to trigger the entry's real top-level evaluation ahead of `executeScriptLocally` (e.g. dev-server.ts's module-graph priming load), so a customer module reaching for `$` during that load fails the same way it would inside `executeScriptLocally`, instead of silently resolving to whatever `globalDollarOutsideExecution` happens to hold. */ +export function loadCustomerModuleEntry( + loadModule: LoadModule, + entrySpecifier: string, +): Promise> { + return customerModuleLoadContext.run({ assigned: false, value: undefined }, () => + loadModule(entrySpecifier), + ); +} + /** Executes a real `$.Actions.foo.bar(...)` call; the dev server supplies the implementation using its own auth, so this module never holds or sees a credential itself. */ export type ExecuteAction = ( fqn: string, @@ -550,8 +560,9 @@ async function runScriptLocally( const run = async (): Promise => { // Loads and evaluates the customer's module BEFORE installing $ and the SDK bridges below, matching production's own ordering (backend/virtual-entry.ts statically imports the customer module before its wrapper installs $ and the SDK bridges) — code that reaches for $ or a typed action during its own top-level evaluation fails the same way locally as it would in Datadog, instead of silently succeeding against bindings production wouldn't have installed yet. - const mod = await customerModuleLoadContext.run({ assigned: false, value: undefined }, () => - loadModule(func.absolutePath + LOCAL_EXECUTION_LOAD_SUFFIX), + const mod = await loadCustomerModuleEntry( + loadModule, + func.absolutePath + LOCAL_EXECUTION_LOAD_SUFFIX, ); const fn = mod[func.name]; if (typeof fn !== 'function') { From 8b69b176e5307a47319e16385a36143d83c1ec0c Mon Sep 17 00:00:00 2001 From: Tiffany Trinh Date: Fri, 28 Aug 2026 03:16:53 -0400 Subject: [PATCH 10/14] fix(apps): add missing startup-warning test coverage, fix convention violations found in round-6 review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add test coverage for the startup auth-warning log message (dev-server.ts) — no test asserted its exact wording, which is how a stale claim about which endpoints fail escaped an earlier review round. - guardedExecuteAction is async, so its not-current branch's `return Promise.reject(...)` was needless wrapping — use `throw` instead. - Extract three inlined function-call arguments (withTimeout's first argument at two dev-server.ts call sites, executeAction's result in local-execution.ts) into named locals, matching this file's own existing convention at every other withTimeout call site. - Replace a bare `as any` cast (with an eslint-disable to suppress the rule that would flag it) in dev-server-module-graph.test.ts with a narrower `as unknown as ViteDevServer`. - Move two same-line comments onto their own line. - Reword four regression-test comments in dev-server.integration.test.ts that narrated "before the fix, X happened" — restated as the present-tense invariant each test guards. --- .../src/backend/ast-parsing/module-graph.ts | 2 +- .../src/vite/dev-server-module-graph.test.ts | 4 +- .../apps/src/vite/dev-server-module-graph.ts | 63 +++---- .../src/vite/dev-server.integration.test.ts | 160 +++++++----------- .../plugins/apps/src/vite/dev-server.test.ts | 106 +++++++----- packages/plugins/apps/src/vite/dev-server.ts | 86 ++++------ packages/plugins/apps/src/vite/index.test.ts | 9 +- packages/plugins/apps/src/vite/index.ts | 84 ++++----- .../apps/src/vite/local-execution.test.ts | 29 ++-- .../plugins/apps/src/vite/local-execution.ts | 34 ++-- 10 files changed, 243 insertions(+), 334 deletions(-) diff --git a/packages/plugins/apps/src/backend/ast-parsing/module-graph.ts b/packages/plugins/apps/src/backend/ast-parsing/module-graph.ts index b3621c8b2..36c989f14 100644 --- a/packages/plugins/apps/src/backend/ast-parsing/module-graph.ts +++ b/packages/plugins/apps/src/backend/ast-parsing/module-graph.ts @@ -190,7 +190,7 @@ function collectStaticModuleDependencies( })); } -// Exported so callers that must derive dependency ids by resolving each static specifier individually (the dev server has no build-time Rollup ModuleInfo to read them from) extract the exact same specifier list this module zips them against — not a second, independently-written AST walk that could drift from this one. +// Exported so callers without build-time Rollup ModuleInfo (the dev server) can resolve each static specifier individually against the exact same list this module zips against, rather than a second AST walk that could drift from it. export function getStaticModuleSources(ast: Program): string[] { return ast.body.flatMap((node) => { if ( diff --git a/packages/plugins/apps/src/vite/dev-server-module-graph.test.ts b/packages/plugins/apps/src/vite/dev-server-module-graph.test.ts index 26526fc1c..00d2fe0d4 100644 --- a/packages/plugins/apps/src/vite/dev-server-module-graph.test.ts +++ b/packages/plugins/apps/src/vite/dev-server-module-graph.test.ts @@ -3,6 +3,7 @@ // Copyright 2019-Present Datadog, Inc. import path from 'node:path'; +import type { ViteDevServer } from 'vite'; import { LOCAL_EXECUTION_LOAD_SUFFIX } from '../constants'; @@ -26,8 +27,7 @@ function makeFakeServer(resolveId: (specifier: string) => Promise<{ id: string } pluginContainer: { resolveId: (specifier: string) => resolveId(specifier), }, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - } as any; + } as unknown as ViteDevServer; } describe('dev-server-module-graph — collectModuleGraphFromServer', () => { diff --git a/packages/plugins/apps/src/vite/dev-server-module-graph.ts b/packages/plugins/apps/src/vite/dev-server-module-graph.ts index 40f649bb2..a9c892e83 100644 --- a/packages/plugins/apps/src/vite/dev-server-module-graph.ts +++ b/packages/plugins/apps/src/vite/dev-server-module-graph.ts @@ -4,8 +4,8 @@ /* eslint-disable no-await-in-loop */ +import { readFile } from '@dd/core/helpers/fs'; import { transform } from 'esbuild'; -import { readFile } from 'node:fs/promises'; import { parseAst } from 'rollup/parseAst'; import type { ModuleNode, ViteDevServer } from 'vite'; @@ -20,35 +20,24 @@ import { LOCAL_EXECUTION_LOAD_SUFFIX } from '../constants'; /** * Builds the same `ReadonlyMap` shape - * `createBackendModuleGraphCollector`'s `moduleParsed` hook produces during a - * real Rollup build — but for the dev server, where `moduleParsed` never - * fires at all (it's a Rollup-build-only hook; Vite's dev-server plugin - * container doesn't implement it). Instead, this walks Vite's own - * `server.moduleGraph`, which the dev server already populates as a side - * effect of `ssrLoadModule`: by the time an `await server.ssrLoadModule(id)` - * call resolves, the entry's `ModuleNode.importedModules` — and every - * imported module's own `importedModules` — already reflect the full - * transitive static-import graph, recursively, with no extra ticks needed. + * `createBackendModuleGraphCollector`'s `moduleParsed` hook produces during a real Rollup + * build, but for the dev server, where that hook never fires (Vite's plugin container + * doesn't implement it) — instead walking `server.moduleGraph`, which `ssrLoadModule` + * already populates with the entry's full transitive static-import graph by the time it + * resolves. * - * Parses each module's own source read fresh from disk (via `ModuleNode.file`), - * stripped of TS/JSX syntax by `esbuild.transform` in isolation, rather than - * either of Vite's own transform results: the client transform - * (`transformResult`) doesn't run for an SSR-only load, and the SSR transform - * (`ssrTransformResult`) rewrites every `import` into a `__vite_ssr_import__(...)` - * call and resolves bare specifiers to absolute paths — neither shape - * `collectActionCatalogImports`'s plain-`ImportDeclaration` search (built for - * the untransformed syntax a real Rollup build sees) can parse. `esbuild.transform` - * in isolation only strips types/JSX; it doesn't touch import specifiers at all, - * so this reads exactly the same syntax the production build path already trusts. + * Parses each module's source fresh from disk (via `ModuleNode.file`), stripped of TS/JSX + * by `esbuild.transform` in isolation, rather than Vite's own transform results — the + * client transform doesn't run for an SSR-only load, and the SSR transform rewrites + * imports into `__vite_ssr_import__(...)` calls that `collectActionCatalogImports`'s + * plain-`ImportDeclaration` parser can't read. `esbuild.transform` alone only strips + * types/JSX, leaving import specifiers untouched, so this sees the same syntax the + * production build path already trusts. * - * Call this only after `loadModule` has resolved for `bareEntryId + - * LOCAL_EXECUTION_LOAD_SUFFIX` in the same request — the graph it reads is a - * live side effect of that call, not independently maintained state. - * `bareEntryId` is the same unsuffixed id `extractConnectionIdsFromModuleGraph` - * needs to key into the returned map below; the suffix is appended here, - * internally, rather than left to each caller to remember — Vite keys the - * node it just loaded by the full resolved id (suffix included), since it - * treats each distinct query string as a logically distinct module. + * Call only after `loadModule` has resolved for `bareEntryId + LOCAL_EXECUTION_LOAD_SUFFIX` + * in the same request — the graph is a live side effect of that call. The suffix is + * appended internally, since Vite keys the loaded node by its full resolved id, so callers + * only need to pass the bare id `extractConnectionIdsFromModuleGraph` also uses. */ export async function collectModuleGraphFromServer( server: ViteDevServer, @@ -78,7 +67,7 @@ export async function collectModuleGraphFromServer( let source: string; try { - source = await readFile(node.file, 'utf-8'); + source = await readFile(node.file); } catch (error) { const reason = error instanceof Error ? error.message : String(error); throw unsupportedModuleGraphDependency( @@ -102,16 +91,12 @@ export async function collectModuleGraphFromServer( ); } - // `node.importedModules` mixes static AND dynamic imports with no - // documented ordering guarantee, but `createParsedModuleRecord` - // zips dependency ids positionally against the AST's own static - // import/export declarations — so this list must contain ONLY - // static dependencies, in that same order. The dev server's plugin - // container doesn't populate Rollup-style `ModuleInfo.importedIds` - // outside a real build, so this resolves each of the AST's own - // static specifiers individually instead, via the same resolution - // Vite itself would use — guaranteeing a correct 1:1 correspondence - // by construction, since both sides are derived from this same ast. + // `node.importedModules` mixes static and dynamic imports with no ordering guarantee, + // but `createParsedModuleRecord` zips dependency ids positionally against the AST's + // static import/export declarations, so this list must be static-only, same order. The + // dev server has no Rollup-style `ModuleInfo.importedIds` outside a real build, so each + // static specifier is resolved individually via Vite's own resolution instead — a + // correct 1:1 correspondence by construction, since both sides derive from the same AST. const staticModuleSources = getStaticModuleSources(ast); const importerFile = node.file; const resolutions = await Promise.all( diff --git a/packages/plugins/apps/src/vite/dev-server.integration.test.ts b/packages/plugins/apps/src/vite/dev-server.integration.test.ts index ed7649224..0d8b8291a 100644 --- a/packages/plugins/apps/src/vite/dev-server.integration.test.ts +++ b/packages/plugins/apps/src/vite/dev-server.integration.test.ts @@ -3,26 +3,22 @@ // Copyright 2019-Present Datadog, Inc. /** - * Real coverage for the local-execution path's module resolution: no mocked - * `viteBuild`/`loadModule`, no hand-written stand-in module. This spins up - * a real Vite dev server (`createServer`, middleware mode — no port bound) - * rooted at the same `apps_backend_project` fixture `backend/integration.test.ts` - * uses, and lets its real `ssrLoadModule` import a real `.backend.ts` file - * directly and execute it via the real `/__dd/executeAction` HTTP handler, - * including resolving `@datadog/apps-backend` from the fixture's own project - * root rather than build-plugins' own dependency tree. + * Real coverage for the local-execution path's module resolution — no mocked + * `viteBuild`/`loadModule`, no hand-written stand-in module. Spins up a real Vite dev + * server (`createServer`, middleware mode, no port bound) rooted at the same + * `apps_backend_project` fixture `backend/integration.test.ts` uses, and lets its real + * `ssrLoadModule` import and execute a real `.backend.ts` file via the real + * `/__dd/executeAction` handler, including resolving `@datadog/apps-backend` from the + * fixture's own project root. * - * The nested-import test below registers the real `getVitePlugin()` hooks - * on this server, needed specifically to exercise `resolveId`'s - * `LOCAL_EXECUTION_LOAD_SUFFIX` propagation against Vite's own real module - * resolution, which a mocked `this.resolve()` can't reproduce. + * The nested-import test below registers the real `getVitePlugin()` hooks on this server + * to exercise `resolveId`'s `LOCAL_EXECUTION_LOAD_SUFFIX` propagation against Vite's actual + * module resolution, which a mocked `this.resolve()` can't reproduce. * - * `@datadog/apps-backend` and `@datadog/action-catalog` are both real, - * locally-resolvable fixture packages — see `packages/tests/src/_jest/ - * fixtures/node_modules/@datadog/apps-backend` and `.../@datadog/action-catalog` - * — rather than mocked modules, so the connection-ID coverage below exercises - * Vite's actual SSR transform output, not a hand-crafted AST shape a real - * transform would never produce. + * `@datadog/apps-backend` and `@datadog/action-catalog` are real, locally-resolvable + * fixture packages (not mocked), so the connection-ID coverage below exercises Vite's + * actual SSR transform output rather than a hand-crafted AST shape a real transform would + * never produce. */ import { getAuthenticatedRequest } from '@dd/apps-plugin/auth'; @@ -145,12 +141,10 @@ describe('Dev Server Middleware — real end-to-end local execution', () => { logLevel: 'silent', server: { middlewareMode: true, hmr: false }, plugins: [appsPlugin], - // Local execution only ever goes through ssrLoadModule, never the - // client bundle — Vite's auto-crawl-and-pre-bundle step exists for - // the browser path this feature never uses, and a fixture-only - // dependency (like the fake @datadog/action-catalog package below) - // can trip it up in ways that have nothing to do with what this - // suite is actually testing. + // Local execution only ever goes through ssrLoadModule, never the client bundle, so + // Vite's auto-crawl-and-pre-bundle step (for the browser path) is disabled — a + // fixture-only dependency like the fake @datadog/action-catalog package below can + // otherwise trip it up in ways unrelated to what this suite tests. optimizeDeps: { noDiscovery: true }, }); }); @@ -197,14 +191,11 @@ describe('Dev Server Middleware — real end-to-end local execution', () => { }); }, 30000); - // Real coverage for the resolveId propagation fix in vite/index.ts: - // nestedImport.backend.ts statically imports plainEcho from - // getRuntimeUsers.backend.ts. Without propagating - // LOCAL_EXECUTION_LOAD_SUFFIX onto that nested import, Vite would - // resolve it unsuffixed, the transform hook would replace it with the - // frontend RPC-proxy stub (calling globalThis.DD_APPS_RUNTIME, which - // doesn't exist server-side), and this would throw instead of returning - // the real value. + // Real coverage for resolveId's LOCAL_EXECUTION_LOAD_SUFFIX propagation: nestedImport.backend.ts + // statically imports plainEcho from getRuntimeUsers.backend.ts. Without propagating the suffix + // onto that nested import, Vite would resolve it unsuffixed, the transform hook would swap in + // the frontend RPC-proxy stub (calling the server-nonexistent globalThis.DD_APPS_RUNTIME), and + // this would throw instead of returning the real value. test('Should preserve real code for a nested *.backend.ts import, not swap it for the frontend RPC-proxy stub', async () => { const auth: AuthOptionsWithDefaults = { apiKey: 'test-api-key', @@ -237,16 +228,12 @@ describe('Dev Server Middleware — real end-to-end local execution', () => { expect(body.result).toEqual({ data: { value: 'nested-value' } }); }, 30000); - // Real coverage for the multi-hop case the single-hop propagation above - // still misses: viaHelper.backend.ts imports helper.ts (a plain, - // non-backend module), which itself imports plainEcho from - // getRuntimeUsers.backend.ts. resolveId only appended the suffix when - // the DIRECT importer string ended with it, so helper.ts (reached - // through a suffixed importer, but never suffixed itself, since it - // isn't a *.backend.ts file) became an unsuffixed importer for its own - // import — silently dropping the marker one hop later than the direct - // backend-to-backend case above, and swapping getRuntimeUsers.backend.ts - // for its frontend RPC-proxy stub. + // Real coverage for the multi-hop case the single-hop test above doesn't exercise: + // viaHelper.backend.ts imports helper.ts (plain, non-backend), which imports plainEcho from + // getRuntimeUsers.backend.ts. Suffix propagation must follow the chain through helper.ts + // (reached via a suffixed importer, but never itself suffixed) — otherwise helper.ts becomes + // an unsuffixed importer one hop past the direct case, and getRuntimeUsers.backend.ts + // resolves to its frontend RPC-proxy stub instead of its real code. test('Should preserve real code for a *.backend.ts import reached through an intermediate non-backend module', async () => { const auth: AuthOptionsWithDefaults = { apiKey: 'test-api-key', @@ -279,22 +266,15 @@ describe('Dev Server Middleware — real end-to-end local execution', () => { expect(body.result).toEqual({ data: { value: 'via-helper-value' } }); }, 30000); - // Regression coverage for the real configureServer-installed middleware, - // not a hand-built one: every other test in this file constructs its own - // middleware via createDevServerMiddleware(..., () => [], ...), which - // bypasses getAllowedConnectionIds' real wiring entirely (a hardcoded - // () => [] never exercises collectModuleGraphFromServer at all). Sending - // the request through server.middlewares — the real Connect stack - // getVitePlugin's own configureServer hook installed when this file's - // createServer() call ran — is what actually proves the fix: before it, - // getAllowedConnectionIds threw "missing module record" for the entry - // module itself on every call, since moduleParsed (a Rollup-build-only - // hook) never fires on a real Vite dev server. + // Regression coverage for the real configureServer-installed middleware, not the hand-built + // one every other test in this file uses via createDevServerMiddleware(..., () => [], ...), + // which bypasses getAllowedConnectionIds' real wiring entirely. Sending the request through + // server.middlewares — the real Connect stack getVitePlugin's configureServer hook installs — + // is what actually exercises getAllowedConnectionIds resolving the entry's record without + // moduleParsed, a Rollup-build-only hook that never fires on a real dev server. test('Should execute successfully through the real configureServer-installed middleware, walking a real multi-hop import graph', async () => { - // Registers viaHelperFunc in the real backend-function registry — - // configureServer's real middleware looks functions up there, and - // registration is itself a side effect of transforming the file as - // a normal (unsuffixed) frontend import, exactly like a real + // Registers viaHelperFunc in the real backend-function registry — a side effect of + // transforming the file as a normal (unsuffixed) frontend import, exactly like a real // frontend entry point importing the generated client SDK would. await server.ssrLoadModule(viaHelperFunc.absolutePath); @@ -313,20 +293,16 @@ describe('Dev Server Middleware — real end-to-end local execution', () => { expect(body.result).toEqual({ data: { value: 'real-middleware-value' } }); }, 30000); - // Regression coverage for a real getAllowedConnectionIds wired exactly as - // vite/index.ts's configureServer builds it, on the very first request - // for an entry — no priming import beforehand. Uses noSdkFunc, which no - // earlier test in this file touches: reusing nestedImportFunc or - // viaHelperFunc here would already have a warm moduleGraph node (and - // module-runner cache) left over from an earlier test against this same - // shared beforeAll server, masking the real gap this test guards — on a - // cold entry, Vite only ever registers the node under the fully-resolved - // (suffixed) id handleExecuteAction's own loadModule call just produced, - // and collectModuleGraphFromServer was looking it up by the bare path. + // Regression coverage for a real getAllowedConnectionIds, wired exactly as vite/index.ts's + // configureServer builds it, on the very first request for an entry — no priming import + // beforehand. Uses noSdkFunc since any other function here would already have a warm + // moduleGraph node from an earlier test against this shared beforeAll server, masking the + // invariant under test: on a cold entry, Vite only registers the node under the fully-resolved + // (suffixed) id, so collectModuleGraphFromServer must look it up by that id, not the bare path. test('Should compute allowed connection IDs on the very first request for an entry, with no prior priming import', async () => { const loadModule = server.ssrLoadModule.bind(server); - // collectModuleGraphFromServer now appends LOCAL_EXECUTION_LOAD_SUFFIX internally, - // so this closure only ever handles the bare id — matching vite/index.ts's real wiring. + // collectModuleGraphFromServer appends LOCAL_EXECUTION_LOAD_SUFFIX internally, so this + // closure only handles the bare id — matching vite/index.ts's real wiring. const getAllowedConnectionIds = async (entryId: string) => extractConnectionIdsFromModuleGraph( entryId, @@ -365,19 +341,14 @@ describe('Dev Server Middleware — real end-to-end local execution', () => { expect(body.result).toEqual({ data: { ok: true } }); }, 30000); - // Regression coverage for the connection-ID collector against a REAL - // Vite SSR transform, not a hand-crafted AST fixture. Vite's SSR - // transform rewrites every `import` into a `__vite_ssr_import__(...)` - // call and resolves bare specifiers to absolute paths — neither shape - // the plain-`ImportDeclaration` search in collectActionCatalogImports - // (built for the untransformed syntax a real Rollup build sees) can - // parse. Before collectModuleGraphFromServer started reading each - // module's original source fresh from disk instead, this collector - // silently returned an empty allowlist for every locally-executed - // function that imports a typed action-catalog function — rejecting any - // real connectionId-scoped call with "not in this function's allowed - // connections: []", not because of a real access violation, but because - // the collector could never see the import that should have allowed it. + // Regression coverage for the connection-ID collector against a REAL Vite SSR transform, not a + // hand-crafted AST fixture. Vite's SSR transform rewrites imports into + // `__vite_ssr_import__(...)` calls the plain-`ImportDeclaration` search in + // collectActionCatalogImports can't parse — before collectModuleGraphFromServer started + // reading each module's original source from disk instead, this silently returned an empty + // allowlist for any function importing a typed action-catalog function, rejecting real calls + // with "not in this function's allowed connections: []" for lack of visibility, not a real + // access violation. test('Should recognize a connectionId-scoped action-catalog call and allow it, not silently reject it', async () => { const loadModule = server.ssrLoadModule.bind(server); const getAllowedConnectionIds = async (entryId: string) => @@ -403,10 +374,9 @@ describe('Dev Server Middleware — real end-to-end local execution', () => { getMockLogger(), ); - // The connection-ID collector is the thing under test here, not the - // real preview-async round trip (already covered by dev-server.test.ts - // and local-execution.test.ts) — this only needs the request to get - // past the allowedConnectionIds check, so a minimal reply is enough. + // The connection-ID collector is under test here, not the preview-async round trip + // (already covered elsewhere) — the request just needs to pass the allowedConnectionIds + // check, so a minimal reply is enough. const apiScope = nock('https://api.datadoghq.com') .post('/api/v2/app-builder/queries/preview-async') .reply(200, { data: { id: 'receipt-action-catalog' } }) @@ -429,19 +399,11 @@ describe('Dev Server Middleware — real end-to-end local execution', () => { expect(apiScope.isDone()).toBe(true); }, 30000); - // Coverage for a module mixing a static import with a top-level dynamic - // import textually between two static ones. dev-server-module-graph.ts - // now resolves each static specifier individually against the AST - // instead of reading node.importedModules positionally (which mixes - // static and dynamic imports with no documented ordering guarantee) — - // this fixture exercises that resolution path directly. Note: verified - // against a real Vite dev server that node.importedModules for an - // SSR-loaded module doesn't actually include a non-local dynamic - // import's target at all, so the specific silent-misattribution failure - // this was meant to reproduce doesn't manifest on the old code either; - // the new resolution approach is kept regardless since it's correct by - // construction rather than relying on node.importedModules' undocumented - // behavior. + // Coverage for a module mixing a static import with a top-level dynamic import textually + // between two static ones — exercises dev-server-module-graph.ts resolving each static + // specifier individually against the AST, independent of node.importedModules's undocumented + // ordering for mixed static/dynamic imports, so the resolution stays correct by construction + // regardless of what that ordering happens to be for a given module. test('Should recognize a connectionId-scoped action-catalog call even when a top-level dynamic import sits between two static imports', async () => { const loadModule = server.ssrLoadModule.bind(server); const getAllowedConnectionIds = async (entryId: string) => diff --git a/packages/plugins/apps/src/vite/dev-server.test.ts b/packages/plugins/apps/src/vite/dev-server.test.ts index afc12c238..88b634c35 100644 --- a/packages/plugins/apps/src/vite/dev-server.test.ts +++ b/packages/plugins/apps/src/vite/dev-server.test.ts @@ -29,20 +29,12 @@ jest.mock('@dd/core/helpers/oauth-request', () => ({ }), })); -/** - * Shape of the `$.Actions` dynamic proxy — an arbitrarily-nested property - * path (e.g. `$.Actions.slack.chat.postMessage`) that's callable at any - * depth. Used to type `globalThis.$` in tests without an `any` cast. - */ +/** Shape of the `$.Actions` dynamic proxy — a nested property path (e.g. `$.Actions.slack.chat.postMessage`) callable at any depth; types `globalThis.$` in tests without an `any` cast. */ type ActionsProxy = { [key: string]: ActionsProxy } & ((...args: unknown[]) => Promise); const mockViteBuild = jest.fn(); -/** - * Stands in for the real `server.ssrLoadModule` — the local executeAction - * path doesn't bundle, so tests exercising it configure this directly - * instead of `mockBuildWithParsedBackend`. - */ +/** Stands in for the real `server.ssrLoadModule` — the local executeAction path doesn't bundle, so tests exercising it configure this directly instead of `mockBuildWithParsedBackend`. */ const mockLoadModule = jest.fn(); const DD_API_ORIGIN = 'https://api.datadoghq.com'; @@ -185,6 +177,43 @@ describe('Dev Server Middleware', () => { nock.cleanAll(); }); + describe('startup auth warning', () => { + test('Should warn that both executeAction endpoints will be unavailable when auth is not configured', () => { + createDevServerMiddleware( + mockViteBuild, + mockLoadModule, + () => mockFunctions, + async () => [], + mockAuth, + undefined, + '/project', + mockLog, + ); + + expect(mockLogFn).toHaveBeenCalledWith( + expect.stringContaining( + 'Both the /__dd/executeAction and /__dd/executeActionViaCloud endpoints will be unavailable', + ), + 'warn', + ); + }); + + test('Should not warn when auth is configured', () => { + createDevServerMiddleware( + mockViteBuild, + mockLoadModule, + () => mockFunctions, + async () => [], + mockAuth, + getApiKeyRequest(), + '/project', + mockLog, + ); + + expect(mockLogFn).not.toHaveBeenCalledWith(expect.anything(), 'warn'); + }); + }); + describe('createDevServerMiddleware routing', () => { const middleware = createDevServerMiddleware( mockViteBuild, @@ -938,11 +967,9 @@ describe('Dev Server Middleware', () => { expect(res.statusCode).toBe(200); const body = JSON.parse(res.getBody()); expect(body.success).toBe(true); - // The action's raw output ({ok, ts}, its own schema, not wrapped - // by preview-async itself) is what $.Actions.foo.bar() resolves - // to; the outer {data: ...} comes from the function's own return - // value going through executeScriptLocally's usual wrapping, not - // from anything action-specific. + // The action's raw output ({ok, ts}) is what $.Actions.foo.bar() resolves to; the + // outer {data: ...} comes from executeScriptLocally's usual return-value wrapping, + // not anything action-specific. expect(body.result).toEqual({ data: { ok: true, ts: '123' } }); expect(apiScope.isDone()).toBe(true); expect(capturedBody?.data.attributes.query.properties.spec).toEqual({ @@ -1023,20 +1050,14 @@ describe('Dev Server Middleware', () => { ); }); - // The priming loadModule call (see handleExecuteAction) is the only - // place the entry's top-level code actually runs — Vite caches the - // module, so executeScriptLocally's own load below just reuses this - // same resolved object — so it must carry the same $-scoping - // guarantee executeScriptLocally's load would otherwise provide. - test('Should throw when a customer module reaches for $ during its own top-level evaluation, even though the module-graph priming load runs before executeScriptLocally installs its own scoping', async () => { + // The priming loadModule call (see handleExecuteAction) is the only place the entry's + // top-level code runs (Vite caches the module for executeScriptLocally's reuse), so it + // must carry the same $-scoping guarantee executeScriptLocally's own load would provide. + test('Should read $ as undefined when a customer module reaches for it during its own top-level evaluation, even though the module-graph priming load runs before executeScriptLocally installs its own scoping', async () => { let dollarDuringTopLevelLoad: unknown = 'not captured'; mockLoadModule.mockImplementation(async (specifier: string) => { if (specifier === mockFunctions[0].absolutePath + LOCAL_EXECUTION_LOAD_SUFFIX) { - try { - dollarDuringTopLevelLoad = (globalThis as Record).$; - } catch (error) { - dollarDuringTopLevelLoad = error; - } + dollarDuringTopLevelLoad = (globalThis as Record).$; return { [mockFunctions[0].name]: () => 'done' }; } throw new Error(`Cannot find module '${specifier}'`); @@ -1052,22 +1073,18 @@ describe('Dev Server Middleware', () => { await res.done; expect(res.statusCode).toBe(200); - expect(dollarDuringTopLevelLoad).toBeInstanceOf(Error); - expect((dollarDuringTopLevelLoad as Error).message).toBe( - 'No active local execution to resolve $ under.', - ); + expect(dollarDuringTopLevelLoad).toBeUndefined(); }); - // Guards the priming loadModule call (see handleExecuteAction) — it - // evaluates the entry's real top-level code before - // executeScriptLocally's own hang-detection timeout is installed, so - // a customer module with a hanging top-level await would otherwise - // wedge this request forever with no bound at all. + // Guards the priming loadModule call (see handleExecuteAction) — it evaluates the entry's + // real top-level code before executeScriptLocally's hang-detection timeout is installed, + // so a hanging top-level await would otherwise wedge this request forever. test('Should eventually time out and return a clear error when the priming load never settles', async () => { jest.useFakeTimers(); try { mockLoadModule.mockImplementation( - () => new Promise(() => {}), // never settles + // Never settles. + () => new Promise(() => {}), ); const req = createMockRequest('/__dd/executeAction', { @@ -1079,10 +1096,9 @@ describe('Dev Server Middleware', () => { middleware(req, res, jest.fn()); const doneAssertion = res.done; - // createMockRequest emits the body via a real process.nextTick, - // which fake timers don't advance — drain it first so the - // priming load's own setTimeout is actually scheduled before - // runAllTimersAsync tries to advance past it. + // createMockRequest emits the body via a real process.nextTick, which fake timers + // don't advance — drain it first so the priming load's setTimeout is scheduled + // before runAllTimersAsync tries to advance past it. await jest.advanceTimersByTimeAsync(0); await jest.runAllTimersAsync(); await doneAssertion; @@ -1095,10 +1111,9 @@ describe('Dev Server Middleware', () => { } }); - // Guards getAllowedConnectionIds — it reads every reachable module - // from disk and transforms it (dev-server-module-graph.ts), with no - // bound of its own, unlike the sibling priming load right next to it - // in handleExecuteAction which already has one. + // Guards getAllowedConnectionIds — it reads and transforms every reachable module from + // disk (dev-server-module-graph.ts) with no bound of its own, unlike the sibling priming + // load next to it in handleExecuteAction. test('Should eventually time out and return a clear error when getAllowedConnectionIds never settles', async () => { jest.useFakeTimers(); try { @@ -1107,7 +1122,8 @@ describe('Dev Server Middleware', () => { mockViteBuild, mockLoadModule, () => mockFunctions, - () => new Promise(() => {}), // never settles + // Never settles. + () => new Promise(() => {}), mockAuth, getApiKeyRequest(), '/project', diff --git a/packages/plugins/apps/src/vite/dev-server.ts b/packages/plugins/apps/src/vite/dev-server.ts index 263cee24c..04f9af5f4 100644 --- a/packages/plugins/apps/src/vite/dev-server.ts +++ b/packages/plugins/apps/src/vite/dev-server.ts @@ -128,11 +128,10 @@ async function bundleBackendFunction( } /** - * Submit a query to Datadog's app-builder `preview-async` endpoint and - * return its receipt ID. `querySpec` is the query's own `spec` object — - * either the `jsFunctionWithActions` wrapper (a whole script) or a single - * real action's own `{fqn, inputs}` directly (see `makeExecuteActionRemotely` - * below) — `submitQuery` itself doesn't care which. + * Submits a query to Datadog's app-builder `preview-async` endpoint and returns its + * receipt ID. `querySpec` is the query's own `spec` — either the `jsFunctionWithActions` + * wrapper or a single action's `{fqn, inputs}` (see `makeExecuteActionRemotely`) — + * `submitQuery` doesn't care which. */ async function submitQuery( querySpec: Record, @@ -184,11 +183,7 @@ async function submitQuery( return receiptId; } -/** - * Execute a script via Datadog's app-builder queries API — the existing - * production round trip, unchanged. Wraps the whole script as a - * `jsFunctionWithActions` query. - */ +/** Executes a script via Datadog's app-builder queries API — the production round trip, wrapping the whole script as a `jsFunctionWithActions` query. */ async function executeScriptViaDatadog( scriptBody: string, func: BackendFunction, @@ -221,7 +216,7 @@ async function executeScriptViaDatadog( return outputs; } -/** Submits a single-action `preview-async` query per `$.Actions` call and logs its result/error, since production's own equivalent signal only reaches Datadog's backend, not the developer's `npm run dev` console. Callers must already have confirmed auth is configured — see `createDevServerMiddleware`'s upfront check on this route. */ +/** Submits a single-action `preview-async` query per `$.Actions` call and logs its result/error, since production's equivalent signal never reaches the `npm run dev` console. Callers must have already confirmed auth is configured (see `createDevServerMiddleware`). */ function makeExecuteActionRemotely( auth: AuthConfig, doAuthenticatedRequest: DoAuthenticatedRequest, @@ -257,17 +252,12 @@ interface PollResult { } /** - * Long-poll Datadog API until a submitted query's execution completes or - * times out. Returns the raw `outputs` value — shape varies by query type - * (a `jsFunctionWithActions` query wraps its result as `{data: }`; - * a direct single-action query's `outputs` is that action's own defined - * output schema) — callers interpret it accordingly. - * - * The server holds each poll connection open (~30s) and responds with - * done: true when the result is ready, or done: false when its long-poll - * window expires. This loop handles application-level re-polling - * (done: false), not HTTP retries — doRequest already retries transient - * HTTP failures (5xx, network errors) internally. + * Long-polls Datadog API until a submitted query's execution completes or times out, + * returning the raw `outputs` value — shape varies by query type (`jsFunctionWithActions` + * wraps as `{data: }`; a single-action query's `outputs` is that action's own schema), + * so callers interpret it accordingly. The server holds each poll open ~30s and responds + * `done: false` on timeout; this loop only handles that application-level re-polling, since + * `doRequest` already retries transient HTTP failures internally. */ async function pollQueryExecution( receiptId: string, @@ -385,11 +375,9 @@ async function handleDebugBundle( } /** - * Handle POST /__dd/executeAction — imports a backend function's real file - * directly and executes it in-process (see local-execution.ts); no bundling - * on this path. Auth is required upfront (checked by the caller in - * createDevServerMiddleware before this is reached), matching production's - * own auth-before-execution ordering. + * Handles POST /__dd/executeAction — imports a backend function's real file and executes + * it in-process (see local-execution.ts), with no bundling. Auth is checked upfront by the + * caller in `createDevServerMiddleware`, matching production's auth-before-execution ordering. */ async function handleExecuteAction( req: IncomingMessage, @@ -408,38 +396,31 @@ async function handleExecuteAction( log.debug(`Executing action locally: ${displayName} with args`); - // The registry's own `func.allowedConnectionIds` is always `[]` here - // — only the bundling collector populates it, and this path - // intentionally skips bundling. Loading the entry once first lets - // the module-graph collector observe Vite's `server.moduleGraph` for - // this entry (see collectModuleGraphFromServer), so the connection-ID - // allowlist reflects the function's actual imports instead of being - // silently empty. This priming load runs before executeScriptLocally - // installs its own hang-detection timeout below, and it evaluates the - // entry's real top-level code (ssrLoadModule, not a parse-only step) - // — so it needs its own bound, or a customer module with a hanging - // top-level await would wedge this request forever. It's also the - // only place that top-level code actually runs (Vite caches the - // module, and executeScriptLocally below reuses this same resolved - // object rather than re-evaluating it), so it must go through - // loadCustomerModuleEntry's scoping rather than a raw loadModule call - // — otherwise a customer module reaching for $ during its own - // top-level evaluation would silently resolve to whatever `$` a - // prior execution happened to leave behind, instead of throwing the - // same way it does inside executeScriptLocally. + // The bundling collector is what normally populates func.allowedConnectionIds, but this + // path skips bundling — priming the entry through Vite's moduleGraph first (see + // collectModuleGraphFromServer) is what makes the allowlist reflect the function's real + // imports instead of staying empty. Runs before executeScriptLocally's hang-detection + // timeout, and evaluates the entry's real top-level code (not a parse-only step) — needs + // its own bound, or a hanging top-level await wedges the request forever. Also the only + // place top-level code runs (Vite caches the module), so it must use + // loadCustomerModuleEntry's $-scoping, not a raw loadModule call, or a customer module's + // top-level $ access would resolve to a stale value carried over from outside any + // execution, instead of the undefined a fresh top-level access should see. const entrySpecifier = func.absolutePath + LOCAL_EXECUTION_LOAD_SUFFIX; + const primingLoadPromise = loadCustomerModuleEntry(loadModule, entrySpecifier); const primedModule = await withTimeout( - loadCustomerModuleEntry(loadModule, entrySpecifier), + primingLoadPromise, DEFAULT_TIMEOUT_MS, `Loading "${displayName}"`, ); // Same reasoning as the priming load above: this reads every reachable module from disk and // transforms it, with no bound of its own — a stalled file read or a hung esbuild.transform // call would otherwise wedge this request forever. + const allowedConnectionIdsPromise = getAllowedConnectionIds(func.absolutePath); const funcWithConnectionIds: BackendFunction = { ...func, allowedConnectionIds: await withTimeout( - getAllowedConnectionIds(func.absolutePath), + allowedConnectionIdsPromise, DEFAULT_TIMEOUT_MS, `Resolving allowed connections for "${displayName}"`, ), @@ -473,10 +454,9 @@ async function handleExecuteAction( } /** - * Handle POST /__dd/executeActionViaCloud — bundles a backend function and - * executes it via the existing production round trip (queue + Deno - * subprocess). Kept as a distinctly-purposed command (`npm run dev:verify`) - * for pre-publish parity checks, not a mode flag on the same endpoint. + * Handles POST /__dd/executeActionViaCloud — bundles a backend function and executes it via + * the production round trip (queue + Deno subprocess), kept as its own endpoint + * (`npm run dev:verify`) for pre-publish parity checks rather than a mode flag. */ async function handleExecuteActionViaCloud( req: IncomingMessage, @@ -567,7 +547,7 @@ export function createDevServerMiddleware( sendError(res, 500, 'Unexpected error'); }); } else if (req.url === '/__dd/executeAction') { - // Matches production, which authenticates before any backend-function code runs (app-builder-api's PreviewAsyncQueryHandler checks the user first, before the query/execution path) — checked here upfront rather than only lazily inside a $.Actions call, so a function that never calls $.Actions isn't a loophole around the same requirement. A local presence check only (not a real credential-validation network call), so it costs no latency on the fast local dev loop. + // Matches production's auth-before-execution ordering (PreviewAsyncQueryHandler checks the user first) — checked upfront here, not lazily inside a $.Actions call, so a function that never calls $.Actions isn't a loophole. Just a local presence check, not a credential-validation network call, so it adds no latency to the dev loop. if (!doAuthenticatedRequest) { sendError(res, 400, `Auth credentials not configured. ${AUTH_GUIDANCE}`); return; diff --git a/packages/plugins/apps/src/vite/index.test.ts b/packages/plugins/apps/src/vite/index.test.ts index adced4f6d..96bc0c922 100644 --- a/packages/plugins/apps/src/vite/index.test.ts +++ b/packages/plugins/apps/src/vite/index.test.ts @@ -279,11 +279,10 @@ describe('Backend Functions - getVitePlugin', () => { }); test('Should force @datadog/apps-backend and @datadog/action-catalog through the SSR transform pipeline instead of externalizing them', () => { - // These SDKs ship ESM-only. Vite's dev-server SSR mode externalizes - // node_modules by default (a plain require(), for speed), which - // throws "Cannot use import statement outside a module" for an - // ESM-only package -- ssr.noExternal is what the local executeAction - // path's server.ssrLoadModule call depends on to load them correctly. + // These SDKs ship ESM-only, but Vite's dev-server SSR mode externalizes node_modules by + // default (a plain require()), which throws "Cannot use import statement outside a + // module" for them — ssr.noExternal is what server.ssrLoadModule depends on to load them + // correctly. const plugin = getVitePlugin(defaultOptions); const configHook = plugin!.config as () => { ssr: { noExternal: string[] } }; const config = configHook(); diff --git a/packages/plugins/apps/src/vite/index.ts b/packages/plugins/apps/src/vite/index.ts index 8080e131e..54a92dd5f 100644 --- a/packages/plugins/apps/src/vite/index.ts +++ b/packages/plugins/apps/src/vite/index.ts @@ -124,26 +124,19 @@ export const getVitePlugin = ({ const { setBackendFunctions, getBackendFunctions } = createBackendFunctionRegistry(); - // Non-backend module IDs reached transitively from a suffixed backend - // entry point. A plain helper module's own id can't carry - // LOCAL_EXECUTION_LOAD_SUFFIX (it's never ambiguous between a proxy and - // real code, so it needs no suffix), but it still needs to be recognized - // as an importer belonging to the suffixed subgraph — otherwise a - // *.backend.ts file reached through it (rather than directly from - // another *.backend.ts file) would lose the marker one hop later than a - // direct backend-to-backend import does. + // Non-backend module ids reached transitively from a suffixed backend entry point. A plain + // helper module's id never carries LOCAL_EXECUTION_LOAD_SUFFIX itself (it's never ambiguous + // between a proxy and real code), but must still be recognized as part of the suffixed + // subgraph — otherwise a *.backend.ts file reached through it would lose the marker one hop + // later than a direct backend-to-backend import does. const suffixedSubgraphImporters = new Set(); return { - // The dev server's local-execution path loads backend-function - // dependencies (e.g. @datadog/apps-backend, @datadog/action-catalog) - // via `server.ssrLoadModule`, which by default externalizes - // node_modules packages (a plain `require()`, for speed) rather than - // transforming them. Those two SDKs ship ESM-only, so an externalized - // `require()` throws "Cannot use import statement outside a module". - // `ssr.noExternal` forces Vite's SSR transform pipeline to handle - // them instead, matching how the production bundling path already - // inlines every dependency by default. + // @datadog/apps-backend and @datadog/action-catalog ship ESM-only, but server.ssrLoadModule + // externalizes node_modules by default (a plain require(), for speed), which throws + // "Cannot use import statement outside a module" for them. ssr.noExternal forces Vite's + // SSR transform pipeline to handle them instead, matching how production bundling already + // inlines every dependency. config() { return { ssr: { @@ -151,38 +144,26 @@ export const getVitePlugin = ({ }, }; }, - // Propagates the LOCAL_EXECUTION_LOAD_SUFFIX marker through the - // backend-file dependency graph: `ssrLoadModule` only tags the one - // entry module it's called with, so without this hook, a - // `.backend.ts` file statically importing another `.backend.ts` file - // would resolve that nested import unsuffixed, hitting transform's - // "not suffixed" branch below and getting replaced with the frontend - // RPC-proxy stub — breaking local execution for a multi-backend-file - // import graph. Propagates whenever the importer itself was suffixed - // OR is a previously-seen non-backend module reached from within the - // suffixed subgraph (see `suffixedSubgraphImporters` above) — this is - // local execution's own module graph either way, not a regular - // frontend import — and only appends the suffix onto another - // `.backend.ts` file (a plain helper module never hits the - // proxy-vs-real-code branching this marker exists to disambiguate, - // so it needs no suffix of its own). + // Propagates the LOCAL_EXECUTION_LOAD_SUFFIX marker through the backend-file dependency + // graph: ssrLoadModule only tags the one entry module it's called with, so without this + // hook a `.backend.ts` file importing another `.backend.ts` file would resolve that nested + // import unsuffixed and get replaced with the frontend RPC-proxy stub, breaking local + // execution for a multi-backend-file graph. Propagates whenever the importer was itself + // suffixed, or is a previously-seen non-backend module reached from within the suffixed + // subgraph (see `suffixedSubgraphImporters`) — and only appends the suffix onto another + // `.backend.ts` file, since a plain helper module never hits the proxy-vs-real-code branch + // this marker disambiguates. resolveId: { - // Must run before Vite's own built-in resolver: a plain relative - // specifier like `./other.backend` is fully resolvable by Vite's - // internal filesystem-based resolution alone, which — running at - // its default, unenforced order — would resolve and short-circuit - // the hook chain before this plugin's own resolveId ever saw it. + // Must run before Vite's built-in resolver: a plain relative specifier like + // `./other.backend` is fully resolvable by Vite's own filesystem resolution, which at + // its default order would short-circuit the hook chain before this plugin ever saw it. // `pre` guarantees this hook gets first look at every id. order: 'pre', async handler(source, importer, resolveOptions) { - // Scoped to `resolveOptions.ssr`: local execution's own - // traversal is always an SSR resolution (it runs through - // `server.ssrLoadModule`), so a helper's id recorded here - // must only count for a later SSR-context resolution too — - // otherwise the same helper subsequently reached from the - // ordinary (non-SSR) client graph would inherit the marker - // and serve real backend code to the browser instead of the - // frontend RPC-proxy stub. + // Scoped to `resolveOptions.ssr` since local execution's traversal is always an + // SSR resolution — otherwise the same helper reached later from the ordinary + // client graph would inherit the marker and serve real backend code to the + // browser instead of the frontend RPC-proxy stub. const isPartOfSuffixedSubgraph = !!importer && (importer.endsWith(LOCAL_EXECUTION_LOAD_SUFFIX) || @@ -302,14 +283,11 @@ export const getVitePlugin = ({ } const loadModule = server.ssrLoadModule.bind(server); - // Call only after `loadModule` has resolved for this same entryId (plus its - // LOCAL_EXECUTION_LOAD_SUFFIX) — the graph read below is a live side effect of - // that call, not independently maintained state (moduleParsed, the mechanism the - // production bundling path uses instead, never fires during a real Vite dev - // server — it's a Rollup-build-only hook). collectModuleGraphFromServer owns - // appending the suffix internally, so this closure only ever handles the bare - // backend-file path — the same shape extractConnectionIdsFromModuleGraph needs - // to key into the returned records map. + // Call only after `loadModule` has resolved for this same entryId — the graph read + // below is a live side effect of that call, not independently maintained state + // (moduleParsed, production's mechanism, is a Rollup-build-only hook that never + // fires on a real dev server). collectModuleGraphFromServer appends + // LOCAL_EXECUTION_LOAD_SUFFIX internally, so this closure only handles the bare path. const getAllowedConnectionIds = async (entryId: string) => extractConnectionIdsFromModuleGraph( entryId, diff --git a/packages/plugins/apps/src/vite/local-execution.test.ts b/packages/plugins/apps/src/vite/local-execution.test.ts index 4332cfd77..4b7994ed7 100644 --- a/packages/plugins/apps/src/vite/local-execution.test.ts +++ b/packages/plugins/apps/src/vite/local-execution.test.ts @@ -42,11 +42,7 @@ beforeEach(() => { jest.spyOn(shared, 'isDatadogAppsBackendInstalled').mockReturnValue(false); }); -/** - * Shape of the `$.Actions` dynamic proxy — an arbitrarily-nested property - * path (e.g. `$.Actions.slack.chat.postMessage`) that's callable at any - * depth. Used to type `globalThis.$` in tests without an `any` cast. - */ +/** Shape of the `$.Actions` dynamic proxy — a nested property path (e.g. `$.Actions.slack.chat.postMessage`) callable at any depth; types `globalThis.$` in tests without an `any` cast. */ type ActionsProxy = { [key: string]: ActionsProxy } & ((...args: unknown[]) => Promise); const stubExecuteAction: ExecuteAction = async (fqn) => ({ data: null, stub: true, fqn }); @@ -545,7 +541,7 @@ describe('local-execution — executeScriptLocally', () => { ); }); - // executeAction stands in for dev-server.ts's real makeExecuteActionRemotely, whose long-poll can legitimately outlast a short hang-detection timeout — that's network wait time, not a hung customer function. + // executeAction stands in for dev-server.ts's makeExecuteActionRemotely, whose long-poll can legitimately outlast a short hang-detection timeout — that's network wait, not a hung function. test('Should not time out while a real $.Actions call is still legitimately in flight, even past the configured timeout', async () => { const slowExecuteAction: ExecuteAction = () => new Promise((resolve) => setTimeout(() => resolve({ ok: true }), 80)); @@ -564,21 +560,18 @@ describe('local-execution — executeScriptLocally', () => { }), }), mockLogger, - 50, // shorter than slowExecuteAction's own 80ms + // Shorter than slowExecuteAction's own 80ms. + 50, ); expect(result).toEqual({ data: { ok: true } }); }); - // Guards against the hang-detection timer staying paused forever: without - // a bound on the $.Actions call itself, a stalled network request (no - // abort signal/deadline of its own) would wedge this execution — and, - // since local executions are serialized via `enqueue`, every request - // queued behind it — indefinitely. The absolute execution ceiling - // (MAX_TOTAL_EXECUTION_TIMEOUT_MS, 6 minutes) now always fires before - // the per-call bound (MAX_ACTION_CALL_TIMEOUT_MS, 10 minutes) could, so - // that's the mechanism actually observed here — MAX_ACTION_CALL_TIMEOUT_MS - // remains a backstop for any path the ceiling doesn't cover. + // Guards against the hang-detection timer staying paused forever: without a bound on the + // $.Actions call itself, a stalled request would wedge this execution, and every request + // serialized behind it via `enqueue`, indefinitely. The absolute execution ceiling (6 minutes) + // always fires before the per-call bound (10 minutes) could, so that's the mechanism this test + // observes — the per-call bound remains a backstop for any path the ceiling doesn't cover. test('Should eventually time out an in-flight $.Actions call that never settles, and not wedge subsequently queued executions', async () => { jest.useFakeTimers(); try { @@ -624,7 +617,9 @@ describe('local-execution — executeScriptLocally', () => { } }); - // A fire-and-forget $.Actions call (not awaited by the customer function) increments pendingActionCalls the same as an awaited one, pausing the per-call hang-detection timer for as long as that call stays in flight — up to MAX_ACTION_CALL_TIMEOUT_MS (10 minutes) if the call never settles, even though the customer function itself moved on to something else entirely. The absolute execution ceiling below must still fire well before that. + // A fire-and-forget $.Actions call pauses the per-call hang-detection timer for as long as it + // stays in flight (up to 10 minutes), even though the customer function has moved on — the + // absolute execution ceiling below must still fire well before that. test('Should eventually time out via an absolute execution ceiling, independent of any $.Actions call still in flight', async () => { jest.useFakeTimers(); try { diff --git a/packages/plugins/apps/src/vite/local-execution.ts b/packages/plugins/apps/src/vite/local-execution.ts index 27b623a2f..b76112e55 100644 --- a/packages/plugins/apps/src/vite/local-execution.ts +++ b/packages/plugins/apps/src/vite/local-execution.ts @@ -112,16 +112,16 @@ function isIndexableRecord(value: unknown): value is Record { export const DEFAULT_TIMEOUT_MS = 10_000; -/** Bounds a single `$.Actions` call while it's exempt from the hang-detection timer above (see `guardedExecuteAction`). `doRequest` attaches no abort signal or deadline of its own, so an in-flight call that never settles would otherwise wedge this execution — and, since local executions are serialized, every request queued behind it — forever. Set generously past `pollQueryExecution`'s own worst-case long-poll budget (10 retries at up to ~30s each) so a legitimate slow action is never cut off. */ +/** Bounds a single `$.Actions` call while it's exempt from the hang-detection timer (see `guardedExecuteAction`) — `doRequest` has no deadline of its own, so an unsettled call would wedge this execution, and every serialized request queued behind it, forever. Set generously past `pollQueryExecution`'s worst-case long-poll budget (10 retries × ~30s) so a legitimately slow action is never cut off. */ const MAX_ACTION_CALL_TIMEOUT_MS = 10 * 60_000; -/** Absolute ceiling on one execution's total wall-clock time, independent of `pendingActionCalls`'s pause-and-extend mechanism (see `guardedExecuteAction`) — that mechanism can't distinguish a customer function genuinely awaiting a slow `$.Actions` call from one that fired a call without awaiting it and then hung on something unrelated, so an unawaited call currently masks a real hang for as long as MAX_ACTION_CALL_TIMEOUT_MS. Set just above `pollQueryExecution`'s own worst-case long-poll budget (10 retries at up to ~30s each, ~300s) so a legitimate single slow `$.Actions` call still always finishes — this bounds the masked-hang worst case to roughly 6 minutes instead of the full 10, not lower, since going lower would start killing real in-progress calls instead of just hangs. */ +/** Absolute ceiling on one execution's wall-clock time, independent of `pendingActionCalls`'s pause-and-extend mechanism (see `guardedExecuteAction`) — that mechanism can't tell a function genuinely awaiting a slow `$.Actions` call from one that fired-and-forgot a call and then hung on something else, so an unawaited call can mask a real hang for up to `MAX_ACTION_CALL_TIMEOUT_MS`. Set just above `pollQueryExecution`'s worst case (~300s) so one legitimate slow call still finishes, bounding the masked-hang case to ~6 minutes rather than the full 10 — going lower would start killing real in-progress calls instead of just hangs. */ const MAX_TOTAL_EXECUTION_TIMEOUT_MS = 6 * 60_000; /** Loads a module by specifier, resolved against the customer's own project rather than build-plugins' dependency tree — the dev server passes its Vite instance's `ssrLoadModule` here. */ export type LoadModule = (specifier: string) => Promise>; -/** Loads a customer module's entry under the same top-level-evaluation scoping `runScriptLocally`'s own load uses (see `customerModuleLoadContext`'s doc comment above) — for callers that need to trigger the entry's real top-level evaluation ahead of `executeScriptLocally` (e.g. dev-server.ts's module-graph priming load), so a customer module reaching for `$` during that load fails the same way it would inside `executeScriptLocally`, instead of silently resolving to whatever `globalDollarOutsideExecution` happens to hold. */ +/** Loads a customer module under the same top-level-evaluation `$`-scoping `runScriptLocally` uses (see `customerModuleLoadContext`) — for callers like dev-server.ts's priming load that trigger real top-level evaluation ahead of `executeScriptLocally`. */ export function loadCustomerModuleEntry( loadModule: LoadModule, entrySpecifier: string, @@ -219,7 +219,7 @@ function makeActionsProxy( }); } -/** Bounds a promise that would otherwise be able to hang forever — a registration's underlying `loadModule` call (a broken/circular module graph, not just a slow one), or a `$.Actions` call whose transport attaches no deadline of its own — so it rejects instead of leaving its caller waiting indefinitely. Doesn't cancel the underlying promise (not possible for a plain `Promise`), so it still runs its side effects late if it eventually does settle; see each call site's own doc comment for why that's harmless there. `label` is the full, already-attributed subject of the timeout message (e.g. `` `Loading ${specifier}` ``), not appended to a fixed prefix, so it reads naturally for both a load and an action call. */ +/** Bounds a promise that could otherwise hang forever — a `loadModule` call against a broken/circular graph, or a `$.Actions` call with no deadline of its own — rejecting instead of leaving the caller waiting indefinitely. Doesn't cancel the underlying promise (not possible for a plain `Promise`), so late side effects can still fire if it eventually settles; see each call site for why that's harmless there. `label` is the full, already-attributed subject of the timeout message (e.g. `` `Loading ${specifier}` ``), not a suffix on a fixed prefix, so it reads naturally for both loads and action calls. */ export function withTimeout(promise: Promise, timeoutMs: number, label: string): Promise { return new Promise((resolve, reject) => { const timer = setTimeout(() => { @@ -491,16 +491,11 @@ async function runScriptLocally( // A timed-out execution is abandoned, not canceled — its fn() may keep running and must not act under a newer execution's identity. isCurrent() gates both this execution's own captured `$.Actions` closure and the shared adapters, which resolve the calling execution's dispatch from AsyncLocalStorage rather than whichever registration is currently live. const scope = executionEpoch.start(); - // `executeAction`'s own long-poll (dev-server.ts's pollQueryExecution) - // can legitimately take far longer than `timeoutMs` on its own — that's - // time spent waiting on a real network round trip, not evidence the - // customer function itself has hung. Pausing the hang-detection timer - // while at least one call is in flight, and giving it a fresh - // `timeoutMs` window once every in-flight call has settled, means a - // function that keeps making real progress via `$.Actions` is never - // penalized for it, while a function that genuinely hangs (with no - // `$.Actions` call in flight) still times out at the same `timeoutMs` - // it always did. + // `executeAction`'s long-poll (dev-server.ts's pollQueryExecution) can legitimately outlast + // `timeoutMs` — that's network wait, not a hung customer function. Pausing the hang-detection + // timer while a call is in flight, and giving a fresh `timeoutMs` window once all in-flight + // calls settle, means real `$.Actions` progress is never penalized, while a genuine hang (no + // call in flight) still times out at the usual `timeoutMs`. let timer: ReturnType | undefined; let rejectTimeout: ((error: Error) => void) | undefined; let pendingActionCalls = 0; @@ -517,18 +512,17 @@ async function runScriptLocally( const guardedExecuteAction: ExecuteAction = async (fqn, inputs, connectionId) => { if (!scope.isCurrent()) { // A concluded scope stays concluded forever, not just "not the latest" — the wording stays conclusion-neutral rather than claiming a timeout that may not have happened. - return Promise.reject( - new Error( - `Execution of "${func.name}" already concluded; refusing to run ` + - `"${fqn}" as this stale execution to avoid using a newer execution's identity.`, - ), + throw new Error( + `Execution of "${func.name}" already concluded; refusing to run ` + + `"${fqn}" as this stale execution to avoid using a newer execution's identity.`, ); } pendingActionCalls += 1; clearTimeout(timer); try { + const actionCallPromise = executeAction(fqn, inputs, connectionId); return await withTimeout( - executeAction(fqn, inputs, connectionId), + actionCallPromise, MAX_ACTION_CALL_TIMEOUT_MS, `$.Actions call to "${fqn}"`, ); From 65a987a78f302ba39fde2f854e54e26d15309470 Mon Sep 17 00:00:00 2001 From: Tiffany Trinh Date: Fri, 28 Aug 2026 16:02:25 -0400 Subject: [PATCH 11/14] fix(apps): stop the per-request loader wrapper from defeating SDK registration caching MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit handleExecuteAction wrapped loadModule in a fresh closure every request to reuse an already-primed entry, but local-execution.ts's action-catalog and apps-backend registration caches are keyed on loadModule's own identity — so every request looked like a fresh, unregistered loadModule and re-ran SDK registration in full instead of hitting the cache. executeScriptLocally now takes the primed entry as its own parameter, so loadModule itself stays the stable reference the registration caches expect. --- packages/plugins/apps/src/vite/dev-server.ts | 14 +++++----- .../apps/src/vite/local-execution.test.ts | 22 +++++++-------- .../plugins/apps/src/vite/local-execution.ts | 27 +++++++++++++++---- 3 files changed, 39 insertions(+), 24 deletions(-) diff --git a/packages/plugins/apps/src/vite/dev-server.ts b/packages/plugins/apps/src/vite/dev-server.ts index 04f9af5f4..7bf70c512 100644 --- a/packages/plugins/apps/src/vite/dev-server.ts +++ b/packages/plugins/apps/src/vite/dev-server.ts @@ -426,20 +426,20 @@ async function handleExecuteAction( ), }; - // executeScriptLocally loads this same entry specifier again - // internally; reuse the module already resolved above instead of - // making Vite re-run ssrLoadModule for it a second time. - const loadModuleReusingPrimedEntry: LoadModule = (specifier) => - specifier === entrySpecifier ? Promise.resolve(primedModule) : loadModule(specifier); - const executeAction = makeExecuteActionRemotely(auth, doAuthenticatedRequest, log); const result = await executeScriptLocally( funcWithConnectionIds, projectRoot, args, executeAction, - loadModuleReusingPrimedEntry, + // `loadModule` itself stays the stable reference `local-execution.ts`'s registration + // caches are keyed on; `primedModule` (passed separately below) lets executeScriptLocally + // reuse the entry already resolved above instead of making Vite re-run ssrLoadModule for + // it a second time. + loadModule, log, + DEFAULT_TIMEOUT_MS, + primedModule, ); res.statusCode = 200; diff --git a/packages/plugins/apps/src/vite/local-execution.test.ts b/packages/plugins/apps/src/vite/local-execution.test.ts index 4b7994ed7..9ec848b8b 100644 --- a/packages/plugins/apps/src/vite/local-execution.test.ts +++ b/packages/plugins/apps/src/vite/local-execution.test.ts @@ -933,23 +933,17 @@ describe('local-execution — executeScriptLocally', () => { expect(registeredImpl).toBeDefined(); }); - // The happy-path counterpart to the "shared loadModule with a never-settling load" test below: proves the plain success case is deduped too, not just the failure/eviction paths. - test('Should load the action-catalog module only once across two successful executions that share the same loadModule', async () => { + test('Should reuse the cached action-catalog registration across executions that share the same loadModule reference, even when each passes a different primedEntry — matching dev-server.ts, which threads one stable loadModule but a fresh per-request primed module', async () => { jest.spyOn(shared, 'isActionCatalogInstalled').mockReturnValue(true); let actionCatalogLoadCount = 0; const loadModule: LoadModule = async (specifier: string) => { - if (specifier === func.absolutePath + LOCAL_EXECUTION_LOAD_SUFFIX) { - return { example: () => 'ok' }; - } if (specifier === '@datadog/action-catalog/action-execution') { actionCatalogLoadCount += 1; return { setExecuteActionImplementation: () => {} }; } - const notFoundError: NodeJS.ErrnoException = new Error( - `Cannot find module '${specifier}'`, - ); - notFoundError.code = 'MODULE_NOT_FOUND'; - throw notFoundError; + const error: NodeJS.ErrnoException = new Error(`Cannot find module '${specifier}'`); + error.code = 'MODULE_NOT_FOUND'; + throw error; }; const first = await executeScriptLocally( @@ -959,6 +953,8 @@ describe('local-execution — executeScriptLocally', () => { stubExecuteAction, loadModule, mockLogger, + undefined, + { example: () => 'first' }, ); const second = await executeScriptLocally( func, @@ -967,10 +963,12 @@ describe('local-execution — executeScriptLocally', () => { stubExecuteAction, loadModule, mockLogger, + undefined, + { example: () => 'second' }, ); - expect(first).toEqual({ data: 'ok' }); - expect(second).toEqual({ data: 'ok' }); + expect(first).toEqual({ data: 'first' }); + expect(second).toEqual({ data: 'second' }); expect(actionCatalogLoadCount).toBe(1); }); diff --git a/packages/plugins/apps/src/vite/local-execution.ts b/packages/plugins/apps/src/vite/local-execution.ts index b76112e55..c9eb66b4e 100644 --- a/packages/plugins/apps/src/vite/local-execution.ts +++ b/packages/plugins/apps/src/vite/local-execution.ts @@ -470,9 +470,19 @@ export async function executeScriptLocally( loadModule: LoadModule, log: Logger, timeoutMs: number = DEFAULT_TIMEOUT_MS, + primedEntry?: Record, ): Promise { return enqueue(() => - runScriptLocally(func, projectRoot, args, executeAction, loadModule, log, timeoutMs), + runScriptLocally( + func, + projectRoot, + args, + executeAction, + loadModule, + log, + timeoutMs, + primedEntry, + ), ); } @@ -484,6 +494,7 @@ async function runScriptLocally( loadModule: LoadModule, log: Logger, timeoutMs: number, + primedEntry?: Record, ): Promise { // Never log the args themselves — they may carry secrets/PII, matching dev-server.ts's cloud path. log.debug(`Executing "${func.name}" in-process with args`); @@ -554,10 +565,16 @@ async function runScriptLocally( const run = async (): Promise => { // Loads and evaluates the customer's module BEFORE installing $ and the SDK bridges below, matching production's own ordering (backend/virtual-entry.ts statically imports the customer module before its wrapper installs $ and the SDK bridges) — code that reaches for $ or a typed action during its own top-level evaluation fails the same way locally as it would in Datadog, instead of silently succeeding against bindings production wouldn't have installed yet. - const mod = await loadCustomerModuleEntry( - loadModule, - func.absolutePath + LOCAL_EXECUTION_LOAD_SUFFIX, - ); + // A caller that already primed this same entry (e.g. dev-server.ts's own module-graph + // priming load) passes the resolved module directly here instead of making `loadModule` + // resolve it a second time — keeping `loadModule` itself unwrapped and stable, since the + // registration caches below key off its identity, not off which entry was last loaded. + const mod = + primedEntry ?? + (await loadCustomerModuleEntry( + loadModule, + func.absolutePath + LOCAL_EXECUTION_LOAD_SUFFIX, + )); const fn = mod[func.name]; if (typeof fn !== 'function') { throw new Error(`"${func.name}" is not a function exported from ${func.absolutePath}`); From a0f37c53c134c6dfb73f53da080c00e64cd16809 Mon Sep 17 00:00:00 2001 From: Tiffany Trinh Date: Fri, 28 Aug 2026 16:14:38 -0400 Subject: [PATCH 12/14] fix(apps): scope suffixed-subgraph tracking to one local execution, not the whole dev server MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit resolveId's suffixedSubgraphImporters Set lived for the dev server's entire process lifetime, with no distinction between "this helper is part of the local execution currently running" and "this helper was part of some past execution." A later, unrelated SSR resolution of the same helper module (e.g. from an app that also does its own ordinary SSR) would inherit the marker and serve real backend code instead of the frontend RPC-proxy stub. Scopes the Set via AsyncLocalStorage instead, established in loadCustomerModuleEntry alongside the existing customerModuleLoadContext — every caller that loads a customer entry, real dev-server request or test harness alike, funnels through that one function, so the fix reaches every path uniformly rather than only whichever loadModule happened to be wrapped. --- packages/plugins/apps/src/vite/index.test.ts | 73 +++++++++++++++++++ packages/plugins/apps/src/vite/index.ts | 29 ++++---- .../plugins/apps/src/vite/local-execution.ts | 9 ++- 3 files changed, 95 insertions(+), 16 deletions(-) diff --git a/packages/plugins/apps/src/vite/index.test.ts b/packages/plugins/apps/src/vite/index.test.ts index 96bc0c922..f749911a2 100644 --- a/packages/plugins/apps/src/vite/index.test.ts +++ b/packages/plugins/apps/src/vite/index.test.ts @@ -6,6 +6,7 @@ import * as assets from '@dd/apps-plugin/assets'; import * as identifier from '@dd/apps-plugin/identifier'; import { getVitePlugin } from '@dd/apps-plugin/vite/index'; import type { ViteBundler } from '@dd/apps-plugin/vite/index'; +import { localExecutionResolutionContext } from '@dd/apps-plugin/vite/local-execution'; import { InjectPosition } from '@dd/core/types'; import { getContextMock, getRepositoryDataMock } from '@dd/tests/_jest/helpers/mocks'; import { parseAst } from 'rollup/parseAst'; @@ -46,6 +47,20 @@ function getTransformHandler(plugin: ReturnType): Function return transform.handler; } +/** Narrows a Vite plugin's `resolveId` hook to its full-object form (`{ handler, ... }`) so tests can call it directly. */ +function getResolveIdHandler(plugin: ReturnType): Function { + const resolveId = plugin?.resolveId; + if ( + typeof resolveId !== 'object' || + resolveId === null || + !('handler' in resolveId) || + typeof resolveId.handler !== 'function' + ) { + throw new Error('Expected plugin.resolveId to be an object with a handler function.'); + } + return resolveId.handler; +} + const mockViteBuild = jest.fn(); const mockVite = { build: mockViteBuild, @@ -268,6 +283,64 @@ describe('Backend Functions - getVitePlugin', () => { expect(result?.code).toEqual(expect.stringContaining('executeBackendFunction')); }); + describe('resolveId suffix propagation through a plain helper module', () => { + const entryFile = '/build/src/backend/entry.backend.ts'; + const helperImporter = '/build/src/helper.ts'; + const nestedBackendFile = '/build/src/backend/otherHandler.backend.ts'; + + /** Resolves the entry's own `./helper` import, marking `helperImporter` as part of whichever subgraph tracking Set (if any) is active on the AsyncLocalStorage store at call time — the same first hop a real local execution's traversal makes. */ + const resolveEntryToHelper = (resolveIdHandler: Function) => + resolveIdHandler.call( + { resolve: jest.fn(async () => ({ id: helperImporter })) }, + './helper', + `${entryFile}${LOCAL_EXECUTION_LOAD_SUFFIX}`, + { ssr: true }, + ); + + /** Resolves a nested backend import from the helper — the second hop that should only inherit the suffix if `helperImporter` is still recognized as part of the current subgraph. */ + const resolveHelperToBackendFile = (resolveIdHandler: Function) => + resolveIdHandler.call( + { resolve: jest.fn(async () => ({ id: nestedBackendFile })) }, + './otherHandler.backend', + helperImporter, + { ssr: true }, + ); + + test('Should propagate the suffix onto a nested backend import reached through a helper resolved earlier in the same local execution', async () => { + const plugin = getVitePlugin(defaultOptions); + const resolveIdHandler = getResolveIdHandler(plugin); + + const result = await localExecutionResolutionContext.run(new Set(), async () => { + await resolveEntryToHelper(resolveIdHandler); + return resolveHelperToBackendFile(resolveIdHandler); + }); + + expect((result as { id: string } | null)?.id).toBe( + `${nestedBackendFile}${LOCAL_EXECUTION_LOAD_SUFFIX}`, + ); + }); + + // Regression test: a plain module-level Set (this hook's design before the + // AsyncLocalStorage fix) would still recognize `helperImporter` here, since nothing ever + // cleared it after the local execution below finished — incorrectly serving real backend + // code into what should be an ordinary, unrelated SSR resolution of the same helper. + test('Should NOT propagate the suffix onto the same helper importer once no local execution is in flight, even though an earlier execution already traversed it', async () => { + const plugin = getVitePlugin(defaultOptions); + const resolveIdHandler = getResolveIdHandler(plugin); + + // A prior, now-finished local execution traverses entry -> helper. + await localExecutionResolutionContext.run(new Set(), () => + resolveEntryToHelper(resolveIdHandler), + ); + + // Later, unrelated SSR resolution of the same helper importer — outside any local + // execution's own load. + const result = await resolveHelperToBackendFile(resolveIdHandler); + + expect(result).toBeNull(); + }); + }); + test('Should inject the apps runtime', () => { getVitePlugin(defaultOptions); diff --git a/packages/plugins/apps/src/vite/index.ts b/packages/plugins/apps/src/vite/index.ts index 54a92dd5f..1aa8385da 100644 --- a/packages/plugins/apps/src/vite/index.ts +++ b/packages/plugins/apps/src/vite/index.ts @@ -30,6 +30,7 @@ import { buildBackendFunctions } from './build-backend-functions'; import { collectModuleGraphFromServer } from './dev-server-module-graph'; import { createDevServerMiddleware } from './dev-server'; import { handleUpload } from './handle-upload'; +import { localExecutionResolutionContext } from './local-execution'; export type ViteBundler = { build: typeof build; @@ -124,13 +125,6 @@ export const getVitePlugin = ({ const { setBackendFunctions, getBackendFunctions } = createBackendFunctionRegistry(); - // Non-backend module ids reached transitively from a suffixed backend entry point. A plain - // helper module's id never carries LOCAL_EXECUTION_LOAD_SUFFIX itself (it's never ambiguous - // between a proxy and real code), but must still be recognized as part of the suffixed - // subgraph — otherwise a *.backend.ts file reached through it would lose the marker one hop - // later than a direct backend-to-backend import does. - const suffixedSubgraphImporters = new Set(); - return { // @datadog/apps-backend and @datadog/action-catalog ship ESM-only, but server.ssrLoadModule // externalizes node_modules by default (a plain require(), for speed), which throws @@ -149,10 +143,10 @@ export const getVitePlugin = ({ // hook a `.backend.ts` file importing another `.backend.ts` file would resolve that nested // import unsuffixed and get replaced with the frontend RPC-proxy stub, breaking local // execution for a multi-backend-file graph. Propagates whenever the importer was itself - // suffixed, or is a previously-seen non-backend module reached from within the suffixed - // subgraph (see `suffixedSubgraphImporters`) — and only appends the suffix onto another - // `.backend.ts` file, since a plain helper module never hits the proxy-vs-real-code branch - // this marker disambiguates. + // suffixed, or is a previously-seen non-backend module reached from within the current + // local execution's own subgraph (see `localExecutionResolutionContext`) — and only + // appends the suffix onto another `.backend.ts` file, since a plain helper module never + // hits the proxy-vs-real-code branch this marker disambiguates. resolveId: { // Must run before Vite's built-in resolver: a plain relative specifier like // `./other.backend` is fully resolvable by Vite's own filesystem resolution, which at @@ -163,11 +157,18 @@ export const getVitePlugin = ({ // Scoped to `resolveOptions.ssr` since local execution's traversal is always an // SSR resolution — otherwise the same helper reached later from the ordinary // client graph would inherit the marker and serve real backend code to the - // browser instead of the frontend RPC-proxy stub. + // browser instead of the frontend RPC-proxy stub. The store itself being present + // is the other half of the scoping: it's only populated while a local execution's + // own loadModule call is in flight (see configureServer), so an unrelated SSR + // resolution elsewhere in the process never inherits a marker left behind by an + // earlier, already-finished execution. + const subgraphImporters = localExecutionResolutionContext.getStore(); const isPartOfSuffixedSubgraph = !!importer && (importer.endsWith(LOCAL_EXECUTION_LOAD_SUFFIX) || - (resolveOptions.ssr === true && suffixedSubgraphImporters.has(importer))); + (resolveOptions.ssr === true && + !!subgraphImporters && + subgraphImporters.has(importer))); if (!isPartOfSuffixedSubgraph) { return null; } @@ -181,7 +182,7 @@ export const getVitePlugin = ({ } if (!BACKEND_FILE_RE.test(resolved.id)) { - suffixedSubgraphImporters.add(resolved.id); + subgraphImporters?.add(resolved.id); return resolved; } diff --git a/packages/plugins/apps/src/vite/local-execution.ts b/packages/plugins/apps/src/vite/local-execution.ts index c9eb66b4e..0c118aefe 100644 --- a/packages/plugins/apps/src/vite/local-execution.ts +++ b/packages/plugins/apps/src/vite/local-execution.ts @@ -33,6 +33,9 @@ const hadPreexistingDollar = Reflect.has(globalThis, '$'); /** Marks the window where a customer module's own top-level code is loading, narrower than "no execution box on the call stack" (also true between executions, where the undefined-returning fallback below is correct). Carries its own mutable box so a top-level `$` write (e.g. `zx/globals`) lands scoped to this module's own load, not the shared `globalDollarOutsideExecution` slot a later, unrelated load would also read from. */ const customerModuleLoadContext = new AsyncLocalStorage<{ assigned: boolean; value: unknown }>(); +/** Scopes vite/index.ts's suffixed-subgraph tracking (see its `resolveId` hook) to one entry's own module-graph traversal, run alongside `customerModuleLoadContext` below — every caller that loads a customer entry, real dev-server request or test harness alike, funnels through `loadCustomerModuleEntry`, so scoping here (rather than wherever a particular `loadModule` happens to be constructed) reaches every path uniformly. Without this, a single process-wide Set would let a helper module reached by one local execution's traversal stay marked for the dev server's whole lifetime, so a later unrelated SSR resolution of the same helper would inherit the marker and serve real backend code instead of the frontend RPC-proxy stub. */ +export const localExecutionResolutionContext = new AsyncLocalStorage>(); + /** Backs `globalThis.$` outside any execution box (e.g. this module's own import-time state); seeded from any `$` already installed before this module loaded so the accessor below doesn't discard a legitimate `zx/globals`-style passthrough. */ let globalDollarOutsideExecution: unknown = Reflect.get(globalThis, '$'); @@ -126,8 +129,10 @@ export function loadCustomerModuleEntry( loadModule: LoadModule, entrySpecifier: string, ): Promise> { - return customerModuleLoadContext.run({ assigned: false, value: undefined }, () => - loadModule(entrySpecifier), + return localExecutionResolutionContext.run(new Set(), () => + customerModuleLoadContext.run({ assigned: false, value: undefined }, () => + loadModule(entrySpecifier), + ), ); } From 414cab9b307559cc8ed8a096ea7fb9a91baea05e Mon Sep 17 00:00:00 2001 From: Tiffany Trinh Date: Fri, 28 Aug 2026 17:02:03 -0400 Subject: [PATCH 13/14] test(tools): cover bundle()'s subpath-aware external matcher MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The external matcher exists specifically to externalize a dependency's subpath imports (e.g. rollup/parseAst) alongside its bare specifier, fixing a real crash this PR's Motivation calls out — but had no test coverage. Runs the real .mjs module in a node subprocess since ts-jest compiles this test file to CommonJS, which can't import a real ES module directly. --- packages/tools/src/rollupConfig.test.ts | 59 +++++++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/packages/tools/src/rollupConfig.test.ts b/packages/tools/src/rollupConfig.test.ts index e01117bd6..04b36bd0c 100644 --- a/packages/tools/src/rollupConfig.test.ts +++ b/packages/tools/src/rollupConfig.test.ts @@ -449,3 +449,62 @@ describe('Bundling', () => { console.timeEnd(timeId); }); }); + +describe('bundle - external matcher', () => { + // rollupConfig.mjs is a real ES module; ts-jest compiles this test file to CommonJS, so + // neither a static nor a dynamic `import()` of it works under Jest (Node refuses to + // `require()` an ES module). Exercising it via a real `node` subprocess sidesteps that + // entirely and runs the exact same code a real build does. + const runExternalMatcher = (ids: string[], config: { external?: string[] } = {}): boolean[] => { + const script = ` + import { bundle } from ${JSON.stringify(pathToFileURL(path.resolve(__dirname, 'rollupConfig.mjs')).href)}; + const packageJson = { + module: 'dist/src/index.js', + main: 'dist/src/index.cjs', + name: '@datadog/some-plugin', + peerDependencies: { vite: '6.0.0' }, + dependencies: { chalk: '2.3.1', rollup: '4.45.1' }, + }; + const { external } = bundle(packageJson, ${JSON.stringify(config)}); + console.log(JSON.stringify(${JSON.stringify(ids)}.map((id) => external(id)))); + `; + const output = executeSync('node', ['--input-type=module', '-e', script]); + return JSON.parse(output); + }; + + test('Should treat a dependency as external', () => { + expect(runExternalMatcher(['chalk'])).toEqual([true]); + }); + + test('Should treat a peer dependency as external', () => { + expect(runExternalMatcher(['vite'])).toEqual([true]); + }); + + test('Should treat a Node.js built-in as external', () => { + expect(runExternalMatcher(['fs'])).toEqual([true]); + }); + + test('Should treat an id explicitly listed in config.external as external', () => { + expect( + runExternalMatcher(['some-extra-package'], { external: ['some-extra-package'] }), + ).toEqual([true]); + }); + + test('Should treat a subpath import of a dependency as external, not just its exact bare specifier', () => { + // The bug this matcher exists to fix: a plain string in Rollup's own `external` array + // only matches an id exactly, so `rollup/parseAst` would otherwise get bundled despite + // `rollup` itself being declared external. + expect(runExternalMatcher(['rollup/parseAst'])).toEqual([true]); + }); + + test('Should not treat an unrelated package as external', () => { + expect(runExternalMatcher(['left-pad'])).toEqual([false]); + }); + + test('Should not treat a package whose name merely starts with a dependency name as a subpath of it', () => { + // `rollup-plugin-esbuild` is not a subpath of `rollup` — the matcher must check for a + // `/` boundary (`startsWith('rollup/')`), not a bare string-prefix match, or an unrelated + // sibling package sharing a name prefix would be wrongly externalized. + expect(runExternalMatcher(['rollup-plugin-esbuild'])).toEqual([false]); + }); +}); From fbddcb4b2bedcf39e956f1357d6229dc8639ed3f Mon Sep 17 00:00:00 2001 From: Tiffany Trinh Date: Fri, 28 Aug 2026 17:15:41 -0400 Subject: [PATCH 14/14] test(apps): cover module-graph error paths, cycles, and executeActionViaCloud auth gate Adds regression tests for unreadable/unparseable module source and a self-referential import cycle in collectModuleGraphFromServer, plus a no-auth-configured case for /__dd/executeActionViaCloud mirroring the existing /__dd/executeAction coverage. --- .../src/vite/dev-server-module-graph.test.ts | 84 +++++++++++++++++-- .../plugins/apps/src/vite/dev-server.test.ts | 27 ++++++ 2 files changed, 106 insertions(+), 5 deletions(-) diff --git a/packages/plugins/apps/src/vite/dev-server-module-graph.test.ts b/packages/plugins/apps/src/vite/dev-server-module-graph.test.ts index 00d2fe0d4..4ec62a55b 100644 --- a/packages/plugins/apps/src/vite/dev-server-module-graph.test.ts +++ b/packages/plugins/apps/src/vite/dev-server-module-graph.test.ts @@ -2,6 +2,8 @@ // This product includes software developed at Datadog (https://www.datadoghq.com/). // Copyright 2019-Present Datadog, Inc. +import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; import path from 'node:path'; import type { ViteDevServer } from 'vite'; @@ -16,13 +18,24 @@ const FIXTURE_ROOT = path.resolve( const ENTRY_ID = path.join(FIXTURE_ROOT, 'helper.ts'); const SUFFIXED_ENTRY_ID = ENTRY_ID + LOCAL_EXECUTION_LOAD_SUFFIX; -function makeFakeServer(resolveId: (specifier: string) => Promise<{ id: string } | null>) { +/** A minimal fake ModuleNode shape, matching only the fields collectModuleGraphFromServer reads. */ +interface FakeModuleNode { + id: string; + file?: string; + importedModules: Set; +} + +function makeFakeServer( + resolveId: (specifier: string) => Promise<{ id: string } | null>, + entryNode: FakeModuleNode = { + id: SUFFIXED_ENTRY_ID, + file: ENTRY_ID, + importedModules: new Set(), + }, +) { return { moduleGraph: { - getModuleById: (id: string) => - id === SUFFIXED_ENTRY_ID - ? { id: SUFFIXED_ENTRY_ID, file: ENTRY_ID, importedModules: new Set() } - : undefined, + getModuleById: (id: string) => (id === SUFFIXED_ENTRY_ID ? entryNode : undefined), }, pluginContainer: { resolveId: (specifier: string) => resolveId(specifier), @@ -47,4 +60,65 @@ describe('dev-server-module-graph — collectModuleGraphFromServer', () => { expect(records.has(ENTRY_ID)).toBe(true); }); + + test('Should throw a clear error when a module file cannot be read from disk', async () => { + const missingFile = path.join(FIXTURE_ROOT, 'does-not-exist.ts'); + const entryNode: FakeModuleNode = { + id: SUFFIXED_ENTRY_ID, + file: missingFile, + importedModules: new Set(), + }; + const server = makeFakeServer(async () => null, entryNode); + + await expect(collectModuleGraphFromServer(server, ENTRY_ID, FIXTURE_ROOT)).rejects.toThrow( + /unreadable module source/, + ); + }); + + describe('when a module file fails to parse', () => { + let tempDir: string; + let badFile: string; + + beforeAll(() => { + tempDir = mkdtempSync(path.join(tmpdir(), 'dev-server-module-graph-test-')); + badFile = path.join(tempDir, 'broken.ts'); + writeFileSync(badFile, 'export function broken( {{{ this is not valid syntax'); + }); + + afterAll(() => { + rmSync(tempDir, { recursive: true, force: true }); + }); + + test('Should throw a clear error instead of propagating the raw parser exception', async () => { + const entryNode: FakeModuleNode = { + id: SUFFIXED_ENTRY_ID, + file: badFile, + importedModules: new Set(), + }; + const server = makeFakeServer(async () => null, entryNode); + + await expect( + collectModuleGraphFromServer(server, ENTRY_ID, FIXTURE_ROOT), + ).rejects.toThrow(/unparseable module source/); + }); + }); + + test('Should not infinite-loop or double-process a module reached through a cycle in the import graph', async () => { + const resolvedPath = path.join(FIXTURE_ROOT, 'getRuntimeUsers.backend.ts'); + const entryNode: FakeModuleNode = { + id: SUFFIXED_ENTRY_ID, + file: ENTRY_ID, + importedModules: new Set(), + }; + // A self-referential cycle: the entry "imports" itself via node.importedModules, the + // same shape a real circular backend-to-backend import produces in Vite's own module + // graph. The `visited` Set must stop this from being processed a second time. + entryNode.importedModules.add(entryNode); + const server = makeFakeServer(async () => ({ id: resolvedPath }), entryNode); + + const records = await collectModuleGraphFromServer(server, ENTRY_ID, FIXTURE_ROOT); + + expect(records.size).toBe(1); + expect(records.has(ENTRY_ID)).toBe(true); + }); }); diff --git a/packages/plugins/apps/src/vite/dev-server.test.ts b/packages/plugins/apps/src/vite/dev-server.test.ts index 88b634c35..a3ae3238e 100644 --- a/packages/plugins/apps/src/vite/dev-server.test.ts +++ b/packages/plugins/apps/src/vite/dev-server.test.ts @@ -437,6 +437,33 @@ describe('Dev Server Middleware', () => { expect(res.statusCode).toBe(404); }); + test('Should reject a request with no auth configured upfront, matching the /__dd/executeAction gate', async () => { + const noAuthMiddleware = createDevServerMiddleware( + mockViteBuild, + mockLoadModule, + () => mockFunctions, + async () => [], + mockOauthOnlyAuth, + undefined, + '/project', + mockLog, + ); + + const req = createMockRequest('/__dd/executeActionViaCloud', { + functionName: encodeQueryName(mockFunctions[0]), + args: [], + }); + const res = createMockResponse(); + + noAuthMiddleware(req, res, jest.fn()); + await res.done; + + expect(res.statusCode).toBe(400); + const body = JSON.parse(res.getBody()); + expect(body.success).toBe(false); + expect(body.error).toContain('Auth credentials not configured'); + }); + /* * The nock mock replies with 403 to simulate the upstream Datadog API * rejecting the request (e.g. bad credentials). The middleware still