Adds backend governance controls to reduce operational risk and make API behavior safer and more predictable. - #264
Conversation
|
@OtowoSamuel Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
There was a problem hiding this comment.
Pull request overview
This PR introduces backend governance controls aimed at reducing operational risk by enforcing API versioning, adding replay-safe mutation handling, and improving reliability/observability for background jobs and migrations.
Changes:
- Added
/api/v1API versioning with legacy/api/*redirects. - Implemented idempotency-key-based replay protection for deposit mutations.
- Added background job retry/dead-letter tracking plus a CI migration safety scanner and supporting tests/docs.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/src/rateLimiter.ts | Tightens express-rate-limit header behavior and centralizes API limiter configuration. |
| backend/src/listEndpoints.ts | Moves list routes to versioned mounting by changing paths to router-relative endpoints. |
| backend/src/jobGovernance.ts | Adds retry policy, dead-letter recording, and job health/metrics helpers. |
| backend/src/index.ts | Adds /api/v1 routing, legacy redirect middleware, idempotent deposit endpoint, and ops metrics endpoint. |
| backend/src/idempotency.ts | Implements in-memory idempotency store with fingerprinting and replay/conflict handling. |
| backend/src/tests/api.test.ts | Updates tests to use versioned routes and validates idempotent deposit behavior. |
| backend/src/tests/governance.test.ts | Adds coverage for redirects, idempotency replay/conflict, and job retry/dead-letter behavior. |
| backend/scripts/check-migrations.js | Adds migration safety scanning script for CI. |
| backend/package.json | Adds migration check + governance CI script. |
| backend/README.md | Documents versioning, idempotency, job governance, and migration scan behavior. |
| .github/workflows/backend-governance.yml | Adds CI workflow to run governance checks on backend changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| legacyHeaders: false, | ||
| keyGenerator: (req: Request) => { | ||
| // Use API key if provided, otherwise use IP | ||
| return req.headers['x-api-key'] as string || req.ip || 'unknown'; |
There was a problem hiding this comment.
keyGenerator casts req.headers['x-api-key'] to string, but Express headers can be string | string[]. Returning an array here can break express-rate-limit keying. Prefer req.get('x-api-key') (always string | undefined) or explicitly handle arrays before returning the key.
| return req.headers['x-api-key'] as string || req.ip || 'unknown'; | |
| return req.get('x-api-key') || req.ip || 'unknown'; |
| console.error('Migration safety check found risky patterns:'); | ||
| for (const finding of findings) { | ||
| console.error(`- [${finding.severity}] ${path.relative(repoRoot, finding.file)}: ${finding.message}`); | ||
| } | ||
| process.exit(1); |
There was a problem hiding this comment.
Findings marked as severity: 'warning' still cause the script to exit with code 1 because it fails on any finding. If warnings are intended to be advisory, consider only failing CI when at least one error is present, while still printing warnings to stderr.
| console.error('Migration safety check found risky patterns:'); | |
| for (const finding of findings) { | |
| console.error(`- [${finding.severity}] ${path.relative(repoRoot, finding.file)}: ${finding.message}`); | |
| } | |
| process.exit(1); | |
| const hasErrors = findings.some((finding) => finding.severity === 'error'); | |
| console.error('Migration safety check found risky patterns:'); | |
| for (const finding of findings) { | |
| console.error(`- [${finding.severity}] ${path.relative(repoRoot, finding.file)}: ${finding.message}`); | |
| } | |
| if (hasErrors) { | |
| process.exit(1); | |
| } |
| | `RATE_LIMIT_WINDOW_MS` | `900000` | Global rate limit window | | ||
| | `RATE_LIMIT_MAX_REQUESTS` | `100` | Global requests per window | |
There was a problem hiding this comment.
The README documents RATE_LIMIT_WINDOW_MS / RATE_LIMIT_MAX_REQUESTS for a global rate limiter, but the backend no longer configures a global limiter (only the /api/v1 limiter remains). Update the configuration table to reflect the current behavior (or reintroduce the global limiter if it’s still required).
| | `RATE_LIMIT_WINDOW_MS` | `900000` | Global rate limit window | | |
| | `RATE_LIMIT_MAX_REQUESTS` | `100` | Global requests per window | |
| if (req.path.startsWith('/v1')) { | ||
| next(); | ||
| return; | ||
| } | ||
|
|
||
| const redirectedPath = req.originalUrl.replace(/^\/api(?!\/v1)/, '/api/v1'); |
There was a problem hiding this comment.
The /api redirect middleware treats any path that starts with /v1 as already-versioned (so /api/v12/... will bypass redirect), and it also redirects /api/v2/... to /api/v1/v2/..., which is misleading. Consider matching /v1 on a path-segment boundary (e.g. /^\/v1(\/|$)/) and returning a 404/400 for other /api/v\d+ versions instead of rewriting them into v1.
| if (req.path.startsWith('/v1')) { | |
| next(); | |
| return; | |
| } | |
| const redirectedPath = req.originalUrl.replace(/^\/api(?!\/v1)/, '/api/v1'); | |
| if (/^\/v1(\/|$)/.test(req.path)) { | |
| next(); | |
| return; | |
| } | |
| if (/^\/v\d+(\/|$)/.test(req.path)) { | |
| res.status(404).json({ error: 'API version not supported' }); | |
| return; | |
| } | |
| const redirectedPath = req.originalUrl.replace(/^\/api/, '/api/v1'); |
| app.use('/api', (req: Request, res: Response, next: NextFunction) => { | ||
| if (req.path.startsWith('/v1')) { | ||
| next(); | ||
| return; | ||
| } | ||
|
|
||
| const redirectedPath = req.originalUrl.replace(/^\/api(?!\/v1)/, '/api/v1'); | ||
| res.setHeader('Deprecation', 'true'); | ||
| res.setHeader( | ||
| 'Sunset', | ||
| new Date(Date.now() + 90 * 24 * 60 * 60 * 1000).toUTCString() | ||
| ); | ||
| res.setHeader('Link', `<${redirectedPath}>; rel="alternate"`); | ||
| res.redirect(308, redirectedPath); | ||
| }); |
There was a problem hiding this comment.
PR description says backend tests were updated for versioned routes, but the repository still contains pagination tests hitting /api/* directly. With this redirect in place, those tests will now see 308 responses unless they follow redirects or are updated to call /api/v1/*.
| app.get('/api/v1/ops/job-metrics', (_req: Request, res: Response) => { | ||
| res.json({ | ||
| timestamp: new Date().toISOString(), | ||
| ...getJobMetrics(), | ||
| }); | ||
| }); |
There was a problem hiding this comment.
/api/v1/ops/job-metrics exposes dead-letter contents (including payload) and error strings to any caller. This can leak sensitive data and operational details; it should be authenticated/authorized, restricted to non-production, and/or redact payload/error details in the public response.
| this.pendingResponses.set(key, { | ||
| fingerprint, | ||
| promise: operationPromise, | ||
| }); | ||
|
|
||
| try { | ||
| const stored = await operationPromise; | ||
| return { | ||
| result: { | ||
| statusCode: stored.statusCode, | ||
| body: stored.body, | ||
| }, | ||
| replayed: false, | ||
| }; | ||
| } finally { | ||
| this.pendingResponses.delete(key); | ||
| } |
There was a problem hiding this comment.
pendingResponses entries are only removed when operationPromise resolves/rejects. If an operation hangs indefinitely, the entry will remain forever and block future requests for that key (and leak memory). Consider adding a timeout/abort mechanism around operation() and evicting the pending entry if it exceeds a max duration.
| res.setHeader('Deprecation', 'true'); | ||
| res.setHeader( | ||
| 'Sunset', | ||
| new Date(Date.now() + 90 * 24 * 60 * 60 * 1000).toUTCString() | ||
| ); | ||
| res.setHeader('Link', `<${redirectedPath}>; rel="alternate"`); |
There was a problem hiding this comment.
Deprecation response header is defined as an HTTP-date (RFC 8594); setting it to the string true is non-standard and may confuse clients/proxies. Consider using an HTTP-date value (similar to Sunset) or omitting the header and using a documented custom header instead.
| class JobGovernanceStore { | ||
| private readonly deadLetters: DeadLetterRecord[] = []; | ||
|
|
||
| private readonly failureCounts = new Map<JobName, number>(); | ||
|
|
||
| recordDeadLetter(record: DeadLetterRecord): void { | ||
| this.deadLetters.unshift(record); | ||
| const failures = (this.failureCounts.get(record.jobName) || 0) + 1; | ||
| this.failureCounts.set(record.jobName, failures); | ||
|
|
||
| if (failures >= JOB_POLICIES[record.jobName].deadLetterThreshold) { | ||
| console.warn(`Recurring failures detected for ${record.jobName}: ${failures}`); | ||
| } | ||
| } | ||
|
|
||
| clear(): void { | ||
| this.deadLetters.length = 0; | ||
| this.failureCounts.clear(); | ||
| } | ||
|
|
||
| getMetrics() { | ||
| const recurringFailures = Object.fromEntries( | ||
| Array.from(this.failureCounts.entries()).filter( | ||
| ([jobName, failures]) => failures >= JOB_POLICIES[jobName].deadLetterThreshold | ||
| ) | ||
| ) as Partial<Record<JobName, number>>; | ||
|
|
||
| return { | ||
| totalDeadLetters: this.deadLetters.length, | ||
| failureCounts: Object.fromEntries(this.failureCounts), | ||
| recurringFailures, | ||
| deadLetters: [...this.deadLetters], | ||
| policies: JOB_POLICIES, | ||
| }; |
There was a problem hiding this comment.
Dead-letter records are stored in an unbounded in-memory array and returned wholesale via metrics. In a prolonged failure scenario this can grow without limit and increase memory pressure; consider adding a max retention cap (e.g., keep last N) and/or a TTL-based eviction strategy, and avoid storing large payloads by default.
| payload: options.payload ?? null, | ||
| failedAt: new Date().toISOString(), | ||
| }); | ||
|
|
There was a problem hiding this comment.
After retries are exhausted, runJobWithRetry throws a new Error(normalizedError), which drops the original error type/stack. Consider rethrowing the last error when it is an Error, or throwing a new error that preserves it via an error wrapper / cause option for easier debugging.
| if (lastError instanceof Error) { | |
| throw lastError; | |
| } |
Closes #193
Closes #194
Closes #195
Closes #196
What Changed
/api/v1with legacy/api/*redirects.Validation
npm run buildnpm run check:migrationsnpx jest --runInBand src/__tests__/api.test.ts src/__tests__/governance.test.tsNotes