[APPS] Add backend function connection support#331
[APPS] Add backend function connection support#331sdkennedy2 wants to merge 11 commits intomasterfrom
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e3230b4ae
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c0359d4fc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| log.debug(`Refreshed connection IDs from ${connectionsPath} (${connectionIds.length})`); | ||
| } catch (error) { | ||
| const message = error instanceof Error ? error.message : String(error); | ||
| log.error(`Failed to refresh connection IDs: ${message}`); |
There was a problem hiding this comment.
Clear stale connection IDs after failed HMR refresh
When connections.ts has already loaded IDs and a later edit makes server.transformRequest/extraction fail, this catch only logs the error while connectionRegistry keeps the previous array. The dev middleware calls getConnectionIds() on each /__dd/executeAction, so previews keep sending removed connection IDs until another successful refresh or a server restart; because these IDs are an allowlist, a failed refresh should clear or block the registry instead of reusing stale permissions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c34e58376
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
1c34e58 to
ce1106b
Compare
|
@codex review |
…ward allowedConnectionIds to dev preview
…s validation - buildStart now uses server.transformRequest in dev (vite serve), where this.load returns a ModuleInfo proxy whose `code` getter throws. server is captured via configureServer, which fires before buildStart in dev. - buildStart logs the framed [connections] error before re-throwing so it survives any masking by downstream closeBundle errors (e.g. error-tracking sourcemaps' "No output files found"). - extractConnectionIds now derives line:col from node.start byte offsets (which SWC produces) against the source text. Previously the loc?.start branch was dead — rollup's parser doesn't emit ESTree loc info.
8ba24f1 to
e7ee68b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ba24f1659
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| test('Should throw when both "connections" and "CONNECTIONS" are exported', () => { | ||
| const ast = program([ | ||
| exportConnections([stringProperty('A', 'uuid-1')], 'connections'), | ||
| exportConnections([stringProperty('B', 'uuid-2')], 'CONNECTIONS'), | ||
| ]); | ||
| expect(() => extractConnectionIds(ast, filePath, '')).toThrow( | ||
| 'multiple top-level "export const CONNECTIONS" (or "connections") declarations are not allowed', | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Out of curIosity, why should we support both cases? Do we need to, or can we pick one?
There was a problem hiding this comment.
Thats fair pushback. I just thought it was an easy foot gun to make. I plan on adding a couple eslint rules perhaps we standardize on the uppercase version and we can avoid the footgun with linting.
| { | ||
| type: 'SpreadElement', | ||
| argument: { type: 'Identifier', name: 'other' }, | ||
| } as SpreadElement, |
There was a problem hiding this comment.
Does satisfies work here in place of as?
| // surfaces immediately. | ||
| describe('framed source location from parseAst', () => { | ||
| test('Should include line:col when value is not a string literal', async () => { | ||
| const { parseAst } = await import('rollup/parseAst'); |
There was a problem hiding this comment.
Why is this dynamically imported? Also why rollup's AST parser specifically? I feel like something should expose the parser somewhere that we can use instead of doing this.
| const middleware = createDevServerMiddleware({ | ||
| viteBuild: mockViteBuild, | ||
| getBackendFunctions: () => mockFunctions, | ||
| getConnectionIds: () => [], | ||
| auth: mockAuth, | ||
| projectRoot: '/project', | ||
| log: mockLog, | ||
| }); |
There was a problem hiding this comment.
Nice, this is a much better interface
| const inputs = ( | ||
| capturedBody as { | ||
| data: { | ||
| attributes: { query: { properties: { spec: { inputs: unknown } } } }; | ||
| }; | ||
| } | ||
| ).data.attributes.query.properties.spec.inputs as { | ||
| allowedConnectionIds: string[]; | ||
| }; |
There was a problem hiding this comment.
This is an awkward construction especially when capturedBody is set to unknown and inputs is also set to unknown
There was a problem hiding this comment.
That is wild. Claude wrote that. I glossed over the tests, I usually don't invest too much resources in unit test coverage during the design partner phase since nothing is really set in stone and things are likely to change.
This line is pretty egregious though haha. Happy to fix it.
| /** | ||
| * Returns the Vite-specific plugin hooks for the apps plugin. | ||
| * | ||
| * Production (closeBundle): builds backend functions (if any) then uploads | ||
| * all assets sequentially. | ||
| * | ||
| * Dev (configureServer): registers middleware for local backend function | ||
| * testing when auth credentials are available. | ||
| */ |
There was a problem hiding this comment.
Please restore this comment
There was a problem hiding this comment.
I actually intentionally removed it because I thought it was weird describing all of the inner functions at the top (claude wanted to add even more to it as the complexity of the plugin grew), but it could be useful to differentiate the production versus dev. I'm not particularly opinionated. I can add it back.
There was a problem hiding this comment.
There's a lot of mocking and extracting in the new helper functions at the top... is there a way to simplify without faking everything? The more mocks/fakes there are, the less confidence I have in the code
There was a problem hiding this comment.
I wonder if we should just exclude index.ts from unit tests and rely on the e2e test. This file mostly just glues everything together and it might just be better to verify everything works end to end.
We could in a later PR just refactor index.ts and pull out distinct pieces and create unit tests for them directly for example:
- Upload function
- Registry functions
- etc
| ); | ||
| expect(Object.keys(manifest.backend.functions).sort()).toEqual(expectedKeys.sort()); | ||
| for (const entry of Object.values(manifest.backend.functions)) { | ||
| expect(Array.isArray(entry.allowedConnectionIds)).toBe(true); |
There was a problem hiding this comment.
Are we able to test the values of the array as well, or are we satisfied with an Array.isArray check?
6f12a4e to
b64302d
Compare
yoannmoinet
left a comment
There was a problem hiding this comment.
Overall looks good to me.
Just a nit on some error handling that can be misleading.
| } catch { | ||
| // not found at this extension — try the next. | ||
| } |
There was a problem hiding this comment.
This will swallow any errors, not just the "not found" errors.
Maybe a specific gate of ENOENT errors only would be necessary.
Unless we're ok with this.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c605f1893d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (filePath) { | ||
| this.addWatchFile(filePath); | ||
| } |
There was a problem hiding this comment.
Watch for newly added connection files
In vite build --watch this only registers the selected file when one already exists, so a watch session that starts without connections.* will never rerun when the developer later adds connections.ts/.js, and a session that starts with a lower-priority file will not notice a newly added higher-priority one. Because buildStart is the only production path that refreshes connectionRegistry, the uploaded manifest keeps an empty or stale allowedConnectionIds list until the process is restarted.
Useful? React with 👍 / 👎.

Motivation
High Code App backend functions today fail at runtime when they call an action that requires a
connectionId(e.g.@datadog/action-catalog/openai/openai#chatCompletion). The Action Platform server rejects the call withAccess denied for connection {uuid}. Add it to the allowed list in the step configuration.because the build plugin never tells the server which connections each function is allowed to use.This PR implements the Backend Function Connections RFC, which chose a single-file approach: customers maintain a centralized
connections.tsat the project root listing every connection their app uses, and the build plugin reads it once at build time to feed both the production upload manifest and the dev-preview request body.CLI tooling and the ESLint rule from the RFC's Future Enhancements section are intentionally out of scope.
Changes
Customer-facing surface
Customers add a
connections.ts(or.tsx/.js/.jsx) at the project root. EitherCONNECTIONS(uppercase) orconnections(lowercase) is accepted as the export name:Backend functions reference connection IDs as before (literal UUIDs or via
CONNECTIONS.OPEN_AIonce they import it). Onnpm run build, the plugin emits amanifest.jsonat the root of the upload ZIP with the per-function allowed connection IDs nested underbackend.functions, keyed by encoded query name. Every backend function key gets the sameallowedConnectionIdsarray (the union fromconnections.ts). The server-side actions runtime supports per-function distinct lists, but the RFC explicitly chose the flat union.{ "backend": { "functions": { "631339bbf278cb23b10f1118572d0e3d21166de9e4363c6941d055c9f1b19ea1.helloBackend": { "allowedConnectionIds": [ "321d6080-5c01-4793-be58-475f0f20e801", "9e51df63-7cba-4067-a1fc-a156195074e8" ] } } } }For dev preview, the same list is forwarded inside
inputs.allowedConnectionIdsof the/api/v2/app-builder/queries/preview-asyncrequest body, matching the action shape the server expects.QA Instructions
End-to-end against a scaffolded high-code-app (e.g.
~/dd/web-ui/test-action-catalog-app):connections.ts, runnpm run buildwithlogLevel: 'debug'. Expectmanifest.jsonat the root of the upload ZIP withbackend.functions[<key>].allowedConnectionIds: []for every function.connections.tsexporting two UUIDs,npm run build, inspect the assembled ZIP — rootmanifest.jsonshould contain every backend-function key underbackend.functions, each with both UUIDs inallowedConnectionIds.DD_API_KEY/DD_APP_KEYset and aconnections.tscontaining the OpenAI UUID, add a backend function callingchatCompletionAction({ ..., connectionId: '...' }).npm run dev, click the action — request should succeed (noAccess denied for connection ...). Editconnections.tsto remove the UUID, re-trigger — should fail with the expected denial. VerifieshandleHotUpdate-driven re-extraction.export const connections = { ... }, repeat steps 2–3. Behavior should be identical.OPEN_AI: process.env.OPEN_AI_ID→npm run buildfails with a framed error pointing at the offending node.default exportform → fails withconnections file must define "export const CONNECTIONS" (or "connections") = { ... }.yarn test:unit packages/plugins/apps(171 tests pass, including 26 extractor tests covering both export-name variants, buildStart/handleHotUpdate test groups, and a dev-serverallowedConnectionIdsassertion).yarn test:e2e packages/tests/src/e2e/appsPlugin(60 tests pass, including the new rootmanifest.jsonshape assertion).Blast Radius
@datadog/vite-pluginwithapps.enable). Other bundlers and other plugin features are untouched.connections.tsnow fail the build where they previously didn't. Customers without aconnections.tssee no change — the manifest is emitted with emptyallowedConnectionIdsarrays, matching pre-existing behavior (no connection-using actions allowed).allowedConnectionIdsinside theinputsfield of the preview-async request body. This is additive — empty list means same behavior as today.Documentation