fix: drop fragile alias-migration custom resource, delete manually instead - #45
Merged
Merged
Conversation
…stead The migration's DeleteAlias call needs fn.functionName (a Ref), so it always depends on the function. Any DependsOn added the other way propagates through SAM to the function's generated Version/Alias too, forming a direct cycle — this is what broke BtfpDev-Api's changeset. Deleted dev's orphaned live alias by hand (aws lambda delete-alias) and removed the custom resource; documented the one-time manual step for any future function migrating onto publishLiveAlias in docs/infra.md.
2 tasks
jjosef
added a commit
that referenced
this pull request
Sep 20, 2026
…d NestJS 12 / Node 26 upgrades (#49) * fixes and changes, adding solution architecture rules * refactor: replace SAM AutoPublishAlias with a native CDK alias and canary The SAM transform (AWS::Serverless-2016-10-31 plus L1 property overrides) needed a one-time manual `aws lambda delete-alias` and had already required five follow-up fixes (#41, #42, #44, #45, #46) for alias-migration cycles and a silently no-op'd invoke permission. publishCurrentAlias now uses lambda.Alias and, only when a canary is requested, codedeploy.LambdaDeploymentGroup with two rollback alarms (alias Errors, and new-version Errors so an old-version error cannot fail a good deploy). Prod keeps the 10%/5min canary. Dev and the SES forwarder cut over immediately with no CodeDeploy deployment, so dev deploys no longer wait on a canary that provides no value there. The alias is named `current`, not `live`: the SAM-managed `live` alias still exists on the deployed functions, and CloudFormation creates before it deletes, so reusing the name would 409 and bring the manual step back. Also drops the BTFP_INVOKE_ALIAS env var (only existed to dodge "version exists") and the fromFunctionAttributes(sameEnvironment) workaround; a real Alias grants invoke permission natively. Adds the repo's first CDK tests (vitest + aws-cdk-lib/assertions) covering the alias, the canary group and alarms, and the absence of the SAM Transform. Deploy note: `cdk diff BtfpDev/Api` shows BffFunction as a replacement (SAM Serverless::Function -> Lambda::Function). The function has an auto-generated name, so CloudFormation creates the new one first. The log group name will change. Prod's first deploy of this is not a canary (new alias on a new function), so review the prod-diff job before approving. * fix: fail closed on missing secrets and CORS origin in production buildMycotaAuthConfig fell back to 'change-me-in-local-env' for JWT_SECRET, and enableCors fell back to origin: true (reflect any origin) with credentials enabled. In a deployed stage where the env var was ever unset (CI defaults the JWT secrets to REPLACE_BEFORE_DEPLOYING_* placeholders), that meant silently running with a publicly known signing secret, or an open credentialed CORS policy. Outside production behavior is unchanged. In production a missing or placeholder JWT_SECRET / WEB_ORIGIN now throws at startup, so a bad deploy fails loudly (and trips the prod canary alarm) instead of running insecurely. Verified before changing: both deployed stages currently have a real JWT_SECRET and WEB_ORIGIN set, so this does not affect a running stage. * fix: reject unknown request fields, and fix edits losing their target thing Turn on forbidNonWhitelisted so an undeclared field is a 400 instead of being silently stripped. Silent stripping is the bug class behind the repeated POST /contributions failures (#22-#26), and it was hiding a second, real bug: SubmitPage put `thingId` inside the contribution payload rather than passing it as api.submitContribution's second argument. CreateThingDto has no thingId, so whitelist stripped it and every "edit an existing entry" was proposed as a brand-new thing with no link to the entry being edited (relying only on a name match in findDuplicate). The page now passes editingId as the top-level thingId the DTO actually declares. Audited every body-accepting endpoint against what the web app really sends (email sign-in, professional verification, quiz, contributions); they all match their DTOs exactly, so strict mode is safe. The MCP controller takes a union type and is not validated by the pipe. The pipe options now live in one exported constant used by both app.ts and the DTO specs, replacing a hand-copied config in the spec that could drift. New specs cover a missing payload, a thingId nested in payload, and unknown top-level fields, plus the valid edit shape. Not exercised in a browser: the edit form needs a signed-in session and a running backend. Covered by the DTO contract tests and typecheck instead. * feat: structured JSON logging and readable stack traces for the BFF Diagnosing the last prod-shaped incident from CloudWatch was slow: stack traces pointed at minified lambda.js:687:76249, Nest log contexts were mangled to "[e]", and every stack was split across separate log events. In production the BFF now logs one JSON object per line (level, time, requestId, context, message, stack) via JsonLogger, so a stack stays inside one event. The Lambda handler runs each invocation inside an AsyncLocalStorage context carrying awsRequestId, so every line, including cold-start logs, is tagged with its request. Local dev keeps Nest's readable console logger; LOG_LEVEL=debug enables debug lines. The bundle now keeps class names (Nest prints them as the log context) and the image ships lambda.js.map so NODE_OPTIONS=--enable-source-maps can map frames back to src/*.ts (the env var is set in the follow-up infra change). Verified against the built bundle in production mode: JSON lines with the right request ids and readable contexts, one event per error, source-mapped frames, and a placeholder JWT_SECRET or missing WEB_ORIGIN aborts startup. * docs: make .cursor/rules the source of truth for agent instructions CLAUDE.md held the detailed procedure while .cursor/rules/sync-before-code-changes deferred to it ("Follow CLAUDE.md"), so the same guidance lived in two places. Move all of it into Cursor rules split by topic and reduce CLAUDE.md and the new AGENTS.md to pointers. - sync-before-code-changes (always): the fetch/compare/PR-merged decision checklist and the merge/rebase guard, now self-contained. - recover-from-merged-branch: the stash-and-restart sequence. Corrected while moving it: the old order (`git checkout main` then `git fetch . origin/main:main`) fails because git refuses to fetch into the checked-out branch, so main is now fast-forwarded before switching to it. CLAUDE.md imports both with @ so Claude Code still loads them every session; AGENTS.md indexes all rules (including bff-typescript-cdk-best-practices) for other tools and points at .claude/skills and the sibling mycota repo. * chore: upgrade dependencies to latest, except the NestJS/Fastify set that waits on mycota Everything that does not depend on the @bubltec/mycota-* packages moves to the registry's latest. NestJS, Fastify, @fastify/*, class-validator and mycota itself are deliberately held back: mycota 0.6.0 pins NestJS 10 as a hard dependency, so the app cannot move first without ending up with two Nest copies. That set is prepared separately and lands once mycota's Nest 12 release is published. Major bumps: - TypeScript 5.9 -> 7.0, vitest 2 -> 5, vite 6 -> 8, @vitejs/plugin-react 4 -> 6, oxfmt 0.59 -> 0.68, lefthook 1 -> 2, esbuild 0.24 -> 0.28, @types/node 26.6.2 - aws-cdk-lib 2.261 -> 2.270, aws-cdk, constructs, aws-sdk clients, playwright, react 19.3, react-router, tailwind, oxlint, turbo, tsx - @base-ui-components/react (deprecated, renamed) -> @base-ui/react 1.8.0 Changes the new toolchain required: - packages/config/tsconfig-node.json: "types": ["node"]. TypeScript 7 no longer auto-includes every @types/* package, so process/console/fetch/URL were unresolved in every Node workspace. - Five web components: import path @base-ui-components/react -> @base-ui/react. No API or type changes were needed. - data/seed/src/scrape-vetmeds.ts: one line wrapped by the newer oxfmt. Verified: typecheck (10 tasks), build, lint, format, and unit tests (137 across bff, scraper, shared-types, seed, infra) all pass. The e2e workspace is not part of this check (CI excludes it; it needs a browser and a deployed target). aws-cdk-lib: synthesized every stack before and after and diffed the templates. Data, DNS, CI and Scraper are unchanged apart from the CDKMetadata version stamp. Api Version/Alias hashes differ only because the bundle was rebuilt by the new compiler. Each Web stack's BucketDeployment handler picks up the library's newer bundled code. No unexpected infrastructure changes. base-ui: verified in a browser against a mock API. Select (opens, options, selection updates the app), Combobox (typing filters to one option, popup-open state), and Dialog (opens centered with backdrop, focus enters the input, Escape closes and focus returns to the trigger) all behave correctly with no console warnings. * chore: save exact versions, and read the Node version from .nvmrc in CI - Add .npmrc with save-exact=true and `saveExact: true` in pnpm-workspace.yaml. Both are needed: tested against pnpm 11.20, `.npmrc` alone is ignored (a bare `pnpm add x` still wrote a caret range) while the workspace setting works. The .npmrc keeps npm and older pnpm consistent. This affects new additions; the existing ranges are pinned in the follow-up upgrade change. - CI and deploy workflows use `node-version-file: .nvmrc` instead of a hard-coded `node-version: 26` (7 places), so the Node version is changed in one place. * chore: pin every dependency to an exact version, and declare @types/node in shared-types Follows the save-exact setting: convert the existing caret ranges in all package.json files (89 specs) to exact versions. Each one is pinned to the version that is actually installed (read from `pnpm ls`), not to the floor of its old range. That matters: stripping the caret would have silently downgraded four packages (@nestjs/common, @nestjs/core and @nestjs/platform-fastify to 10.4.0, and class-validator to 0.14.1, instead of the installed 10.4.22 and 0.14.4). No resolved version changes from the pinning itself: the lockfile diff for it contains only `specifier:` lines. Also fixes a latent bug from the TypeScript 7 upgrade: packages/shared-types extends the Node tsconfig preset (which now sets "types": ["node"]) but never declared @types/node. It only compiled because a hoisted copy happened to exist at the repo root, so the previous commit passed by luck and failed after a clean reinstall (TS2688 "Cannot find type definition file for 'node'"). It is now an explicit devDependency. Verified from scratch: wiped every node_modules, `pnpm install --frozen-lockfile`, then typecheck, build and test across all workspaces except e2e (16 tasks). * docs: correct the architecture rule and record what we learned The rule I wrote earlier praised things that turned out to be weak and did not cover what changed since. Corrections: - lambda-canary.ts is no longer "the model": the SAM transform it used was replaced by a native alias and deployment group, and the rule now says why (five follow-up fixes and a manual migration step). The alias is `current`, not `live`. - SearchService.invalidate() only clears the cache in one container; stated. - Validation: the pipe options are one exported constant, and unknown fields are a 400 (forbidNonWhitelisted) rather than silently dropped. - Fail-closed configuration is now enforced (requireInProduction, corsOrigin), not just advised. - CDK tests exist now; "verification" says so, and requires a before/after synth diff for aws-cdk-lib bumps. Added: logging and alerts, the TypeScript 7 `types`/@types/node requirement, exact-pin policy (pin to the installed version, never strip the caret), the mycota peer-dependency and "upgrade mycota first" workflow, the single Fastify override, Node from .nvmrc, the pnpm 11 quirks (.npmrc save-exact is ignored; the env prefix is pnpm_config_), and four new pitfalls, including the ESM-first `import.meta.url` bundle crash that no unit test could catch. Also a Known Deviations section listing what the repo still does not do (plaintext JWT/Brave env vars, actions pinned by tag not SHA, default oxlint rules, full-table Scan, single AWS account, DTO/shared-types duplication) and the two pieces of work that are written but not yet committed or released. * chore: require Node 26 and enforce it The root `engines` still said >=24 although .nvmrc, CI, the Lambda image and @types/node are all Node 26. Raise it to >=26 and set `engineStrict` so a wrong Node version fails the install instead of only warning. Only `engineStrict` in pnpm-workspace.yaml is honored by pnpm 11; `engine-strict` in .npmrc is ignored. Verified: install under strict engines, then typecheck, build and test across all workspaces except e2e. * chore: upgrade to NestJS 12, Fastify 5 and class-validator 0.15 (mycota 1.0.0) Bumped, all as exact pins: - @nestjs/common, core, platform-fastify 10.4 -> 12.0.3; @nestjs/jwt 12.0.2, @nestjs/passport 12.0.0, @nestjs/config 3.3 -> 12.0.0, @nestjs/swagger 8 -> 12.0.1 - fastify 4 -> 5.12.5, @fastify/cookie 9 -> 11.1.2, @fastify/aws-lambda 4 -> 6.4.1 - class-validator 0.14 -> 0.15.1 - @bubltec/mycota-auth, -dynamo, -professional-verification, -cdk -> 1.0.0 mycota 1.0.0 declares NestJS and class-validator as peer dependencies, so this app owns the single copy. Confirmed in the regenerated lockfile and with `pnpm why`: one @nestjs/common@12.0.3, one fastify, one class-validator, no Nest 10 entries, and every mycota package resolves to the published 1.0.0. Two things this upgrade required: 1. Bundle crashed at startup. @nestjs/swagger 12 is ESM-first and runs `createRequire(import.meta.url)` at load time; esbuild turns import.meta into an empty object in our CommonJS bundle, so it threw and the Lambda could not start (every cold start would have failed). Typecheck and unit tests cannot catch this. build-lambda.mjs now defines import.meta.url via a banner. The UI asset lookup after it only runs from SwaggerModule.setup, which we never call. 2. Two Fastify copies. @nestjs/platform-fastify pins fastify to an exact version as a regular dependency, so the app's own fastify installed a second copy and the @fastify/cookie plugin types stopped lining up (TS2345). A workspace override keeps a single copy; keep it equal to apps/bff's fastify version. No application code changes were needed. Verified from a wiped node_modules with `pnpm install --frozen-lockfile` against the published packages: - typecheck, build, lint, format and unit tests pass (16 turbo tasks, twice) - the real production Lambda bundle, driven with real requests: swagger document generates (200); mycota's JwtAuthGuard and VerifiedGuard resolve through DI and reject anonymous callers (401); mycota's own DTO is validated by class-validator (400 "email must be an email"), which only works with a single copy; strict forbidNonWhitelisted applies to that DTO too (400); unknown routes 404; no error-level log events - cdk synth: every stack identical to before except the Api stacks' image-derived Version/Alias hashes * fix: stop vitest collecting the Lambda build's temporary output `pnpm build` in apps/bff compiles into .tsc-out (see scripts/build-lambda.mjs) while turbo runs the tests in parallel. Vitest 3+ no longer excludes build output, so it collected half-written .tsc-out/*.spec.js copies and failed with "Cannot find module". It had only passed before by timing. Reproduced by running the tests while a build was in progress (failures), and confirmed fixed the same way (8 files, 37 tests pass). Also two forced full turbo runs, both 16/16. apps/bff/vitest.config.ts excludes .tsc-out and dist*. * docs: drop the resolved Nest 12 note from Known Deviations * docs and fixes * fix: remove the redundant HttpApiInvokeAlias CfnPermission Its comment described the old SAM/imported alias, where CDK skipped addPermission because the alias ARN was a token, so the HTTP API had no invoke permission. With the native lambda.Alias, HttpLambdaIntegration already creates that permission itself, so this was a second, duplicate resource policy statement on the same alias. Verified by synthesizing: Dev and Prod each still have exactly one AWS::Lambda::Permission for the API, and it targets the current alias.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The migration's DeleteAlias call needs fn.functionName (a Ref), so it always depends on the function. Any DependsOn added the other way propagates through SAM to the function's generated Version/Alias too, forming a direct cycle — this is what broke BtfpDev-Api's changeset.
Deleted dev's orphaned live alias by hand (aws lambda delete-alias) and removed the custom resource; documented the one-time manual step for any future function migrating onto publishLiveAlias in docs/infra.md.