diff --git a/.abca/commands/review_pr.md b/.abca/commands/review_pr.md index 10cb2bb0..7a597dd6 100644 --- a/.abca/commands/review_pr.md +++ b/.abca/commands/review_pr.md @@ -97,6 +97,13 @@ Then apply principal-architect judgment over the diff: IDs, cdk-nag clean. Watch for cost and operational footguns. - **Tests** — Are unit tests added/updated under the matching `*/test/` tree? Do they cover the new behavior and failure paths, not just the happy path? +- **Test performance (CDK synth)** — New/changed CDK tests must not re-enable Lambda bundling at + synth or synthesize the same stack repeatedly. `cdk/` disables bundling globally via + `test/setup/disable-bundling.ts` (~15× faster synth); flag any test that turns + `aws:cdk:bundling-stacks` back on (only valid via `postCliContext`, not constructor + `context` — the env var overwrites the latter) without asserting on a bundled asset, or + that calls `new App()` + `Template.fromStack()` per-test instead of once in `beforeAll`. + See #366. - **Routing** — Changes should land in the right package per the AGENTS.md routing table (agent runtime in `agent/`, API/Lambdas in `cdk/`, CLI in `cli/`). diff --git a/AGENTS.md b/AGENTS.md index 0f6f2c0f..e11a9acf 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -45,6 +45,7 @@ Handler entry tests: `cdk/test/handlers/orchestrate-task.test.ts`, `create-task. - Adding or editing files in **`docs/design/`** or **`docs/guides/`** without running **`cd docs && node scripts/sync-starlight.mjs`** — CI will reject ("Fail build on mutation") because the Starlight mirror files in `docs/src/content/docs/` are stale. Always commit the regenerated mirrors alongside source changes. - Changing **`cdk/.../types.ts`** without updating **`cli/src/types.ts`** — CLI and API drift. - Running raw **`jest`/`tsc`/`cdk`** from muscle memory — prefer **`mise //cdk:test`**, **`mise //cdk:compile`**, **`mise //cdk:synth`** (see [Commands you can use](#commands-you-can-use)). +- **Bundling Lambda assets in CDK unit tests** — `Template.fromStack()` triggers a full synth that bundles every `NodejsFunction` via esbuild (~28s for `AgentStack`). Unit tests assert on CloudFormation structure, not bundled code, so for those assertions it is overhead — plus a smoke-test of Lambda `entry` resolution and handler compilation that unit tests intentionally cede to the `agent/` build (a broken entry path or handler TS error stops surfacing at `Template.fromStack()` and instead fails at real synth/deploy or in the `agent/` build). The `cdk/` Jest config disables it globally via `test/setup/disable-bundling.ts` (sets `aws:cdk:bundling-stacks: []` in `CDK_CONTEXT_JSON`), which a bare `new App()` picks up automatically. This does not stop tests from synthesizing — `Template.fromStack()` still runs a full synth; it only skips the esbuild step. **Do not re-enable bundling** in a test unless you are specifically asserting on the bundled-asset output (hash / S3 key) — and if you must, opt out narrowly via that test's `App` `postCliContext` (e.g. `new App({ postCliContext: { 'aws:cdk:bundling-stacks': ['**'] } })`), not globally. Note that constructor `context` does **not** work for this key: CDK overwrites it with `CDK_CONTEXT_JSON`, so only `postCliContext` (applied last) overrides the global disable. Minimize full-stack synths regardless: synthesize each distinct stack config once in `beforeAll` and assert against the cached `Template`. See #366. - **`MISE_EXPERIMENTAL=1`** — required for namespaced tasks like **`mise //cdk:build`** (see [CONTRIBUTING.md](./CONTRIBUTING.md)). - **`mise run build`** builds **`//agent:quality`** alongside **`//cdk:build`** (the deployed image bundles **`agent/`**, so agent quality is part of the build) — these run as parallel `depends`, not in a fixed order; agent changes belong in the **`agent/`** tree. - **`prek install`** fails if Git **`core.hooksPath`** is set — another hook manager owns hooks; see [CONTRIBUTING.md](./CONTRIBUTING.md). diff --git a/cdk/package.json b/cdk/package.json index 5968b096..a1097dda 100644 --- a/cdk/package.json +++ b/cdk/package.json @@ -106,6 +106,9 @@ "watchPathIgnorePatterns": [ "/node_modules/" ], + "setupFiles": [ + "/test/setup/disable-bundling.ts" + ], "setupFilesAfterEnv": [ "aws-cdk-lib/testhelpers/jest-autoclean" ], diff --git a/cdk/test/setup/disable-bundling.test.ts b/cdk/test/setup/disable-bundling.test.ts new file mode 100644 index 00000000..d7c5d9c8 --- /dev/null +++ b/cdk/test/setup/disable-bundling.test.ts @@ -0,0 +1,68 @@ +/** + * MIT No Attribution + * + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * the Software without restriction, including without limitation the rights to + * use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of + * the Software, and to permit persons to whom the Software is furnished to do so. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + */ + +// Executable guard for the global Lambda-bundling disable (#366). +// +// The speedup in this package depends entirely on `test/setup/disable-bundling.ts` +// running as a Jest `setupFiles` entry. Until now the only regression guards +// were prose in AGENTS.md and a checklist item in review_pr.md — both +// honor-system. If someone drops the `setupFiles` wiring, reorders Jest setup, +// or a CDK rename breaks the context key, the suite silently reverts to a +// full-bundling (~15× slower) synth with no failing check. These assertions +// make that floor machine-enforced. +import { App, Stack } from 'aws-cdk-lib'; + +describe('global Lambda bundling disable', () => { + it('sets aws:cdk:bundling-stacks to [] in the synth context', () => { + // A bare `new App()` reads CDK_CONTEXT_JSON, which setupFiles populated. + expect(new App().node.tryGetContext('aws:cdk:bundling-stacks')).toEqual([]); + }); + + it('makes a bare-App stack report bundlingRequired === false', () => { + const stack = new Stack(new App(), 'BundlingProbe', { + env: { account: '123456789012', region: 'us-east-1' }, + }); + expect(stack.bundlingRequired).toBe(false); + }); +}); + +describe('per-test bundling opt-out (#366)', () => { + // The documented escape hatch for a test that needs real bundled-asset output + // is `postCliContext`, NOT constructor `context`. CDK's + // `App.loadContext(props.context, props.postCliContext)` applies + // `props.context` first, overwrites it with CDK_CONTEXT_JSON (which the global + // disable sets), then applies `postCliContext` last — so only the latter wins. + // These lock that precedence contract against a future CDK reordering; without + // them the one supported opt-out path is unguarded. + it('re-enables bundling via postCliContext (the documented opt-out)', () => { + const app = new App({ postCliContext: { 'aws:cdk:bundling-stacks': ['**'] } }); + const stack = new Stack(app, 'OptOutProbe', { + env: { account: '123456789012', region: 'us-east-1' }, + }); + expect(stack.bundlingRequired).toBe(true); + }); + + it('does NOT re-enable bundling via constructor context (env var clobbers it)', () => { + const app = new App({ context: { 'aws:cdk:bundling-stacks': ['**'] } }); + const stack = new Stack(app, 'ContextOptOutProbe', { + env: { account: '123456789012', region: 'us-east-1' }, + }); + expect(stack.bundlingRequired).toBe(false); + }); +}); diff --git a/cdk/test/setup/disable-bundling.ts b/cdk/test/setup/disable-bundling.ts new file mode 100644 index 00000000..9ec21a9f --- /dev/null +++ b/cdk/test/setup/disable-bundling.ts @@ -0,0 +1,74 @@ +/** + * MIT No Attribution + * + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * the Software without restriction, including without limitation the rights to + * use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of + * the Software, and to permit persons to whom the Software is furnished to do so. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + */ + +// Disable Lambda asset bundling during unit-test synthesis. +// +// `Template.fromStack()` triggers a full CDK synth, which bundles every +// NodejsFunction via esbuild — ~28s for the AgentStack (413 resources). Unit +// tests assert on CloudFormation structure/properties, not bundled Lambda code, +// so for those assertions bundling is overhead — plus a smoke-test of Lambda +// `entry` resolution and handler compilation that unit tests intentionally cede +// to the `agent/` build (real synth/deploy still bundles). Skipping it cuts a +// single synth ~15× (~28.7s -> ~1.9s). See #366. +// +// `aws:cdk:bundling-stacks: []` tells the CLI/synth to bundle no stacks. CDK +// reads CDK_CONTEXT_JSON when an `App` is constructed, so a bare `new App()` +// (the overwhelming-majority pattern in our tests) picks this up with no +// call-site changes. +// +// Precedence matters for the opt-out. CDK's `App.loadContext(props.context, +// props.postCliContext)` applies `props.context` FIRST, then overwrites it with +// CDK_CONTEXT_JSON, then applies `postCliContext` LAST. So for this key the env +// var beats constructor `context` — `new App({ context: { 'aws:cdk:bundling- +// stacks': ['**'] } })` does NOT re-enable bundling (the env var clobbers it). +// +// This does NOT stop tests from synthesizing — `Template.fromStack()` still +// runs a full synth; it only skips the esbuild asset-bundling step within that +// synth. The opt-out below exists solely for the rare test that needs the +// *bundled-asset output itself* (e.g. asserting on a real asset hash / S3 key), +// where an unbundled synth would silently yield a placeholder value. Such a +// test must opt out via `postCliContext` (which wins over the env var), +// constructing its `App` with +// `new App({ postCliContext: { 'aws:cdk:bundling-stacks': ['**'] } })` +// (or the specific stack id). +const BUNDLING_DISABLED_CONTEXT = { 'aws:cdk:bundling-stacks': [] as string[] }; + +const existing = process.env.CDK_CONTEXT_JSON; +if (existing) { + // Preserve any ambient context, but let the bundling disable win + // unconditionally: a pre-set CDK_CONTEXT_JSON that carries + // `aws:cdk:bundling-stacks` must not silently re-enable bundling and defeat + // the speedup. The documented per-test opt-out is `postCliContext` (applied + // AFTER the env var regardless — see above), so spreading our key LAST keeps + // that escape hatch working while making the global disable robust to + // whatever the environment already set. + let ambient: Record; + try { + ambient = JSON.parse(existing); + } catch { + // Without this guard a malformed pre-set var throws here, inside + // `setupFiles`, and fails EVERY test in EVERY file with an opaque parse + // error. Re-throw with the offending value so the blame is localized. + throw new Error(`malformed CDK_CONTEXT_JSON in environment (must be valid JSON): ${existing}`); + } + const merged = { ...ambient, ...BUNDLING_DISABLED_CONTEXT }; + process.env.CDK_CONTEXT_JSON = JSON.stringify(merged); +} else { + process.env.CDK_CONTEXT_JSON = JSON.stringify(BUNDLING_DISABLED_CONTEXT); +}