fix(gateway): stabilize Fastify runtime, config validation, and CI#21
Open
donny-devops wants to merge 13 commits into
Open
fix(gateway): stabilize Fastify runtime, config validation, and CI#21donny-devops wants to merge 13 commits into
donny-devops wants to merge 13 commits into
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Analysis CompleteGenerated ECC bundle from 11 commits | Confidence: 55% View Pull Request #22Repository Profile
Changed Files (11)
Top hotspots
Top directories
Analysis Depth Readiness (evidence-backed, 29%)ECC Tools uses this to decide whether recommendations should stay at commit-history/setup guidance or expand into CI, security, harness, reference-set, AI-routing, and team backlog work.
Reference Set Readiness (0/7, 0%)
Likely Future Issues (5)
Suggested Follow-up Work (5)
Copy-ready bodies chore: sync config templates for config/gateway.ts + eslint.config.js ## Summary
- Update the example env files, sample configs, or deployment templates that should mirror the changed runtime configuration surface.
## Why
- Backfill example env files or config templates before a fresh setup drifts from the shipped runtime surface.
## Touched paths
- `config/gateway.ts`
- `eslint.config.js`
## Validation
- Update the repo example env file or config template that should reflect the new runtime settings.
- Run the setup, boot, or deployment validation flow that depends on the changed config surface.test: add auth coverage for src/middleware/auth.ts + src/middleware/ddos.ts ## Summary
- Add auth, session, or permission regression coverage for the recently changed security-sensitive surface.
## Why
- Backfill auth or permission regression coverage before another access-control change lands on the touched surface.
## Touched paths
- `src/middleware/auth.ts`
- `src/middleware/ddos.ts`
## Validation
- Add or extend integration / e2e coverage for the changed auth, session, middleware, or permission surface.
- Exercise allowed and denied flows, invalid or expired credentials, or equivalent access-control boundary cases.security: add scanner evidence for src/middleware/auth.ts + src/middleware/ddos.ts ## Summary
- Add security scanner or code-scanning evidence for the recently changed security-sensitive surface.
## Why
- Backfill explicit scanner or code-scanning evidence before another security-sensitive change lands on the touched surface.
## Touched paths
- `src/middleware/auth.ts`
- `src/middleware/ddos.ts`
## Validation
- Run or add the relevant security scanner, code scanning, secret scanning, or dependency/security review check for the touched surface.
- Attach the scanner output, SARIF/code-scanning result, or focused security regression test to the follow-up PR.
- Confirm the changed auth, billing, webhook, secret-handling, agent, or CI surface has an explicit pass/fail gate.ci: add failure-mode evidence for .github/workflows/ci.yml + vitest.config.ts ## Summary
- Add CI failure-mode evidence for the recently changed workflow or test-runner surface.
## Why
- Backfill CI failure-mode evidence before another workflow or test-runner change lands on the touched surface.
## Touched paths
- `.github/workflows/ci.yml`
- `vitest.config.ts`
## Validation
- Add or update a CI failure fixture, captured failing log, troubleshooting note, workflow dry-run evidence, or regression test for the changed CI/test-runner behavior.
- Run the affected workflow or test-runner entrypoint locally or in CI and record pass/fail evidence.chore: refresh lockfile and validate CI after dependency updates ## Summary
- Refresh the lockfile and rerun CI after the dependency or workflow changes in this PR.
## Why
- Package or workflow changes without a lockfile refresh tend to turn into noisy follow-up fixes after merge.
## Touched paths
- `.github/workflows/ci.yml`
- `package.json`
## Validation
- Refresh the lockfile in the same package manager used by the repo.
- Run the repo typecheck / test / CI entrypoints that depend on the updated package graph.Detected Workflows (2)
Generated Instincts (21)
After merging, import with: Files
|
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
This PR improves the codebase with better TypeScript configuration, CI enforcement, and config validation using Zod. However, 3 critical issues block merge:
Critical Issues Requiring Fix
- Security: Hardcoded JWT secret fallback allows token forgery in production
- Security: Unvalidated JSON.parse on environment variable creates injection vector
- Compatibility: Fastify 4.x/5.x version mismatch will cause plugin registration failures
What Works Well
The PR successfully:
- Adds missing
cryptoimport fixing randomUUID compilation error - Implements Zod-based upstream validation with proper error messages
- Makes Redis optional during startup preventing crash on connection failure
- Adds comprehensive CI pipeline with typecheck, lint, test, and build steps
- Exports
isPublicPathfor unit testing - Adds auth failure metrics for observability
Please address the critical issues before merge.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
Comment on lines
+17
to
+28
| "@fastify/cors": "^9.0.1", | ||
| "@fastify/formbody": "^7.4.1", | ||
| "@fastify/helmet": "^11.1.1", | ||
| "@fastify/rate-limit": "^9.1.0", | ||
| "@fastify/http-proxy": "^9.5.0", | ||
| "@fastify/cors": "^9.0.1", | ||
| "@fastify/formbody": "^7.4.0", | ||
| "@fastify/jwt": "^8.0.1", | ||
| "@fastify/rate-limit": "^9.1.0", | ||
| "@opentelemetry/auto-instrumentations-node": "^0.48.0", | ||
| "@opentelemetry/exporter-otlp-grpc": "^0.52.1", | ||
| "@opentelemetry/exporter-prometheus": "^0.52.1", | ||
| "@opentelemetry/sdk-node": "^0.52.1", | ||
| "dotenv": "^16.4.5", | ||
| "fastify": "^4.29.1", |
There was a problem hiding this comment.
🛑 Crash Risk: Fastify 5.x downgrade to 4.x breaks compatibility with existing plugins. The diff shows Fastify was downgraded from ^5.8.4 to ^4.29.1, but all @fastify/* plugin versions remain at their latest which expect Fastify 5.x. This version mismatch will cause runtime failures during plugin registration.
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Security | 3 minor 18 high 6 critical 16 medium |
🟢 Metrics 27 complexity · 0 duplication
Metric Results Complexity 27 Duplication 0
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request modernizes the API gateway by migrating to ES modules, introducing Zod for robust configuration validation, and improving the middleware stack. Key enhancements include refactoring configuration parsing, adding wildcard support for public paths in the authentication middleware, and streamlining DDoS protection. Feedback from the review suggests further hardening the configuration parsing for route-specific rate limits, eliminating redundant request ID generation by utilizing Fastify's built-in IDs, and expanding test coverage to include the new wildcard matching logic.
There was a problem hiding this comment.
Pull request overview
This PR hardens the TypeScript Fastify API gateway by restoring Fastify 4 compatibility, tightening runtime/config behavior, and adding CI/testing/linting coverage.
Changes:
- Updates Fastify runtime dependencies, build output paths, and NodeNext TypeScript configuration.
- Adds config parsing validation, Redis-optional startup behavior, auth metrics, and public-path auth tests.
- Introduces ESLint flat config, Vitest coverage config, and GitHub Actions CI.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
vitest.config.ts |
Adds Vitest node environment and coverage configuration. |
tsconfig.json |
Expands TypeScript project scope and adjusts root/output behavior. |
tests/auth.test.ts |
Adds unit tests for public auth path matching. |
src/server.ts |
Updates Fastify setup, Redis initialization, middleware registration, and shutdown handling. |
src/plugins/requestContext.ts |
Cleans request context formatting/comments. |
src/middleware/ddos.ts |
Removes wildcard parser and updates DDoS fallback/formatting behavior. |
src/middleware/auth.ts |
Adds stricter public-path matching and auth failure metrics. |
package.json |
Pins Fastify 4-compatible dependency set and updates scripts/build entrypoints. |
eslint.config.js |
Adds ESLint flat config for TypeScript linting. |
config/gateway.ts |
Adds Zod-backed upstream parsing and centralized env parsing helpers. |
.github/workflows/ci.yml |
Adds CI quality gate workflow for typecheck, lint, tests, and build. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "build": "tsc", | ||
| "start": "node dist/server.js", | ||
| "lint": "eslint src --max-warnings 0", | ||
| "start": "node dist/src/server.js", |
Comment on lines
+9
to
+13
| languageOptions: { | ||
| parserOptions: { | ||
| project: './tsconfig.json', | ||
| }, | ||
| }, |
Comment on lines
+17
to
+20
| const parsed = Number.parseInt(raw, 10); | ||
| if (!Number.isFinite(parsed)) { | ||
| throw new Error(`${name} must be an integer`); | ||
| } |
Comment on lines
55
to
58
| await redis.connect().catch((err) => { | ||
| console.warn('[redis] failed to connect on start — continuing without Redis:', err.message); | ||
| console.warn('[redis] failed to connect on start; continuing without Redis:', err.message); | ||
| redis = null; | ||
| }); |
| port: parseInt(process.env.REDIS_PORT ?? '6379', 10), | ||
| password: process.env.REDIS_PASSWORD ?? undefined, | ||
| db: parseInt(process.env.REDIS_DB ?? '0', 10), | ||
| host: process.env.REDIS_HOST || null, |
| const path = normalisePath(url); | ||
| return config.jwt.publicPaths.some((publicPath) => { | ||
| if (publicPath.endsWith('*')) { | ||
| return path.startsWith(publicPath.slice(0, -1)); |
Co-authored-by: amazon-q-developer[bot] <208079219+amazon-q-developer[bot]@users.noreply.github.com>
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
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.
Summary
Hardens the Node TypeScript API gateway runtime after Fastify dependency drift and missing CI enforcement caused compile/runtime instability.
Fixes
Quality Gates
Operational Impact
This PR converts the repo from a partially broken proof-of-concept into a reproducible gateway build with enforceable quality controls and safer runtime defaults.