Skip to content

Modernize NestJS and Lambda build stack - #72

Merged
GelatoGenesis merged 10 commits into
mainfrom
codex/nest-build-audit-hardening
Sep 10, 2026
Merged

GelatoGenesis merged 10 commits into
mainfrom
codex/nest-build-audit-hardening

Conversation

@GelatoGenesis

Copy link
Copy Markdown
Collaborator

Summary

  • upgrade NestJS, Express, Swagger, validation, Sentry, TypeScript, Jest, and ESLint as one Node 24-compatible stack
  • replace aws-serverless-express with maintained @codegenie/serverless-express and cover HTTP/Lambda compatibility
  • replace Serverless v3 and optimizer plugins with reproducible production-only ZIP packaging and sequential AWS CLI Lambda updates
  • add fail-closed production and full-tree high/critical dependency audit gates with exact-path, expiring exceptions
  • preserve Sentry debug-ID source maps and zero-downtime handler-path compatibility during rollout

Verification

  • clean yarn install --frozen-lockfile --registry=https://registry.npmjs.org/ under Node 24.20.0
  • yarn lint
  • yarn build
  • yarn test --runInBand (8 suites, 65 tests)
  • yarn audit:production (0 critical, 0 high, 1 moderate, 0 low)
  • yarn audit:all (0 critical, 0 high, 1 moderate, 0 low)
  • repeated yarn lambda:package && yarn lambda:package:verify produced identical SHA-256 e5b95f7fa320f710b444d91de0cecc2d73a7f0635b20ef9e6ae5ccc33a728663

Deployment

No database migration or provisioned-infrastructure replacement. Phase 3 will merge this checked branch into the latest 1a-staging, preserving the separately deployed issue #65 changes, then run the staging workflow and smoke every Lambda/route. No production deployment is authorized.

Signed-off-by: Gelato Genesis <gelatogenesis@Gelatos-Mac-mini.local>
@coderabbitai

coderabbitai Bot commented Sep 9, 2026 •

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 0e61e97b-5425-4f9c-8f23-4844986a4e26


Comment @coderabbitai help to get the list of available commands.

@6529bot

6529bot Bot commented Sep 9, 2026

Copy link
Copy Markdown

6529bot WCAG 2.2 AA analysis - 42c1a1e

Verdict: No WCAG findings

This PR modernizes the NestJS backend and Lambda build/deploy tooling (dependency upgrades, Sentry migration, packaging scripts, CI workflows). It targets 6529-Collections/allowlist-api, a backend REST API repository with no user-facing frontend/UI code. The changes are limited to server code, build scripts, YAML config, and test files — none of which render HTML, components, or interactive UI subject to WCAG 2.2 AA success criteria.

The only HTML surfaces here (Swagger UI at /api, JSON error responses) are library-generated and unchanged in any accessibility-relevant way. There are no components, templates, ARIA usage, focus management, color, or interactive controls to evaluate.

No accessibility review is applicable to this diff.

@6529bot

6529bot Bot commented Sep 9, 2026

Copy link
Copy Markdown

6529bot crypto security analysis - 42c1a1e

Verdict: No security findings

This PR modernizes the build/deploy/test stack (NestJS 12, Express 5, @codegenie/serverless-express, @sentry/aws-serverless, new Lambda packaging/deploy/audit scripts). I reviewed the wallet/auth/signature-relevant changes and the new scripts against the crypto-security focus areas. The auth behavior is preserved (JWT guard, public-endpoint bypass, wallet lowercasing verified in src/api-bootstrap.spec.ts:177), and the framework/CORS/query-parser changes are functionally compatible.

The deploy/audit scripts use execFileSync with argument arrays (no shell interpolation) and write env files with 0o600, so no injection or secret-file exposure introduced. I found no realistic exploit path in the changed code within scope.

Notes (non-blocking, outside crypto-security scope; verify separately):

  • configureApiApplication continues to call app.enableCors() with fully permissive CORS (access-control-allow-origin: *, confirmed in src/api-bootstrap.ts:18 and the CORS test). This is preserved behavior, not a regression, but combined with JWT-bearer auth it remains worth confirming that no cookie/credentialed flows depend on origin restriction.
  • The resolutions in package.json:129 pin lodash to 4.18.1 — that version does not exist on npm (latest is 4.17.x). This is a build-correctness concern, not a security finding, but it may cause yarn install to fail or resolve unexpectedly; please confirm the intended pin.

Coverage caveat: this is large-PR mode with a partially truncated diff (notably the full yarn.lock and 29 omitted relevant files), so this is not an exhaustive review — it prioritizes high-confidence findings in the included auth/deploy/packaging changes.

@6529bot

6529bot Bot commented Sep 9, 2026

Copy link
Copy Markdown

6529bot i18n analysis - 42c1a1e

Verdict: No i18n findings

This PR modernizes the NestJS backend and Lambda build/deploy tooling (dependency upgrades, @sentry/serverless → @sentry/aws-serverless, Express 5 / Nest 12 migration, packaging/audit scripts, CI workflows). It targets 6529-Collections/allowlist-api, a backend REST API with no localized frontend surface.

There is no i18n/ message structure, no React UI, and no t(locale, key, params) usage in this repository. The user-facing strings touched here are all backend-only and outside the scope of the progressive i18n standard:

  • CLI/console output in build scripts (scripts/audit-dependencies.cjs, new-api-endpoint-generator-wrapper.js) is developer tooling, not localized product UI.
  • API error messages (BadRequestException/NotFoundException strings in allowlist.service.ts, other.service.ts, etc.) are REST API response payloads, not React copy with an i18n path.
  • Swagger metadata (DocumentBuilder().setTitle(...), @ApiProperty({ description })) is API documentation, not end-user localized UI.
  • Discord CI notification strings in the workflows are operational alerts.

The .toLowerCase()/.toUpperCase() calls in the endpoint generator and other.service.ts operate on ASCII identifiers (controller names, operation-type enum keys), not locale-sensitive display values, so the normalizeLocale/compareLocalized helper guidance does not apply.

No i18n review is applicable to this diff. Given large PR mode and partial/truncated context, this is not an exhaustive audit, but no localized frontend surface exists in the included changes.

@6529bot

6529bot Bot commented Sep 9, 2026

Copy link
Copy Markdown

6529bot general PR review - 42c1a1e

Verdict: Needs changes

Important

  • scripts/deploy-lambdas.cjs:112-119 — Environment drift on repeated deploys. desiredEnvironment spreads existing config then always sets SENTRY_ENV to 'staging'/'production'. If the live function already has SENTRY_ENV=staging, environmentChanged is false and the block is skipped—fine. But the first-time comparison uses configuration.environment?.[key], and any pre-existing var that Secrets Manager/runtime injected but is not in desiredEnvironment is preserved (good) while any that AWS trims will not be restored. More concretely: the config update omits --role, --vpc-config, --memory-size, --timeout, etc., so functions updated only via this script rely on those already being set out-of-band. That's acceptable for "update existing" but there is no verification that configuration.handler/runtime pre-existing values match before the first update-function-code. If allowlist-api currently points at src/api-lambda.handler and code is pushed before config, the new zip must contain that path — which it does (src/api-lambda.js compat shim). OK, but note the compat shim require('../dist/api-lambda.js') runs Sentry.init and full bootstrap identically, so the transition claim holds only if dist/ files exist in the same zip. They do. No blocker, but the config update silently drops any env var present live but absent from the merged object only if AWS itself removes it — confirm the merge is truly additive.

  • scripts/audit-dependencies.cjs:31-49 — Retry loop can use a stale/undefined output. If execFileSync throws on attempts 1–2 without auditSummary and without reaching attempt 3, the loop continues but output = error.stdout may be undefined when the command produces no stdout. On a subsequent successful attempt output is reassigned, but if attempt 3 succeeds via the try path the value is fine; if attempts keep failing without summary, attempt 3 hits process.exit(1). The real gap: on a non-final failing attempt where error.stdout is undefined and it's not attempt 3, the loop simply retries — but there's no guarantee output is defined after the loop if the last iteration succeeded silently. Edge case is narrow; consider asserting output is defined before output.split('\n') at line 51 to avoid a confusing Cannot read properties of undefined.

  • src/api-bootstrap.ts:9 — app.getHttpAdapter().getInstance().set('query parser', 'extended') is only valid when the underlying instance is Express. main.ts calls configureApiApplication(app) where app is created via NestFactory.create(AppModule) (default Express platform), so this holds today. But this helper is now shared and will throw if ever invoked against a Fastify adapter. Low risk given current platform, but worth a guard or comment tying it to @nestjs/platform-express.

Nice-to-have

  • README.md:98 claims changing handlers to dist/ "cannot create a code/config transition outage" because compat handlers exist. That's true only while the artifact ships both src/*.js shims and dist/*.js. verify-lambda-package.cjs:39-41 enforces the shims exist, so the invariant is checked — good. Consider also asserting the shim require target resolves (the runtime require('./src/...') in verify does this).

  • dependency-audit-exceptions.json is [] while the SonarCloud gate reports a new B security rating. The audit gate only blocks high/critical advisories from yarn audit; Sonar's finding is independent and won't be caught by these scripts. Confirm the Sonar security finding is addressed separately before merge, since CI here won't surface it.

  • package.json:129 pins "lodash": "4.18.1" — that version does not exist on npm (lodash tops out at 4.17.x). A frozen-lockfile install may still succeed if the lock resolves to something else, but this resolution is either dead or silently ignored. Verify it resolves; otherwise remove it.

Suggested next steps

  • Confirm lodash resolution 4.18.1 actually resolves during yarn install --frozen-lockfile; fix or drop it.
  • Add a defined-check for output before parsing in audit-dependencies.cjs.
  • Verify the SonarCloud new-code security finding independently, since the new audit scripts won't cover it.

Note: Coverage is partial (large PR, 32 relevant files omitted); this review prioritizes high-confidence issues in the included diff.

Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate.

Review comments:
From 6529bot general PR review on 6529-Collections/allowlist-api#72 (42c1a1e7e78b):
**Verdict**: Needs changes

### Important

- `scripts/deploy-lambdas.cjs:112-119` — Environment drift on repeated deploys. `desiredEnvironment` spreads existing config then always sets `SENTRY_ENV` to `'staging'`/`'production'`. If the live function already has `SENTRY_ENV=staging`, `environmentChanged` is false and the block is skipped—fine. But the first-time comparison uses `configuration.environment?.[key]`, and any pre-existing var that Secrets Manager/runtime injected but is *not* in `desiredEnvironment` is preserved (good) while any that AWS trims will not be restored. More concretely: the config update omits `--role`, `--vpc-config`, `--memory-size`, `--timeout`, etc., so functions updated only via this script rely on those already being set out-of-band. That's acceptable for "update existing" but there is no verification that `configuration.handler`/runtime pre-existing values match before the first `update-function-code`. If `allowlist-api` currently points at `src/api-lambda.handler` and code is pushed before config, the new zip must contain that path — which it does (`src/api-lambda.js` compat shim). OK, but note the compat shim `require('../dist/api-lambda.js')` runs `Sentry.init` and full bootstrap identically, so the transition claim holds only if `dist/` files exist in the same zip. They do. No blocker, but the config update silently drops any env var present live but absent from the merged object only if AWS itself removes it — confirm the merge is truly additive.

- `scripts/audit-dependencies.cjs:31-49` — Retry loop can use a stale/undefined `output`. If `execFileSync` throws on attempts 1–2 without `auditSummary` and without reaching attempt 3, the loop continues but `output = error.stdout` may be `undefined` when the command produces no stdout. On a subsequent successful attempt `output` is reassigned, but if attempt 3 *succeeds* via the `try` path the value is fine; if attempts keep failing without summary, attempt 3 hits `process.exit(1)`. The real gap: on a non-final failing attempt where `error.stdout` is undefined and it's not attempt 3, the loop simply retries — but there's no guarantee `output` is defined after the loop if the last iteration succeeded silently. Edge case is narrow; consider asserting `output` is defined before `output.split('\n')` at line 51 to avoid a confusing `Cannot read properties of undefined`.

- `src/api-bootstrap.ts:9` — `app.getHttpAdapter().getInstance().set('query parser', 'extended')` is only valid when the underlying instance is Express. `main.ts` calls `configureApiApplication(app)` where `app` is created via `NestFactory.create(AppModule)` (default Express platform), so this holds today. But this helper is now shared and will throw if ever invoked against a Fastify adapter. Low risk given current platform, but worth a guard or comment tying it to `@nestjs/platform-express`.

### Nice-to-have

- `README.md:98` claims changing handlers to `dist/` "cannot create a code/config transition outage" because compat handlers exist. That's true only while the artifact ships both `src/*.js` shims and `dist/*.js`. `verify-lambda-package.cjs:39-41` enforces the shims exist, so the invariant is checked — good. Consider also asserting the shim `require` target resolves (the runtime `require('./src/...')` in verify does this).

- `dependency-audit-exceptions.json` is `[]` while the SonarCloud gate reports a new B security rating. The audit gate only blocks high/critical advisories from `yarn audit`; Sonar's finding is independent and won't be caught by these scripts. Confirm the Sonar security finding is addressed separately before merge, since CI here won't surface it.

- `package.json:129` pins `"lodash": "4.18.1"` — that version does not exist on npm (lodash tops out at 4.17.x). A frozen-lockfile install may still succeed if the lock resolves to something else, but this resolution is either dead or silently ignored. Verify it resolves; otherwise remove it.

**Suggested next steps**
- Confirm `lodash` resolution `4.18.1` actually resolves during `yarn install --frozen-lockfile`; fix or drop it.
- Add a defined-check for `output` before parsing in `audit-dependencies.cjs`.
- Verify the SonarCloud new-code security finding independently, since the new audit scripts won't cover it.

Note: Coverage is partial (large PR, 32 relevant files omitted); this review prioritizes high-confidence issues in the included diff.

Signed-off-by: Gelato Genesis <gelatogenesis@Gelatos-Mac-mini.local>
@6529bot

6529bot Bot commented Sep 9, 2026

Copy link
Copy Markdown

6529bot follow-up commit review - f859950

Verdict: Needs changes

Resolved since last review

  • scripts/audit-dependencies.cjs:51-53 — The prior "stale/undefined output" gap is fixed: an explicit typeof output !== 'string' guard now precedes the .split('\n') parse.
  • src/api-bootstrap.ts:7-11 — The prior Express-only concern is addressed with an explicit httpAdapter.getType() !== 'express' guard before calling .set('query parser', ...).
  • scripts/deploy-lambdas.cjs:157-187 — Added post-deploy verification (finalConfiguration re-read + changedEnvironmentKeys check) partially addresses the earlier env-drift concern by failing if desired env keys don't land.

Important

  • package.json:129 — "lodash": "4.18.1" is still pinned to a nonexistent version (lodash tops out at 4.17.x). This was flagged in the prior general and security reviews and remains unchanged. A --frozen-lockfile install will fail or silently misresolve. Confirm it resolves or drop it. (Still open from prior review.)

Nice-to-have

  • scripts/deploy-lambdas.cjs:157-187 — The new post-deploy verification re-reads config with --revision-id omitted, so it validates whatever the current live config is, not necessarily the revision this run published. If another deploy races in between, verification could pass against a foreign revision. Low risk in a serialized CI, but worth noting the check isn't revision-pinned.

  • dependency-audit-exceptions.json:1 remains [] while SonarCloud still reports a new B security rating. As previously noted, the audit gate only covers yarn audit high/critical advisories and won't surface the Sonar finding. Confirm the Sonar security issue is resolved separately before merge.

Suggested next steps

  • Resolve or remove the lodash 4.18.1 resolution and re-verify yarn install --frozen-lockfile from a clean checkout.
  • Confirm the SonarCloud new-code security finding is addressed independently of the audit scripts.

Note: Large-PR mode with partial/truncated context (32 relevant files omitted, yarn.lock truncated); this prioritizes high-confidence carry-over items and newly introduced follow-up code, not exhaustive coverage.

Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate.

Review comments:
From 6529bot follow-up commit review on 6529-Collections/allowlist-api#72 (f85995004071):
**Verdict**: Needs changes

### Resolved since last review

- `scripts/audit-dependencies.cjs:51-53` — The prior "stale/undefined `output`" gap is fixed: an explicit `typeof output !== 'string'` guard now precedes the `.split('\n')` parse.
- `src/api-bootstrap.ts:7-11` — The prior Express-only concern is addressed with an explicit `httpAdapter.getType() !== 'express'` guard before calling `.set('query parser', ...)`.
- `scripts/deploy-lambdas.cjs:157-187` — Added post-deploy verification (`finalConfiguration` re-read + `changedEnvironmentKeys` check) partially addresses the earlier env-drift concern by failing if desired env keys don't land.

### Important

- `package.json:129` — `"lodash": "4.18.1"` is still pinned to a nonexistent version (lodash tops out at 4.17.x). This was flagged in the prior general and security reviews and remains unchanged. A `--frozen-lockfile` install will fail or silently misresolve. Confirm it resolves or drop it. (Still open from prior review.)

### Nice-to-have

- `scripts/deploy-lambdas.cjs:157-187` — The new post-deploy verification re-reads config with `--revision-id` omitted, so it validates whatever the current live config is, not necessarily the revision this run published. If another deploy races in between, verification could pass against a foreign revision. Low risk in a serialized CI, but worth noting the check isn't revision-pinned.

- `dependency-audit-exceptions.json:1` remains `[]` while SonarCloud still reports a new B security rating. As previously noted, the audit gate only covers `yarn audit` high/critical advisories and won't surface the Sonar finding. Confirm the Sonar security issue is resolved separately before merge.

**Suggested next steps**
- Resolve or remove the `lodash` `4.18.1` resolution and re-verify `yarn install --frozen-lockfile` from a clean checkout.
- Confirm the SonarCloud new-code security finding is addressed independently of the audit scripts.

Note: Large-PR mode with partial/truncated context (32 relevant files omitted, `yarn.lock` truncated); this prioritizes high-confidence carry-over items and newly introduced follow-up code, not exhaustive coverage.

Signed-off-by: Gelato Genesis <gelatogenesis@Gelatos-Mac-mini.local>
@GelatoGenesis

Copy link
Copy Markdown
Collaborator Author

Review follow-up for f7147c9:

  • Deployment verification is now pinned to the exact RevisionId returned by this run; a concurrent configuration update makes the deployment fail closed.
  • The Express version-disclosure mitigation is now applied directly to the explicit Lambda Express instance as well as in the shared bootstrap.
  • The lodash finding is based on stale registry knowledge. As of 2026-09-09, npm view lodash version returns 4.18.1; yarn why lodash returns Found "lodash@4.18.1"; and both local and GitHub Actions frozen-lockfile installs succeed. Registry release: https://www.npmjs.com/package/lodash/v/4.18.1 . The pin is therefore retained because it removes a real high-severity advisory.

Validation: lint and build pass; all 65 tests pass.

@6529bot

6529bot Bot commented Sep 9, 2026

Copy link
Copy Markdown

6529bot follow-up commit review - f7147c9

Verdict: No new findings

The changes since the prior follow-up marker (f859950) address the outstanding items and add sound hardening. I verified the follow-up code against the diff and found no new issues worth reporting.

Resolved since last review

  • scripts/deploy-lambdas.cjs:129-192 — The prior "verification isn't revision-pinned" nice-to-have is fixed. The config update now captures the returned RevisionId into expectedRevisionId, and post-deploy verification asserts finalConfiguration.revisionId !== expectedRevisionId fails closed. A concurrent update no longer passes verification against a foreign revision.
  • src/api-bootstrap.ts:8-17 / src/api-lambda.ts:25 — The Express version-disclosure concern raised by the human is addressed: x-powered-by is disabled in the shared bootstrap and on the explicit Lambda Express instance, with a test assertion at src/api-bootstrap.spec.ts:188.
  • package.json:129 — The lodash 4.18.1 pin was confirmed by the author to resolve (verified via npm view, yarn why, and frozen-lockfile installs locally and in CI) and removes a real high-severity advisory. Treating this as resolved/intentional; no longer flagging.

Note: Large-PR mode with partial/truncated context (yarn.lock and 32 relevant files truncated/omitted); this prioritizes the newly introduced follow-up code and carry-over items, not exhaustive coverage. The independent SonarCloud "B Security Rating on New Code" gate remains outside what these audit scripts cover — confirm it is addressed separately before merge, but it is not a code finding in this diff.

Gelato Genesis added 2 commits September 9, 2026 18:14
Signed-off-by: Gelato Genesis <gelatogenesis@Gelatos-Mac-mini.local>
Signed-off-by: Gelato Genesis <gelatogenesis@Gelatos-Mac-mini.local>
@6529bot

6529bot Bot commented Sep 9, 2026

Copy link
Copy Markdown

6529bot follow-up commit review - fe45ac6

Verdict: No new findings

The changes since the prior same-lane follow-up marker (f7147c9) are limited to the SonarCloud quality-gate now passing and the human review note documenting the revision-pinned deploy verification, x-powered-by hardening, and the confirmed-real lodash@4.18.1 pin. I re-verified the newly included follow-up code against the diff and found no new issues worth reporting.

Resolved since last review

  • SonarCloud quality gate now passes (2 new issues, 0 security hotspots, B-rating concern from earlier reviews no longer blocking). The independent Sonar security-rating caveat carried across prior reviews is addressed; nothing in this diff regresses it.

Note: Large-PR mode with partial/truncated context (yarn.lock and many relevant files truncated/omitted). Prior findings from this bot, Claude, CodeRabbit, and the human author (lodash pin, revision-pinned deploy verification, x-powered-by, output guard, Express-adapter guard) were all previously resolved and are not repeated here. This review is not exhaustive; it prioritizes high-confidence carry-over items and newly introduced follow-up code.

Signed-off-by: Gelato Genesis <gelatogenesis@Gelatos-Mac-mini.local>
@6529bot

6529bot Bot commented Sep 9, 2026

Copy link
Copy Markdown

6529bot follow-up commit review - ef77e9b

Verdict: No new findings

The changes since the prior same-lane follow-up marker (fe45ac6) add a staging cold-start smoke test (scripts/smoke-staging-lambdas.cjs, lambda:smoke:staging wiring in staging.yml and package.json) plus supporting README/AGENTS docs. I reviewed the newly introduced smoke-test script and workflow wiring against the diff and found no new issues worth reporting. Prior findings (lodash pin, revision-pinned deploy verification, x-powered-by, output guard, Express-adapter guard, Sonar rating) were all resolved in earlier reviews and are not repeated here.

Notes (non-blocking)

  • scripts/smoke-staging-lambdas.cjs:36-42 — The worker/downloader smoke probes send { __allowlistLambdaSmokeTest: true } and expect response.ok === true. The worker/downloader handlers weren't included in this diff excerpt, so I could not confirm they short-circuit on this flag and return { ok: true } without side effects (the README claims a "reserved, side-effect-free bootstrap probe"). Worth confirming the handlers actually recognize this sentinel before relying on the smoke gate; otherwise a real event would be processed or the probe would fail. Not a code finding in the included diff.

  • scripts/smoke-staging-lambdas.cjs:146 — The cold-start assertion requires logs.includes('Init Duration:'). On a warm container (e.g., if the deploy step already invoked the function, or AWS reuses an environment), there is no Init Duration line and this fails closed. Given it runs immediately after a fresh deploy the container is almost always cold, so this is low risk, but a retry that isn't guaranteed cold could produce a flaky failure. Non-blocking.

Note: Large-PR mode with partial/truncated context (yarn.lock and many relevant files truncated/omitted; worker/downloader handlers not in excerpt). This is not exhaustive; it prioritizes the newly introduced smoke-test code and carry-over items.

Signed-off-by: Gelato Genesis <gelatogenesis@Gelatos-Mac-mini.local>
@6529bot

6529bot Bot commented Sep 9, 2026

Copy link
Copy Markdown

6529bot follow-up commit review - acf02fd

Verdict: No new findings

The changes since the prior same-lane follow-up marker (ef77e9b) are the newest commit set on codex/nest-build-audit-hardening. I reviewed the newly introduced smoke-test worker/downloader validation, deploy verification state handling, and related wiring against the diff. Prior findings (lodash pin, revision-pinned deploy verification, x-powered-by, output guard, Express-adapter guard, Sonar rating) were resolved in earlier reviews and are not repeated here.

Notes (non-blocking)

  • scripts/smoke-staging-lambdas.cjs:36-42 — The worker/downloader probes still send { __allowlistLambdaSmokeTest: true } and expect response.ok === true. The worker/downloader handlers (src/worker-lambda.ts, src/token-pool-downloader-lambda.ts) were not included in this excerpt, so I could not confirm they short-circuit on this sentinel and return { ok: true } without side effects. This was flagged in the prior review and remains unverifiable here — worth confirming the handlers actually recognize the flag before relying on the smoke gate. Not a code finding in the included diff.

  • scripts/deploy-lambdas.cjs:188 — The post-deploy verification now accepts both Active and Inactive states, with the comment noting the staging smoke invocation reactivates functions. For production (which has no smoke step in production.yml), an Inactive state passes verification and is never exercised, so a broken-but-idle function would deploy "green." Low risk given codeSha256/handler/runtime are still asserted, but production has no post-deploy invocation to catch a cold-start regression. Non-blocking.

Note: Large-PR mode with partial/truncated context (yarn.lock and many relevant files truncated/omitted; worker/downloader handlers not in excerpt). This is not exhaustive; it prioritizes the newly introduced follow-up code and carry-over items.

Signed-off-by: Gelato Genesis <gelatogenesis@Gelatos-Mac-mini.local>
@6529bot

6529bot Bot commented Sep 9, 2026

Copy link
Copy Markdown

6529bot follow-up commit review - 720b1b6

Verdict: No new findings

The changes since the prior same-lane follow-up marker (acf02fd) are the newest commit set on codex/nest-build-audit-hardening. I reviewed the deploy verification state handling, smoke-test wiring, and audit/packaging scripts against the included diff and context. Prior findings (lodash pin, revision-pinned deploy verification, x-powered-by, output guard, Express-adapter guard, Sonar rating) were resolved in earlier reviews and are not repeated here.

Notes (non-blocking, carry-over)

  • scripts/deploy-lambdas.cjs:188 — Production deploy verification still accepts Inactive state, and production.yml has no post-deploy smoke step (staging's lambda:smoke:staging at staging.yml:52-53 does exercise cold-start). A broken-but-idle production function could deploy "green" since codeSha256/handler/runtime are still asserted but never invoked. Previously flagged; unchanged. Non-blocking.

  • scripts/smoke-staging-lambdas.cjs:36-42 — Worker/downloader smoke probes send { __allowlistLambdaSmokeTest: true } and require response.ok === true. The handler entrypoints (src/worker-lambda.ts, src/token-pool-downloader-lambda.ts) were not fully included in this excerpt, so I still cannot confirm they short-circuit on the sentinel and return { ok: true } without side effects. Worth confirming the handlers recognize the flag before relying on the gate. Previously flagged; unverifiable here.

Note: Large-PR mode with partial/truncated context (yarn.lock and many relevant files truncated/omitted; worker/downloader handler bodies not in excerpt). This is not exhaustive; it prioritizes newly introduced follow-up code and carry-over items.

Signed-off-by: Gelato Genesis <gelatogenesis@Gelatos-Mac-mini.local>
@6529bot

6529bot Bot commented Sep 9, 2026

Copy link
Copy Markdown

6529bot follow-up commit review - 9d6c24a

Verdict: No new findings

The changes since the prior same-lane follow-up marker (720b1b6) add the esbuild-based Lambda bundling step (scripts/bundle-lambdas.cjs, wired into yarn build) and its supporting verification/packaging updates. I reviewed the new bundler, the verify-lambda-package.cjs runtime-require change, and the smoke/deploy wiring against the included diff. Prior findings (lodash pin, revision-pinned deploy verification, x-powered-by, output guard, Express-adapter guard, Sonar rating) were resolved in earlier reviews and are not repeated here.

Notes (non-blocking)

  • scripts/verify-lambda-package.cjs:134-146 — The handler-load check now injects NODE_OPTIONS=--no-experimental-require-module and empties SENTRY_DSN. This verifies the bundled CJS handlers require() cleanly, but with --no-experimental-require-module it will fail if any bundled dependency ships as ESM-only under Node 24. That's the intended guard (catches ESM-in-CJS packaging regressions early), so this is a correctness win, not a finding — noted only because the bundler's runtimeDependencyBoundary externalizes all non-@nestjs bare imports (scripts/bundle-lambdas.cjs:31-34), so any ESM-only runtime dep would surface here rather than at cold start. Confirm the production dep graph has no ESM-only packages that this would trip on.

  • scripts/bundle-lambdas.cjs:22-30 — Only the enumerated bundledNestPackages set is inlined; all other @nestjs/* subpaths are marked external. If a bundled Nest package internally imports a non-listed @nestjs/* package (e.g. a transitive @nestjs/mapped-types path not in the set), it stays external and must exist in node_modules. verify-lambda-package.cjs requires @nestjs/core and @nestjs/platform-express prefixes but not every externalized Nest package, so a missing externalized dep would only surface in the require-check at line 124. Low risk since the require-check exercises all three handlers, but the required-prefix list is narrower than the externalization surface. Non-blocking.

  • scripts/smoke-staging-lambdas.cjs:36-42 — Carry-over: worker/downloader probes still send { __allowlistLambdaSmokeTest: true } and require response.ok === true. The handler entrypoints (src/worker-lambda.ts, src/token-pool-downloader-lambda.ts) are not in this excerpt, so I still cannot confirm they short-circuit on the sentinel without side effects. Previously flagged; worth confirming before relying on the gate.

  • scripts/deploy-lambdas.cjs:188 — Carry-over: production verification still accepts Inactive state and production.yml has no smoke step, so a broken-but-idle production function could deploy "green." Previously flagged; unchanged. Non-blocking.

Note: Large-PR mode with partial/truncated context (yarn.lock and many relevant files truncated/omitted; worker/downloader handler bodies not in excerpt). This is not exhaustive; it prioritizes the newly introduced bundler code and carry-over items.

Signed-off-by: Gelato Genesis <gelatogenesis@Gelatos-Mac-mini.local>
@6529bot

6529bot Bot commented Sep 10, 2026

Copy link
Copy Markdown

6529bot follow-up commit review - 5aac5c1

Verdict: No new findings

The changes since the prior same-lane marker (9d6c24a) add production smoke wiring (lambda:smoke:prod in production.yml:52-53, renamed scripts/smoke-lambdas.cjs handling both environments), esbuild to verify-lambda-package.cjs forbidden prefixes, and README/AGENTS documentation. I reviewed the renamed smoke script, deploy verification, and workflow wiring against the included diff. Prior findings (lodash pin, revision-pinned deploy verification, x-powered-by, output guard, Express-adapter guard, Sonar rating, esbuild-in-package guard) were resolved in earlier reviews and are not repeated here.

Resolved since last review

  • production.yml:52-53 — The prior carry-over concern (production had no post-deploy invocation to catch a cold-start regression while accepting an Inactive state) is now addressed: lambda:smoke:prod exercises all three functions after deploy, and scripts/smoke-lambdas.cjs:104-131 retries through ResourceNotReadyException by waiting for function-active-v2. The Inactive-state acceptance in deploy-lambdas.cjs:189 is now backed by an actual invocation in both environments.

Notes (non-blocking, carry-over)

  • scripts/smoke-lambdas.cjs:38-46 — The worker/downloader probes still send { __allowlistLambdaSmokeTest: true } and require response?.ok === true. The handler entrypoints (src/worker-lambda.ts, src/token-pool-downloader-lambda.ts) short-circuit was not confirmable in this excerpt (worker/downloader handler bodies not included). The README (README.md:105) describes a "reserved, side-effect-free bootstrap probe," but the worker-lambda.ts diff shown earlier just wraps bootstrap() and processes event — worth confirming these handlers actually recognize the sentinel and return { ok: true } without side effects before relying on the gate. Previously flagged; still unverifiable here.

Note: Large-PR mode with partial/truncated context (yarn.lock and many relevant files truncated/omitted; worker/downloader handler bodies not in excerpt). This is not exhaustive; it prioritizes the newly introduced smoke/deploy wiring and carry-over items.

@sonarqubecloud

Copy link
Copy Markdown

@GelatoGenesis
GelatoGenesis merged commit fc2d228 into main Sep 10, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant