Repository navigation
ci(AIC-3417): test the published packages in a real AWS Lambda [4/4] - #79
Conversation
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
|
@cursor review |
There was a problem hiding this comment.
Devin Review found 3 potential issues.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
… errors Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…ckages (1/4) (#76) ## Summary This PR fixes nothing. It turns the customer's bug into a test that fails on purpose, and writes down the workaround customers can use until the real fix (PR2) ships. Closes [AIC-3414](https://launchdarkly.atlassian.net/browse/AIC-3414). Parent: [AIC-3370](https://launchdarkly.atlassian.net/browse/AIC-3370). PR 1 of 4 — no module-format change yet. ### The bug, in plain terms Our `@launchdarkly/ai-*` packages ship ESM-only: `"exports"` has an `import` entry and no `require` entry. A customer Lambda that emits CommonJS and bundles with Webpack + `webpack-node-externals` leaves our packages external, and Webpack rewrites `await import('@launchdarkly/ai-node')` into `require('@launchdarkly/ai-node')`. Node finds no `require` entry and throws: ```text Error [ERR_PACKAGE_PATH_NOT_EXPORTED]: No "exports" main defined in /var/task/node_modules/@launchdarkly/ai-node/package.json ``` That happens before any of our code runs, so no AI config is evaluated and nothing reaches AI Insights. An app with a hardcoded fallback prompt keeps returning plausible answers — which is why this went unnoticed. ### Why a test, when we already have a ticket Our monorepo physically cannot reproduce this: internally the packages are symlinked, so `require` resolves fine. The bug only exists in the *published* package shape. So the test builds what npm would ship and consumes it like a customer would — `yarn build` → `npm pack` → install the tarballs into an empty project → Webpack-bundle a CommonJS Lambda handler → run it. It bundles that handler twice, differing only by one line of Webpack config (`nodeExternals({ allowlist: [/^@launchdarkly\/ai-/] })`), and asserts both outcomes: ```text left external → must fail with ERR_PACKAGE_PATH_NOT_EXPORTED / No "exports" main defined allowlisted → must load config, initClient, openaiMessages, NodeTracerProvider ``` The failing case is a real assertion, not a skip or `it.fails`. PR2 flips it from "must fail" to "must pass" — that flip is the proof the fix works on real published artifacts, and the reason this PR lands first. The handler only reports which symbols resolved: no SDK key, no LaunchDarkly call, no model spend. ### Which AIC-3370 asks this PR answers It answers asks 2, 3, and 4: - **Ask 2 — an official Lambda recipe naming the exact allowlist.** `packages/ai-node/README.md` gives the Serverless + Webpack config with `allowlist: [/^@launchdarkly\/ai-/]`, and CI asserts that recipe works. - **Ask 3 — a smoke test that packages and invokes a real CommonJS Lambda, because a source-level import test cannot catch this.** The harness installs `npm pack` tarballs into a separate project, so Node reads the published `"exports"` map. PR4 (#79) runs the same shape against the published npm versions in a real Lambda after release. - **Ask 4 — a documented support matrix.** The root README covers CommonJS source, CommonJS bundle output, native ESM, and Node 22, and marks which rows CI proves. It names Webpack only; esbuild and Rollup rows are not covered yet. Asks 1 (a `require` condition) and 5 (an initialization diagnostic) are PR2 (#77) and PR3 (#78). ### Also in this PR - `yarn test:integration` plus a dedicated Node 22 CI job matching the reported Lambda runtime; existing unit-test jobs untouched. - Docs customers can use today: a Lambda + Serverless + Webpack recipe in `packages/ai-node/README.md`, and a module-format support matrix in the root README saying what works, what doesn't, and which rows CI proves. - `biome.json` excludes `**/.staging` and `**/dist-*` so generated Webpack output isn't linted. ### Testing `yarn test:integration` on Node 22.21.1 and Node 24 (2 passed), plus `yarn workspace @launchdarkly/ai-node test`, `yarn code:check`, `yarn lint:pkg`. The harness shells out to `yarn build` and `npm pack`, so it's much slower than the unit suites — hence its own script and CI job. Start review at `integration-tests/commonjs-webpack/README.md`, then `harness.ts`, then the two assertions in `commonjs-webpack.test.ts`. No UI or staging preview applies. [AIC-3414]: https://launchdarkly.atlassian.net/browse/AIC-3414?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ [AIC-3370]: https://launchdarkly.atlassian.net/browse/AIC-3370?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ Link to Devin session: https://app.devin.ai/sessions/2798b5e37086465cb5a3af7c906a217b Open in Devin Desktop: https://app.devin.ai/desktop/session/2798b5e37086465cb5a3af7c906a217b?variant=devin Requested by: @tonytrinh3
…3/AIC-3416-esm-externalization-diagnostics
…CommonJS apps can load it (2/4) (#77) ## Summary **Plain version:** The other LaunchDarkly JavaScript SDKs already ship in two module formats. This AI SDK did not. It shipped ESM only, so old CommonJS projects crashed when they loaded it. This PR gives the AI SDK the same two-format build, so those projects work. **What it means for the code (junior eng):** The other JavaScript SDKs live in `launchdarkly/js-core`. They build with `tsup` and publish both an `import` condition and a `require` condition. The AI SDK was the outlier with no `require` condition. Now every `@launchdarkly/ai-*` package builds with `tsup` and emits `dist/index.js` (ESM) and `dist/index.cjs` (CommonJS). So `require('@launchdarkly/ai-node')` works. A Webpack bundle that keeps the packages external also works. Node no longer throws `ERR_PACKAGE_PATH_NOT_EXPORTED`. Closes [AIC-3415](https://launchdarkly.atlassian.net/browse/AIC-3415). This fixes the failure in [AIC-3370](https://launchdarkly.atlassian.net/browse/AIC-3370). ### Which AIC-3370 asks this PR answers **Plain version:** This PR answers ask 1 in full. It also adds a few rows to the support table in the root README, which is part of ask 4. The other asks belong to other PRs. **What it means for the code (junior eng):** Ask 1 wants a CommonJS-compatible conditional export. The evidence sits in this diff. - Nine packages now build both formats: `client`, `ai-node`, `ai-otel`, `claude-agents`, `claude-messages`, `openai-agents`, `openai-messages`, `langchain-agents`, and `langchain-messages`. - Each `exports` map gains a `require` branch: ```json ".": { "import": { "types": "./dist/index.d.ts", "default": "./dist/index.js" }, "require": { "types": "./dist/index.d.cts", "default": "./dist/index.cjs" } } ``` - In `integration-tests/commonjs-webpack/commonjs-webpack.test.ts`, the `externalized` case flips from an expected failure to an expected success. The suite also adds a `require()` consumer and an `import` consumer that run from packed tarballs on Node 22 and Node 24. Ask 4 wants a support matrix. PR #76 owns that matrix. This PR only updates its rows in the root README, because five load paths now work instead of three. It adds no new bundler and no new row. Asks 2 and 4 belong to PR #76. Ask 3 belongs to PR #76 plus PR #79. Ask 5 belongs to PR #78. ### Changes - This PR adopts the build that js-core already uses. 11 js-core packages build with `tsup`, and 23 of its 32 packages publish a `require` condition. The AI SDK published none, so it was the outlier. - Nine packages move from `tsc` to `tsup-node`. They share one config at `tsup.config.base.ts`. - Each `exports` map gets a `require` condition. It points to `dist/index.cjs` and `dist/index.d.cts`. The fields `main`, `module`, and `types` stay aligned for old tools. - The `externalized` test case from PR1 now expects success. The suite also adds `require()` and `import` consumers, tarball checks, and manifest checks. - The CJS output keeps optional dependencies behind a runtime `import()`. So users still choose to install OpenTelemetry, LangChain providers, and `@launchdarkly/node-server-sdk`. - The integration job in CI now runs on Node 22 and Node 24. <details> <summary>Implementation details</summary> ### The build Each package holds one line of config: ```ts export { default } from '../../tsup.config.base'; ``` The shared config is small: ```ts // tsup.config.base.ts export default defineConfig({ entry: { index: 'src/index.ts' }, format: ['esm', 'cjs'], splitting: false, sourcemap: true, clean: true, dts: true, }); ``` The scripts `prepare`, `build`, and `build:watch` call `tsup-node`. ### Consistency with js-core This PR adds no new pattern. It copies the current js-core build. - The config in `packages/sdk/server-ai/tsup.config.ts` matches the config here field for field: the same entry, the same two formats, `splitting: false`, sourcemaps, clean output, and declarations. - The manifest shape matches too. The field `main` points to the `.cjs` file. The field `module` points to the `.js` file. The `exports` map holds a `require` branch and an `import` branch, and each branch names its own declaration file. - Two cosmetic differences remain. js-core lists `require` before `import`. Our legacy fields omit the leading `./`. Node matches conditions by name and not by order, so neither difference changes behavior. ### Bundler coverage CI proves one bundler: Webpack 5 with `webpack-node-externals`. That tool produced the incident, so it comes first. A `require` condition is the standard mechanism, so esbuild, Rollup, and Vite should now work too. I did not test them, so the README does not claim them. ### Optional dependencies esbuild keeps `await import('...')` of an external package as a real dynamic import in the CJS file. A static `require` would force users to install every optional package. The test suite reads each `dist/index.cjs`. It asserts the file contains `import("@launchdarkly/node-server-sdk")`. It asserts the file does not contain `require("@launchdarkly/node-server-sdk")`. ### TypeScript project layout The declaration build reads files outside the old glob `src/*.ts`. That broke the `composite` project build with error TS6307. Each package `tsconfig.json` drops `composite`. A new file `tsconfig.ref.json` holds `composite: true` and `include: ["src/**/*"]`. The root `tsconfig.json` points at those files. The root `build` and `clean` scripts now call workspace scripts. The build runs `@launchdarkly/ai-server` first, because the handler packages read types from its `dist` folder. ### Alternatives that I rejected A second `tsc` pass plus a `dist/package.json` shim adds more moving parts. It also produces no `.d.cts` file. A custom esbuild wrapper repeats what `tsup` already does. js-core already uses `tsup`. ### Scope I changed no source imports and no runtime logic. The package `@launchdarkly/ai-node` stays a pure barrel. I updated the READMEs, because CommonJS is now supported. ### Testing These commands pass: `yarn build`, `yarn typecheck`, `yarn code:check`, `yarn lint:pkg`, and `yarn test`. I ran `npm pack --dry-run` for all nine packages. Each tarball holds `dist/index.js`, `dist/index.cjs`, `dist/index.d.ts`, and `dist/index.d.cts`. The command `yarn test:integration` passes 21 of 21 tests on Node 22.23.2 and on Node 24.16.0. That run covers the `externalized` case, the `allowlisted` case, and the `require` and `import` consumers. </details> Link to Devin session: https://app.devin.ai/sessions/c730a69bf9534f648dd5cb5f1db6a28b Open in Devin Desktop: https://app.devin.ai/desktop/session/c730a69bf9534f648dd5cb5f1db6a28b?variant=devin Requested by: @tonytrinh3 [AIC-3415]: https://launchdarkly.atlassian.net/browse/AIC-3415?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ [AIC-3370]: https://launchdarkly.atlassian.net/browse/AIC-3370?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Overview** > **Adds CommonJS publishing across the AI SDK** so `require()`, Webpack externalized bundles, and plain CJS/ESM consumers resolve published packages instead of hitting `ERR_PACKAGE_PATH_NOT_EXPORTED` (AIC-3370). > > All nine publishable workspaces switch from **`tsc` to shared `tsup`** (`tsup.config.base.ts`), emitting **`dist/index.js` + `dist/index.cjs`** and matching **`index.d.ts` / `index.d.cts`**. Each `package.json` gains a **`require` branch** in `exports` and sets **`main`** to the CJS file. Root **`build`/`clean`** run workspace scripts ( **`@launchdarkly/ai-server` first** ); TypeScript **`composite`** moves to **`tsconfig.ref.json`** per package. > > **Integration tests and CI** flip the Webpack **externalized** case to expect success, add **`entrypoints.cjs` / `entrypoints.mjs`**, tarball/manifest checks, and assertions that optional peers stay on **runtime `import()`** in CJS. The CommonJS/Webpack job runs on **Node 22 and 24**. READMEs document dual-format support and updated load-path matrix. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 9bb4956. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
…to tonytrinh3/AIC-3417-lambda-release-canary
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
… import [3/4] (#78) ## Summary Read this first: this PR only changes error text. It fixes no load failure. It adds no feature. It does not fix the original [AIC-3370](https://launchdarkly.atlassian.net/browse/AIC-3370) crash, because that crash happened before any SDK code ran. PR #77 fixed that part. | | | || || || || || || || || | **Plain version:** The SDK loads a few optional packages while it starts. A bundler can break that load. The old error message said the package is missing. That message was wrong, because the package is installed. The import still fails. The message now states the real cause and the fix. **What it means for the code (junior eng):** `packages/client/src/lifecycle.ts` loads optional packages with `await import()`. A bundler can turn that call into `require()`. `require()` cannot load an ESM-only package. ESM means ECMAScript Modules, the `import`/`export` module format. Node then throws `ERR_REQUIRE_ESM` or a similar error. Both catch blocks used to report "not installed" for every error. Now a classifier reads the error first. Closes [AIC-3416](https://launchdarkly.atlassian.net/browse/AIC-3416). This PR is part of [AIC-3370](https://launchdarkly.atlassian.net/browse/AIC-3370). This PR is stacked on #77 ([AIC-3415](https://launchdarkly.atlassian.net/browse/AIC-3415)). - The new file `packages/client/src/import-diagnostics.ts` sorts a failed import into three groups. The groups are ESM interoperability, a missing package, and an unknown error. - The ESM message names the package. It contains the phrase `ESM package was externalized into CommonJS output`. It gives the fix `allowlist: [/^@launchdarkly\/ai-/]`. It links the Lambda recipe in `packages/ai-node/README.md`. - Every new error keeps the original error in `cause`. - An unknown error passes through without a new label. Before this PR, the SDK called every failure "not installed". - The telemetry path warns one time per process. `shutdownTelemetry()` resets that state. The node SDK path still throws. ## Which AIC-3370 asks this PR answers **Plain version:** This PR answers ask 5 only. Ask 5 wants an error message that names the real cause. The message names the package. It says the bundler externalized an ESM package. It gives the fix. It links the Lambda recipe. It keeps the original error, so no detail is lost. **What it means for the code (junior eng):** `esmExternalizationDiagnostic` in `packages/client/src/import-diagnostics.ts` builds the text. The text contains `ESM package was externalized into CommonJS output`. It contains `allowlist: [/^@launchdarkly\/ai-/]`. It links the recipe in `packages/ai-node/README.md`. `describeImportFailure` sets `cause` to the original error. `packages/client/src/__tests__/import-diagnostics.test.ts` covers the codes, the message forms, a real `MODULE_NOT_FOUND`, an unknown error, the preserved `cause`, and the warn-once behavior. The limit matters. The diagnostic covers the optional dynamic imports in `packages/client/src/lifecycle.ts`. Those imports run after #77 fixes root package resolution. The diagnostic cannot catch the original root import failure. That failure happens in the Node resolver, before our code runs. This PR answers no other ask. Ask 1 is #77. Ask 2 is #76. Ask 4 is #76. Ask 3 is #76 plus #79. <details> <summary>Implementation details</summary> ### The old behavior Both optional imports discarded the error: ```ts // setupTelemetry } catch { console.warn('...OpenTelemetry SDK packages are not installed...'); return; } // initBaseClient } catch { throw new Error('...node-server-sdk is not installed...'); } ``` Both messages are wrong when the package is present. The bundler is the real cause. ### The new behavior ```ts // setupTelemetry } catch (err) { if (isEsmInteropError(err)) { warnEsmExternalizationOnce(moduleNameFromError(err, 'an OpenTelemetry SDK package'), err); return; } // the peer dependency install message stays the same } // initBaseClient } catch (err) { throw describeImportFailure('@launchdarkly/node-server-sdk', err, MISSING_NODE_SDK_MESSAGE); } ``` `describeImportFailure(moduleName, error, missingDependencyMessage)` returns one of three values. It returns the ESM message with `cause`. It returns the install message with `cause`. It returns the original error object. ### How the classifier decides The classifier reads the error `code` first. It treats `ERR_REQUIRE_ESM` and `ERR_PACKAGE_PATH_NOT_EXPORTED` as ESM interoperability errors. It treats `MODULE_NOT_FOUND` and `ERR_MODULE_NOT_FOUND` as a missing package. The classifier then reads the message text. A bundled loader does not always keep the `code` property. The message patterns are `require() of ES Module`, `Must use import to load ES Module`, `No "exports" main defined`, `is not defined by "exports"`, and `Cannot find module/package`. `moduleNameFromError` reads the package name from the quoted specifier. It also reads the name from a `node_modules/<pkg>` path. The caller passes a fallback name, so the message never shows an empty name. ### Scope limit This PR does not catch the original failure from before PR2. That failure happened in the package resolver, before any SDK code ran. PR #77 removed that failure. ### One behavior change to review The node SDK path used to report "not installed" for every failure. An unrelated failure now propagates unchanged. An example is a `TypeError` from inside the package itself. ### Alternatives that I rejected I did not export the classifier from the public barrel. The diagnostic is internal. I did not keep the logic inside `lifecycle.ts`. A separate file is easier to test. Vitest replaces a throwing import mock with its own factory error. That made the lifecycle tests flaky. ### Testing `packages/client/src/__tests__/import-diagnostics.test.ts` covers each error code and each message form. It checks that a real `MODULE_NOT_FOUND` keeps the install message. It checks that an unknown error passes through by identity. It checks a non-error throw. It checks `cause`. It checks module name extraction from a specifier, from a path, and from the fallback. It checks the message content. It checks the warn-once behavior and the reset. I ran these commands from the repo root, and all passed: `yarn build`, `yarn typecheck`, `yarn code:check`, `yarn lint:pkg`, `yarn test` (395 client tests, three runs to check for flakes), and `yarn test:integration` (21 tests from the PR1 harness). This PR changes no UI. There is no staging preview. ### Reviewer guide 1. Read `packages/client/src/import-diagnostics.ts` first. It holds the classifier and the message text. 2. Read the two catch blocks in `packages/client/src/lifecycle.ts`. The telemetry path still warns and continues. The node SDK path still throws. 3. Read `packages/client/src/__tests__/import-diagnostics.test.ts`. </details> [AIC-3370]: https://launchdarkly.atlassian.net/browse/AIC-3370?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ [AIC-3416]: https://launchdarkly.atlassian.net/browse/AIC-3416?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ Link to Devin session: https://app.devin.ai/sessions/d378cea89d3841e19600978ae3c8f2ad Open in Devin Desktop: https://app.devin.ai/desktop/session/d378cea89d3841e19600978ae3c8f2ad?variant=devin Requested by: @tonytrinh3 <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Overview** > Improves **error messaging only** when optional `await import()` calls in client initialization fail because a bundler externalized ESM packages into CommonJS (misleading “not installed” text before). > > Adds **`import-diagnostics`** to classify failures: ESM interop (`ERR_REQUIRE_ESM`, message patterns), genuine missing modules, or pass-through for unrelated errors. ESM cases get a diagnostic naming the package, an `allowlist: [/^@launchdarkly\/ai-/]` hint, and a link to the Lambda/Webpack recipe; wrapped errors keep the original in **`cause`**. > > **`lifecycle.ts`** now uses this in the OpenTelemetry setup catch (one-time `console.warn` via `warnEsmExternalizationOnce`, reset on `shutdownTelemetry`) and in the `@launchdarkly/node-server-sdk` import catch (`describeImportFailure` still throws, but with the right message). True `MODULE_NOT_FOUND` still shows install guidance; other failures are no longer mislabeled as missing deps. > > Vitest coverage in **`import-diagnostics.test.ts`** exercises classification, module name extraction, diagnostics, and warn-once behavior. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit dc16a78. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…ambda-release-canary Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> # Conflicts: # packages/client/src/__tests__/import-diagnostics.test.ts # packages/client/src/import-diagnostics.ts
jeffdupont
left a comment
There was a problem hiding this comment.
This canary does what it says, and I checked that by making it fire locally. Building the fixture against the pre-fix 8 Sep releases (ai-server 0.2.0, ai-node 0.2.0, openai-messages 0.2.0, ai-otel 0.1.1) and invoking the packaged handler.js throws ERR_PACKAGE_PATH_NOT_EXPORTED No "exports" main defined, which is the AIC-3370 error. Webpack turns the handler's import() calls into require(), so the broken path is the one being tested. Against the current releases (0.4.0 / 0.3.0 / 0.4.0 / 0.2.0), all six capabilities load with the right types, and assert rejects only the runtime because I ran Node 24 locally. build --package also works with no AWS credentials. #76, #77 and #78 are all merged, the base is main, and CI is green. I did not run the AWS deploy, invoke or remove steps, because there is no role yet.
Since the AWS role doesn't exist yet, this README is what whoever creates it will follow, so it's worth getting right now.
1. Must fix: the README asks for a role that can escalate its own privileges.
- The trust policy allows
subrepo:launchdarkly/js-ai-sdk:*, so any branch can assume the role. Anyone with write access can push a branch that edits this workflow and then dispatch it. - The permissions include
iam:CreateRole,PutRolePolicy,AttachRolePolicyandPassRole, pluslambda:CreateFunctionandInvokeFunction. Even scoped told-ai-canary-*, that's enough to create a role, give it any policy, and run code as it.
Fix:
- Limit
subtorepo:launchdarkly/js-ai-sdk:ref:refs/heads/main. The release push runs frommain, and a canary dispatch can be started frommaintoo. - Drop
AttachRolePolicyandDetachRolePolicy. The generated stack only uses inlinePoliciesonIamRoleLambdaExecution; I checked the packaged template. - Require a permissions boundary on
CreateRoleandPutRolePolicy(aniam:PermissionsBoundarycondition), and setprovider.iam.role.permissionsBoundaryinserverless.yml.
2. Should fix: AWS credentials are set before npm install runs. Configure AWS credentials comes before Build the canary package, and the build runs npm install for the Serverless 3 / Webpack toolchain. Every install script in that tree runs with the session credentials in its environment. The build doesn't need AWS: I ran build --package with the credential and config files set to /dev/null, and it packaged fine. Moving the credentials step to just before Deploy fixes it. It also means the cleanup step's cd .canary/app no longer fails when the credentials step is what failed.
3. Should fix: the README's debug command points at a path that doesn't exist. After serverless package, .webpack/ is gone (serverless-webpack deletes it), so require('./.webpack/service/handler.js') fails with MODULE_NOT_FOUND. Unzipping .serverless/ld-ai-canary.zip and requiring handler.js from there works; that's how I reproduced the bug. Alternatively, set keepOutputDirectory: true under custom.webpack.
Limit the OIDC subject to main. Drop the attach-policy actions. Require a permissions boundary. Set the AWS credentials after the build. Point the debug command at the packaged zip. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
Plain version: Thanks Jeff. I fixed all five points in bd31003. What it means for the code (junior eng):
I verified the packaging locally against the current releases. The template drops |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit bd31003. Configure here.
…not run Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
jeffdupont
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround. I re-checked all four points against 41e0fdf, and they're fixed:
- The OIDC
subis limited toref:refs/heads/main. The job has noenvironment:, so that matches both the release push and a dispatch frommain. AttachRolePolicyandDetachRolePolicyare gone, and the boundary is required in both places.- The AWS credentials now start after the build, so
npm installnever sees them. - The debug command unzips the package first, and
.unzipped/sits under the ignored.canary/.
The cleanup gate on steps.aws-credentials.outcome also fixes Bugbot's point. CI is green.
One thing before approval: the README never names the repo variable for the boundary (inline on README.md:117). The workflow reads vars.AWS_LAMBDA_CANARY_PERMISSIONS_BOUNDARY, but the README only names the fixture's env var LD_AI_CANARY_PERMISSIONS_BOUNDARY, and the variable table lists only the role ARN and region. Someone who follows the README sets the role ARN but not the boundary. The job then runs, because it's gated on the role ARN only. The template has no PermissionsBoundary, so the role's own condition denies CreateRole, and the first real canary fails at Deploy with an IAM AccessDenied. That fails safe, but it's a confusing first run for whoever sets this up.
There's one optional cleanup inline too. I didn't run the deploy, invoke or remove steps, because the role still doesn't exist.
…b on it Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
jeffdupont
left a comment
There was a problem hiding this comment.
11acd8d fixes both remaining points: the README names AWS_LAMBDA_CANARY_PERMISSIONS_BOUNDARY, and the job is gated on it. The dead folder check is gone too. CI is green. Approving.
The job stays skipped until someone creates the AWS role (main-only sub, permissions boundary) and sets both repository variables, so merging this is safe now.
|
thank you for the thorough review Jeff! |

Summary
Plain version: We publish packages to npm. A customer installs them and runs them on AWS Lambda.
This change adds a test that does the same thing right after each release.
The test installs the published packages, runs them in a real AWS Lambda, and then deletes that Lambda.
If the test fails, the packages are broken for customers, and we know in minutes.
What it means for the code (junior eng): A canary is a small test against production artifacts.
This canary closes AIC-3370.
The suite from PR1 packs the workspace with
npm pack, so it tests the tarball that we plan to ship.This canary tests the artifact that npm actually serves, in the runtime that customers use.
Closes AIC-3417.
AIC-3416 is merged.
The base branch is
mainnow. I mergedmaininto this branch and resolved the conflicts.id-tokenpermission here serves npm trusted publishing only. The job readsvars.AWS_LAMBDA_CANARY_ROLE_ARN. GitHub skips the job while that variable is empty. I added no credentials. The README lists the trust policy and the permissions.integration-tests/lambda-commonjs/holds the fixture. It uses Serverless 3, Webpack 5, andwebpack-node-externals3. The output iscommonjs2. The runtime isnodejs22.x. The fixture sets no externals allowlist, so Node resolves the LaunchDarkly packages at runtime. That path is the AIC-3370 failure path.canary.mjshas three commands.waitpolls npm.buildinstalls the exact versions and packages the app.assertchecks the payload from the Lambda. The assert step requires the exact type of each export.release-please.ymlgets arelease-canaryjob. A newcanary_versionsdispatch input runs the canary on demand. The cleanup step runs underif: always(). The step fails the job when it cannot delete the stack or the log group.@launchdarkly/ai-servertoo.@launchdarkly/ai-nodedepends on the core package with*, so npm would otherwise pick any visible core version. A core-only release now also runs the canary.Which AIC-3370 asks this PR answers
Plain version: This PR answers ask 3. It runs the packages that npm serves inside a real Lambda after each release.
It does not answer the other four asks. Other PRs in the stack answer those.
What it means for the code (junior eng): This PR completes ask 3.
It installs exact published npm versions. It packages a CommonJS Lambda with Webpack 5 and
webpack-node-externals3.It deploys that Lambda on Node 22. It invokes the Lambda after each release.
Ask 3 has two halves. #76 covers the packed-tarball half. This PR covers the published-artifact half.
The real AWS path is gated on
vars.AWS_LAMBDA_CANARY_ROLE_ARN.No approved AWS account exists yet. No GitHub OIDC role exists yet.
So the deploy path, the invoke path, and the cleanup path are unverified against real AWS.
This PR does not answer asks 1, 2, 4, or 5. #77 answers ask 1. #76 answers asks 2 and 4. #78 answers ask 5.
Implementation details
Why this test exists
The suite in
integration-tests/commonjs-webpackpacks the workspace withnpm pack.That suite cannot catch a publish-time regression.
Examples are a wrong
fileslist, a staledistfolder, or a bad exports map in the published tag.That suite also does not run on Lambda.
The difference is real today. I invoked the generated bundle against the published
0.2.0artifacts.That error is the AIC-3370 bug. The canary passes after we publish the dual output from PR2.
The driver
canary.mjsholds all of the logic. It takes one exact version for each package.You pass
--ai-node=0.3.0and the other flags.You can also set
LD_AI_CANARY_VERSIONS='ai-node=0.3.0,openai-messages=0.3.0,ai-otel=0.2.0'.waitnpm view <pkg>@<version> versionup to 30 times, 10 seconds apart. A new publish is not readable at once.buildfixture/into.canary/app. Runsnpm install --save-exact. Checks the tree withnpm ls. Writesversions.json. Runsserverless packagewhen you pass--package.assertThe driver checks exactness two times.
It first matches the input against
/^\d+\.\d+\.\d+(?:-[0-9A-Za-z-.]+)?$/.It then compares the installed tree from
npm ls --depth=0 --jsonagainst the request.The build writes
versions.jsoninto the app, and the handler returns that file.So the assert step checks the versions that the Lambda loaded.
It does not trust the versions that CI asked for.
The handler returns this shape. The assert step compares each capability against
EXPECTED_CAPABILITIES.{ "ok": true, "runtime": "v22.x", "versions": { }, "capabilities": { "config": "function", "initClient": "function", "shutdown": "function", "openaiMessages": "function", "aiOtel": "object" } }The workflow job
A push reads the versions from
.release-please-manifest.json.A manual run reads them from the
canary_versionsinput.Each run and attempt gets its own stage, so the function name never collides.
The existing publish jobs do not change.
manual-publishnow also requires an emptycanary_versions.So a canary-only manual run publishes nothing.
The cleanup step fails loudly. It ignores only a log group that AWS already removed.
Decisions
nodejs22.x. The package step prints a warning.configValidationMode: warnkeeps it a warning. CloudFormation accepts the runtime. The canary readsprocess.versionand fails on any runtime that is not Node 22.aws-actions/configure-aws-credentialsis pinned to a SHA for v4.3.1. This matches the pinning style of this repository..canary/is now in.gitignoreand in the Biome ignore list. Serverless writes generated CloudFormation JSON there, andcode:checkfails on those files.Testing
yarn code:fix,yarn code:check,yarn lint:pkg,yarn build,yarn typecheck, andyarn testall pass.canary.test.tsadds 8 tests toyarn test:integration. The tests use no network and no AWS account. They cover the happy path, a version that is not exact, a version mismatch, a wrong runtime, an export that did not resolve, an export with the wrong type, a missing export, and an error payload from Lambda.Reviewer guide
integration-tests/lambda-commonjs/README.mdfor the AWS trust policy, the permissions, and the owner.release-canaryjob in.github/workflows/release-please.yml. Check theifcondition, the OIDC step, and the cleanup step.canary.mjsfor the version checks and the assert contract.fixture/. Confirm that you want no externals allowlist in this test.Link to Devin session: https://app.devin.ai/sessions/bc2f6d69a1b54d9a8ee7b3f1cdee2f8c
Open in Devin Desktop: https://app.devin.ai/desktop/session/bc2f6d69a1b54d9a8ee7b3f1cdee2f8c?variant=devin
Requested by: @tonytrinh3
Note
Overview
Adds a published-artifact release canary that runs after relevant npm publishes (or on demand) to catch CommonJS Lambda load failures like AIC-3370.
The new
integration-tests/lambda-commonjsharness usescanary.mjsto poll npm for exact semver versions, stage a Serverless 3 + Webpack 5 +webpack-node-externalsapp with no externals allowlist, and validate an invoke payload (Node 22 runtime, export types, installed versions).canary.test.tscovers assert/version guards offline;yarn test:integrationnow includes it. Generated.canary/output is gitignored and excluded from Biome.release-please.ymlgains arelease-canaryjob (GitHub OIDC → ephemeralld-ai-canary-*deploy/invoke/cleanup) gated onAWS_LAMBDA_CANARY_ROLE_ARNandAWS_LAMBDA_CANARY_PERMISSIONS_BOUNDARY, plus optionalcanary_versionsonworkflow_dispatchfor canary-only runs. Manual publish is skipped whencanary_versionsis set.Reviewed by Cursor Bugbot for commit 11acd8d. Bugbot is set up for automated code reviews on this repo. Configure here.