feat(api-keys): add structured metrics and logging

[Ukorstack]

Closes Liquifact#766

Summary

• Added api_key_auth_duration_seconds histogram with bounded labels (endpoint, method, status, outcome)
• Added api_key_auth_errors_total counter with bounded cause labels (unauthorized, forbidden, validation_error, not_found, internal_error)
• Added structured request logging via res.on('finish') with duration_ms, status, outcome, and error_type
• Exposed metrics through existing /metrics Prometheus endpoint
• Added 14 comprehensive tests covering success, client error, and server error paths
• Updated README.md with API Key Auth Metrics documentation and PromQL examples

Security Notes

• No API keys logged or exposed in metric labels
• No Authorization headers exposed
• No PII logged
• Bounded metric label cardinality
• Existing authentication behavior preserved

Validation

• npm run lint ✅ (no new warnings)
• npm test ✅ (77/77 passing)
• npm run build ⚠️ (pre-existing knexfile.ts overwrite — unrelated)

Summary by CodeRabbit

• New Features

  • Added API key authentication metrics for request duration and error counts.
  • Added structured authentication logs for successful and failed requests, without exposing API keys or secrets.
  • Added documentation describing available metrics, labels, error causes, and log formats.

• Tests

  • Added coverage for authentication metrics and structured logging across success, client error, and internal error scenarios.

[coderabbitai]

📝 Walkthrough

Walkthrough

API key authentication now records bounded Prometheus duration and error metrics, emits structured success and failure logs, and documents and tests these observability behaviors without exposing API key material.

Changes

API key authentication observability

Layer / File(s)	Summary
Authentication metrics and classifications src/metrics.js	Adds bounded outcome and error-cause enums, status classification helpers, duration and error metrics, and exports them.
Middleware metrics and structured logging src/middleware/apiKeyAuth.js	Records final response duration and classifications, increments error counters, and emits structured success or failure logs.
Observability tests and documentation tests/unit/apiKeyAuth.test.js, README.md	Validates metric labels, counters, internal errors, structured logs, and secret exclusion; documents metrics and log formats.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested reviewers: sheyman546, adeyemi-cmd

Sequence Diagram(s)

sequenceDiagram
  participant authenticateApiKey
  participant Response
  participant Metrics
  participant Logger
  authenticateApiKey->>Response: register finish handler
  Response->>authenticateApiKey: emit final status
  authenticateApiKey->>Metrics: record duration and classified outcome
  authenticateApiKey->>Metrics: increment error cause for failures
  authenticateApiKey->>Logger: emit structured success or failure log

Loading

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feature/api-keys-13-observability

---

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

[coderabbitai]

🧹 Nitpick comments (1)

tests/unit/apiKeyAuth.test.js (1)

391-448: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff

Tests rely on prom-client's internal hashMap storage.

apiKeyAuthDurationSeconds.hashMap and entry .labels are undocumented internals; prom-client's own changelog notes this storage is being renamed/restructured to LabelMap in an upcoming breaking release. Works fine on the pinned 14.2.0, but will silently break on a future major upgrade.

🤖 Prompt for 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.

In `@tests/unit/apiKeyAuth.test.js` around lines 391 - 448, Replace the metrics
assertions in the duration histogram tests with checks through prom-client’s
supported public API rather than apiKeyAuthDurationSeconds.hashMap or
entry.labels. Update the success, 401, 403, and API-key exclusion cases to
inspect collected metric samples and their public label values while preserving
the existing expected outcomes.

🤖 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.

Nitpick comments:
In `@tests/unit/apiKeyAuth.test.js`:
- Around line 391-448: Replace the metrics assertions in the duration histogram
tests with checks through prom-client’s supported public API rather than
apiKeyAuthDurationSeconds.hashMap or entry.labels. Update the success, 401, 403,
and API-key exclusion cases to inspect collected metric samples and their public
label values while preserving the existing expected outcomes.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 1d59b5c5-ec0b-4810-9aad-b29ba208e0df

📥 Commits

Reviewing files that changed from the base of the PR and between ab5ff3d and 8d0637c.

📒 Files selected for processing (4)

• README.md
• src/metrics.js
• src/middleware/apiKeyAuth.js
• tests/unit/apiKeyAuth.test.js

[coderabbitai]

Actionable comments posted: 5

Note

Due to the large number of review comments, Critical severity comments were prioritized as inline comments.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)

tests/mocks/setup.js (1)

18-33: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Clear invoiceFiles in the mock reset path. invoiceFiles is module-scoped like investorLocks, but the reset helper only trims investorLocks, so invoice_files rows can leak between tests. filterInvoiceFiles is also a copy of filterInvestorLocks; a shared makeFilter(rows) helper would keep them in sync.

🤖 Prompt for 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.

In `@tests/mocks/setup.js` around lines 18 - 33, Update the mock reset helper to
clear the module-scoped invoiceFiles collection alongside investorLocks,
preventing invoice_files data from leaking between tests. Refactor
filterInvestorLocks and filterInvoiceFiles to use a shared makeFilter(rows)
helper while preserving their existing queryWheres filtering behavior.

src/routes/invoiceFile.js (1)

63-83: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Presigned-upload route missed the fixes applied to the sibling routes in this same file.

Two inconsistencies vs. the four routes updated by this PR in the same file:

• Line 73 derives tenantId as req.user?.id || req.user?.sub || 'unknown', omitting req.user?.tenantId, while POST /:id/file, GET /:id/file, GET /:id/file/verify, and POST /:id/file/verify now all prefer req.user?.tenantId first. If a caller's req.user.tenantId differs from .id/.sub, a file uploaded via this presigned-upload flow can end up scoped to a different tenant than lookups via the other routes expect.
• Line 77 checks error.message against a list of short error codes ('INVALID_MIME_TYPE', 'FILE_TOO_LARGE', ...), but storage.js sets those strings on err.code, not err.message (which is a full sentence). This check can never match, so all storage validation errors here fall through to the generic 500 instead of the intended 400 — the sibling POST /:id/file handler in this file correctly checks err.code (line 120).

🐛 Suggested fix

-    const tenantId = req.user?.id || req.user?.sub || 'unknown';
+    const tenantId = req.user?.tenantId || req.user?.id || req.user?.sub || 'unknown';
     const result = await storageService.getPresignedUploadUrl({ tenantId, invoiceId: id, fileName, mimeType, fileSize });
     return res.status(201).json({ data: { invoiceId: id, uploadUrl: result.url, fileKey: result.key }, message: 'Presigned upload URL generated' });
   } catch (error) {
-    if (['INVALID_MIME_TYPE','FILE_TOO_LARGE','INVALID_FILENAME','INVALID_TENANT_ID','INVALID_INVOICE_ID','INVALID_EXPIRY'].includes(error.message)) {
+    if (['INVALID_MIME_TYPE','FILE_TOO_LARGE','INVALID_FILENAME','INVALID_TENANT_ID','INVALID_INVOICE_ID','INVALID_EXPIRY'].includes(error.code)) {
       return res.status(400).json({ error: 'Bad Request', message: error.message });
     }

🤖 Prompt for 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.

In `@src/routes/invoiceFile.js` around lines 63 - 83, Update the presigned-upload
handler to derive tenantId with req.user?.tenantId before the existing id/sub
fallbacks, matching the sibling file routes. In its catch block, use error.code
rather than error.message when matching the storage validation error codes,
while preserving the existing 400 responses and generic 500 handling.

src/app.js (1)

95-113: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win

Keep the status-aware branch limited to sanitized errors src/app.js:95 — this now returns err.detail || err.title || err.message for any err.status in 400–599, so a non-AppError 5xx with a status tag can leak its raw message instead of reaching the redacted 500 fallback. Restrict this path to known-safe errors or use a fixed message for generic 5xxs.

🤖 Prompt for 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.

In `@src/app.js` around lines 95 - 113, Restrict the status-aware branch in
handleInternalError to recognized sanitized AppError instances before returning
err.detail, err.title, or err.message. Ensure generic errors with 5xx status
tags bypass this response and reach the redacted 500 fallback, or return a fixed
safe message for those cases.

🟠 Major comments (26)

src/errors/mapError.js-101-117 (1)

101-117: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Serialize the normalized generic-error extensions.

mapError() now computes code, retryable, and retryHint for generic errors, but problemJsonHandler only forwards raw AppError extensions. Consequently, generic 429/503 responses drop the documented retry contract, and generic 500 responses drop INTERNAL_SERVER_ERROR.

• src/errors/mapError.js#L101-L117: update the error-to-response path to pass mappedError.code, mappedError.retryable, and mappedError.retryHint into formatProblemDetails.
• tests/problems.test.js#L571-L575: update generic-response expectations and add generic 429/503 serialization coverage.
• docs/RFC7807-Error-Handling.md#L95-L124: align the table’s universal extension and retry-hint claims with the implemented response contract.

🤖 Prompt for 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.

In `@src/errors/mapError.js` around lines 101 - 117, Update the generic error
response path in mapError() to pass mappedError.code, mappedError.retryable, and
mappedError.retryHint to formatProblemDetails. In tests/problems.test.js, update
generic-response expectations and add serialization coverage for generic 429 and
503 errors. In docs/RFC7807-Error-Handling.md, revise the extension and
retry-hint table claims to match the implemented universal response contract.

src/middleware/apiKeyAuth.js-139-142 (1)

139-142: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Remove request IPs from API-key audit logs.

req.ip is PII and directly contradicts the module’s and PR’s no-PII logging guarantee.

• src/middleware/apiKeyAuth.js#L139-L142: omit ip: req.ip from missing-header logs.
• src/middleware/apiKeyAuth.js#L154-L188: omit ip: req.ip from invalid, revoked, and scope-rejection logs.
• src/middleware/apiKeyAuth.js#L201-L213: omit ip: req.ip from successful-auth logs.
• src/middleware/apiKeyAuth.js#L11-L13: retain this guarantee only after removing the IP fields.

🤖 Prompt for 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.

In `@src/middleware/apiKeyAuth.js` around lines 139 - 142, Remove the req.ip field
from all API-key audit log objects in src/middleware/apiKeyAuth.js: the
missing-header logger at lines 139-142, invalid/revoked/scope-rejection logs at
lines 154-188, and successful-auth logs at lines 201-213. Update the no-PII
guarantee at lines 11-13 only as needed so it remains accurate after these
removals.

src/middleware/apiKeyAuth.js-103-112 (1)

103-112: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win

Do not use req.path as a Prometheus label.

req.path includes concrete route parameters, so identifiers create an unbounded time-series cardinality. Use a fixed middleware label or a bounded route identifier supplied by the caller.

🤖 Prompt for 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.

In `@src/middleware/apiKeyAuth.js` around lines 103 - 112, Replace the req.path
label in the apiKeyAuthDurationSeconds.observe call with a fixed middleware
label or bounded route identifier supplied by the caller, while preserving the
existing method, status, outcome, and duration values.

src/middleware/rateLimit.js-246-252 (1)

246-252: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Don't use the raw API key as the rate-limit bucket key.

adminConfigKeyGenerator returns apikey_<plaintext-key>. With resolveRateLimitStore('config') active, that value becomes a live Redis key (rate-limit:config:apikey_<secret>), so admin API keys land in Redis keyspace, KEYS/MONITOR output, and any dashboard that lists bucket keys — the JSDoc explicitly cites dashboard legibility as a goal. Hash the key instead; bucket isolation is preserved.

🔒 Proposed fix

+const crypto = require('node:crypto');
+
 function adminConfigKeyGenerator(req) {
   const apiKey = getApiKey(req);
   if (apiKey) {
-    return `apikey_${apiKey}`;
+    // Hash so the plaintext credential never becomes a Redis key / log field.
+    return `apikey_${crypto.createHash('sha256').update(apiKey).digest('hex').slice(0, 32)}`;
   }
   return req.ip || req.socket?.remoteAddress || '127.0.0.1';
 }

Note this also changes the expectation in tests/unit/adminConfig.rateLimit.test.js (Lines 242-278), which asserts the plaintext prefix.

🤖 Prompt for 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.

In `@src/middleware/rateLimit.js` around lines 246 - 252, Update
adminConfigKeyGenerator to avoid embedding the plaintext API key in rate-limit
bucket keys: hash apiKey with the project’s existing cryptographic utility and
return a stable hashed key with an appropriate non-secret prefix, preserving
per-key bucket isolation. Update the affected adminConfig rate-limit unit test
expectations to assert the hashed format rather than the plaintext key.

src/schemas/config.js-39-90 (1)

39-90: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Replace legacy Zod 3 error options with error callbacks
invalid_type_error, required_error, and errorMap are ignored in Zod 4.3.6, so these schemas fall back to generic messages on type/required failures. Update the shared helpers and the inline schema definitions (including runtimeConfigSchema.section and config) to use error instead.

🤖 Prompt for 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.

In `@src/schemas/config.js` around lines 39 - 90, Replace the Zod 3
invalid_type_error, required_error, and errorMap options throughout the shared
helpers and inline schemas with Zod 4 error callbacks, preserving each field’s
existing custom messages for type, required, and validation failures. Update
_boundedString, boundedUrl, boundedInt, boundedNumber,
runtimeConfigSchema.section, config, and all other inline definitions in the
file without changing their validation rules.

tsconfig.build.json-5-5 (1)

5-5: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Keep rootDir on src unless the dist entrypoint moves too. With include limited to src/**, this emits dist/src/index.js, but start:dist still runs node dist/index.js. Update the dist consumer or revert this change.

🤖 Prompt for 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.

In `@tsconfig.build.json` at line 5, Revert the rootDir change in
tsconfig.build.json to src so the build continues emitting the entrypoint at
dist/index.js, matching the existing start:dist command; only retain a
repository-root rootDir if the dist consumer is updated to run the resulting
dist/src/index.js path.

tests/maturityReminders.test.js-26-32 (1)

26-32: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Inaccurate normalizeReminderReason mock is also the subject of its own test block. Both sites stem from one root cause: the inline mock reimplements the helper with wrong precedence and no err.code handling, and section 14 then asserts against that reimplementation rather than src/metrics.js.

• tests/maturityReminders.test.js#L26-L32: align the mock with src/metrics.js — check template before reject|smtp|recipient, and build the match string from err.code plus err.message.
• tests/maturityReminders.test.js#L862-L892: remove this describe block; it exercises the mock, and the real helper is already covered by tests/metrics.test.js lines 363-387.

🤖 Prompt for 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.

In `@tests/maturityReminders.test.js` around lines 26 - 32, Align the inline
normalizeReminderReason mock in tests/maturityReminders.test.js:26-32 with
src/metrics.js by matching template errors before reject|smtp|recipient cases
and building the match input from err.code plus err.message. Remove the describe
block in tests/maturityReminders.test.js:862-892 because it only tests the mock;
the real helper is covered elsewhere.

src/schemas/indexerEvent.js-16-33 (1)

16-33: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Trim before length validation, not after.

.transform(v => v.trim()) runs after .min/.max, so eventId: ' ' passes min(1) and normalizes to '', and a 257-char value with trailing whitespace is rejected despite trimming to 256. normalizeEvent then persists an empty eventId/eventType. Prefer z.string().trim() up front (and invoiceId's regex will then apply to the trimmed value).

🐛 Proposed fix

   eventId: z
     .string({ invalid_type_error: 'eventId must be a string' })
+    .trim()
     .min(1, { message: 'eventId is required' })
-    .max(256, { message: 'eventId must not exceed 256 characters' })
-    .transform((v) => v.trim()),
+    .max(256, { message: 'eventId must not exceed 256 characters' }),
 
   invoiceId: z
     .string({ invalid_type_error: 'invoiceId must be a string' })
+    .trim()
     .regex(INVOICE_ID_REGEX, {
       message: 'invoiceId must be 1-128 alphanumeric/underscore/hyphen characters',
-    })
-    .transform((v) => v.trim()),
+    }),
 
   eventType: z
     .string({ invalid_type_error: 'eventType must be a string' })
+    .trim()
     .min(1, { message: 'eventType is required' })
-    .max(128, { message: 'eventType must not exceed 128 characters' })
-    .transform((v) => v.trim()),
+    .max(128, { message: 'eventType must not exceed 128 characters' }),

🤖 Prompt for 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.

In `@src/schemas/indexerEvent.js` around lines 16 - 33, Update the eventId,
invoiceId, and eventType schemas in the indexer event definition to trim input
before applying length or regex validation. Replace the trailing transform-based
normalization with Zod’s trim preprocessing, preserving the existing validation
messages and limits so whitespace-only values fail and trimmed values are
validated.

src/schemas/indexerEvent.js-9-13 (1)

9-13: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Restore checksum validation for contractId normalizeEvent() uses this schema directly, so a regex-shaped but checksum-invalid Stellar contract address will still be accepted and written through. Reuse StrKey.isValidContract() here (or the existing isValidStellarContractId helper) instead of the regex alone.

🤖 Prompt for 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.

In `@src/schemas/indexerEvent.js` around lines 9 - 13, Update contractIdSchema to
validate contract addresses with StrKey.isValidContract() or the existing
isValidStellarContractId helper, rather than relying solely on
CONTRACT_ID_REGEX. Preserve the existing validation message and ensure
normalizeEvent() rejects regex-matching addresses with invalid checksums.

src/services/kycService.js-649-706 (1)

649-706: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Non-object JSON bodies crash with a TypeError.

JSON.parse('null') (or a bare number/string) parses successfully, so line 704 dereferences data.recordId on null and throws an untyped TypeError outside the hardened error contract. Validate the parsed shape before use.

🐛 Proposed fix

       let data;
       try {
         data = JSON.parse(responseText);
       } catch (parseErr) {
         throw new KycProviderError(
           `KYC provider returned non-JSON response: ${parseErr.message}`,
           { status: 502, retryable: false, code: 'invalid_response_body' },
         );
       }
 
+      if (!data || typeof data !== 'object' || Array.isArray(data)) {
+        throw new KycProviderError(
+          'KYC provider returned an unexpected response shape',
+          { status: 502, retryable: false, code: 'invalid_response_body' },
+        );
+      }
+
       return data;

🤖 Prompt for 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.

In `@src/services/kycService.js` around lines 649 - 706, Validate the parsed
result in the KYC provider operation before dereferencing it in the recordId,
verifiedAt, and status assignments. Accept only non-null object JSON bodies
(excluding arrays); otherwise throw a non-retryable KycProviderError with the
existing invalid-response contract, so primitive or null responses do not
produce an untyped TypeError.

src/services/kycService.js-620-647 (1)

620-647: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

verifySignature can throw RangeError on a malformed provider signature, escaping the typed-error contract.

verifySignature ends with crypto.timingSafeEqual(Buffer.from(signature, 'hex'), Buffer.from(expected, 'hex')) (src/services/webhooks.js:314-318). A provider (or MITM) supplying a short, odd-length, or non-hex v1= value yields a buffer of different length and timingSafeEqual throws RangeError instead of returning {valid:false}. That raw error propagates out of operation, is classified as non-retryable, and is re-thrown as a plain Error — not the documented KycProviderError{code:'invalid_response_signature'}. Fail-closed still holds via getKycStatus, but direct callers of verifyWithExternalProvider lose the typed contract.

🛡️ Proposed fix

         if (responseSig) {
-          const verification = verifySignature(config.apiSecret, responseText, responseSig);
+          let verification;
+          try {
+            verification = verifySignature(config.apiSecret, responseText, responseSig);
+          } catch (sigErr) {
+            verification = { valid: false, error: 'Malformed signature header' };
+          }
           if (!verification.valid) {

🤖 Prompt for 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.

In `@src/services/kycService.js` around lines 620 - 647, Update the
response-signature verification flow in verifyWithExternalProvider so malformed
signatures cannot let verifySignature’s RangeError escape. Catch or pre-validate
failures around verifySignature, treat them as invalid verification, and
preserve the existing KycProviderError with code invalid_response_signature for
all invalid signatures.

src/services/storage.js-69-89 (1)

69-89: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

MAX_FILE_SIZE duplicates and diverges from the validated INVOICE_FILE_MAX_SIZE config.

storage.js reads process.env.INVOICE_FILE_MAX_SIZE directly and parses it with its own parseSize(), instead of using getInvoiceFileMaxSize() from src/config/index.js. The two implementations disagree on strictness: InvoiceFileMaxSizeSchema (config) rejects invalid values by throwing, while parseSize() here silently falls back to DEFAULT_MAX_FILE_SIZE on a non-match. This means the size limit enforced on the HTTP body (express.raw({ limit: UPLOAD_SIZE_LIMIT }), via getInvoiceFileMaxSize()) and the size limit enforced inside StorageService (this.maxFileSize, via this local parser) can silently diverge for the same misconfigured env value — one path fails fast, the other quietly falls back to a possibly different default, and there's an extra untracked fallback (BODY_LIMIT_INVOICE) not present in the config schema at all.

♻️ Suggested fix: single source of truth

-function parseSize(sizeStr) {
-  if (typeof sizeStr !== 'string' || sizeStr.trim() === '') {
-    return DEFAULT_MAX_FILE_SIZE;
-  }
-  const match = sizeStr.trim().match(/^(\d+(?:\.\d+)?)\s*(b|kb|mb|gb)?$/i);
-  if (!match) {
-    return DEFAULT_MAX_FILE_SIZE;
-  }
-  const value = parseFloat(match[1]);
-  const unit = (match[2] || 'b').toLowerCase();
-  const multipliers = { b: 1, kb: 1024, mb: 1024 ** 2, gb: 1024 ** 3 };
-  return Math.floor(value * multipliers[unit]);
-}
-
-const MAX_FILE_SIZE = parseSize(process.env.INVOICE_FILE_MAX_SIZE || process.env.BODY_LIMIT_INVOICE || '5mb');
+const { getInvoiceFileMaxSize } = require('../config');
+
+function parseSize(sizeStr) {
+  // reuse same regex/units as InvoiceFileMaxSizeSchema
+  ...
+}
+
+const MAX_FILE_SIZE = parseSize(getInvoiceFileMaxSize());

🤖 Prompt for 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.

In `@src/services/storage.js` around lines 69 - 89, Remove the local
parseSize-based MAX_FILE_SIZE configuration and use getInvoiceFileMaxSize() from
src/config/index.js as the single source of truth for StorageService limits.
Eliminate the untracked BODY_LIMIT_INVOICE fallback and ensure invalid
INVOICE_FILE_MAX_SIZE values follow the config schema’s existing validation
behavior.

src/services/storage.js-375-394 (1)

375-394: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Swallowed errors bypass the PDF-header validation instead of failing safe.

The catch block only re-throws when e.code === 'INVALID_PDF_HEADER'; any other failure from the ranged GetObjectCommand (network error, permission denial, malformed response) is silently discarded, and the function proceeds to return valid as if the header check had passed. This defeats the purpose of the check whenever the range-GET fails for any external reason.

🛡️ Suggested fix: don't swallow unexpected errors

       } catch (e) {
         if (e.code === 'INVALID_PDF_HEADER') {
           throw e;
         }
+        logger.error({ err: e, key }, 'PDF header verification failed unexpectedly');
+        throw e;
       }

🤖 Prompt for 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.

In `@src/services/storage.js` around lines 375 - 394, Update the PDF validation
block around the ranged GetObjectCommand so every failure from s3Client.send or
response processing propagates instead of being swallowed. Preserve the existing
rethrow behavior for INVALID_PDF_HEADER and rethrow all other caught errors as
well, preventing the function from returning valid when validation cannot
complete.

src/routes/apiKeys.js-17-17 (1)

17-17: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

POST returns 201 but nothing is actually registered.

Created entries land only in the module-level runtimeEntries Map: they are not visible to authenticateApiKey (which reads the env-backed registry), are not shared across workers, and vanish on restart — yet the response claims 'API key created successfully.'. Either persist through the registry contract or make the endpoint read-only / clearly test-only and keep it out of the mounted app.

Also worth noting: the catch maps every thrown error to 422, so an unexpected runtime failure is reported as a client validation error, and validateEntry's messages leak the internal API_KEYS[0]: prefix into the response.

Also applies to: 65-91

🤖 Prompt for 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.

In `@src/routes/apiKeys.js` at line 17, Update the POST handler and module-level
runtimeEntries so created API keys use the same persistent registry consumed by
authenticateApiKey, or remove the endpoint from the mounted app if it is only
test-only; do not return a success response for memory-only registration. In the
POST catch path, distinguish client validation failures from unexpected runtime
errors instead of mapping every exception to 422. Sanitize validateEntry
messages before returning them so internal API_KEYS[0]: prefixes are not
exposed.

src/metrics.js-1077-1190 (1)

1077-1190: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Persistence metrics block is entirely unreferenced and fails lint.

PERSISTENCE_STATUS_CLASS_ENUM, PERSISTENCE_CAUSE_ENUM, the three normalizePersistence* helpers, and all three persistenceRequest* metrics are defined but never used or exported (8 ESLint no-unused-vars errors). Either add them to module.exports so src/middleware/persistenceMetrics.js can consume them, or drop the block.

Note also that normalizePersistenceCause(null, undefined) returns 'internal' rather than 'none', since Number(undefined) is NaN and NaN < 400 is false — worth guarding when the callers land.

🤖 Prompt for 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.

In `@src/metrics.js` around lines 1077 - 1190, Expose the persistence metrics
block for consumption by exporting PERSISTENCE_STATUS_CLASS_ENUM,
PERSISTENCE_CAUSE_ENUM, all normalizePersistence* helpers, and the three
persistenceRequest* metrics from the module. Ensure normalizePersistenceCause
treats null or undefined status with no error as the success cause “none” before
numeric status comparisons. Remove nothing else from the persistence metrics
implementation.

Source: Linters/SAST tools

src/middleware/stacks.js-33-36 (1)

33-36: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Presence check contradicts the documented branching contract.

docs/request-lifecycle-middleware-order.md (Lines 51-54, added in this PR) states that empty-string and array-valued x-api-key headers are non-branching and must fall back to JWT auth. hasOwnProperty branches on any present header, so X-API-Key: '' now routes to _adminApiKeyMiddleware and yields 401 instead of falling through to authenticateToken. Note src/routes/adminWebhooks.js Line 62 still uses the truthiness check, so the two admin auth paths disagree.

🐛 Proposed fix matching the documented contract

-  const apiKeyHeaderPresent = Object.prototype.hasOwnProperty.call(req.headers, 'x-api-key');
-
-  if (apiKeyHeaderPresent) {
+  const rawApiKey = req.headers['x-api-key'];
+  const hasUsableApiKey = typeof rawApiKey === 'string' && rawApiKey.trim() !== '';
+
+  if (hasUsableApiKey) {
     return _adminApiKeyMiddleware(req, res, next);
   }

🤖 Prompt for 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.

In `@src/middleware/stacks.js` around lines 33 - 36, Replace the
hasOwnProperty-based branching in the middleware stack with a truthiness check
for a valid scalar x-api-key value, so empty-string and array-valued headers
fall through to authenticateToken. Align this behavior with the existing check
in the admin authentication path while preserving _adminApiKeyMiddleware for
usable API key headers.

src/routes/adminWebhooks.js-35-51 (1)

35-51: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Restore the dead-letter route helpers

GET /dead-letters still uses responseHelper, MAX_LIMIT, DEFAULT_LIMIT, decodeCursor, encodeCursor, CursorError, SAFE_COLUMNS, DEAD_LETTER_SORT_FIELD, and redactRow, but this module no longer defines them. Any request to that route will hit a ReferenceError before returning a response; add the missing imports/constants back or move the helpers into this file.

🤖 Prompt for 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.

In `@src/routes/adminWebhooks.js` around lines 35 - 51, The dead-letter route in
adminWebhooks.js references undefined helpers and constants, causing
ReferenceErrors. Restore or import responseHelper, MAX_LIMIT, DEFAULT_LIMIT,
decodeCursor, encodeCursor, CursorError, SAFE_COLUMNS, DEAD_LETTER_SORT_FIELD,
and redactRow so GET /dead-letters can execute without changing its existing
behavior.

src/services/indexerService.js-170-176 (1)

170-176: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift

Unconditional COUNT(*) defeats the point of keyset pagination.

Every request — including cursor mode, whose comment at Line 202 claims hasMore is derived "without a second COUNT query" — runs a full filter-aware count over escrow_events, which the file header describes as growing unboundedly. On Postgres that is a sequential/index-only scan proportional to table size on each page fetch.

Consider computing total only in offset mode (where totalPages needs it), or gating it behind an opt-in includeTotal flag.

♻️ Proposed change

-  const countQ = baseQuery();
-  _applyFilters(countQ, filters);
-  const countRow = await countQ.count('* as total').first();
-  const total = parseInt(countRow.total ?? countRow['count(*)'] ?? 0, 10);
-
   const useCursor = Boolean(pagination.cursor);
+
+  let total = null;
+  if (!useCursor) {
+    const countQ = baseQuery();
+    _applyFilters(countQ, filters);
+    const countRow = await countQ.count('* as total').first();
+    total = parseInt(countRow.total ?? countRow['count(*)'] ?? 0, 10);
+  }

Note this changes the documented meta.total contract for cursor mode, so update the Swagger block in src/routes/adminIndexer.js and tests/indexerListing.test.js accordingly.

🤖 Prompt for 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.

In `@src/services/indexerService.js` around lines 170 - 176, Make the count query
in the indexer listing flow conditional on offset pagination, so cursor requests
avoid executing countQ and set meta.total to the cursor-mode contract value
while preserving totalPages for offset requests. Update the cursor pagination
documentation in the adminIndexer Swagger block and the corresponding
expectations in indexerListing.test.js to reflect the changed meta.total
behavior.

tests/jobPersistence.listJobs.test.js-177-203 (1)

177-203: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Shared in-memory Knex fake mis-evaluates orWhere, invalidating every cursor-traversal assertion. Both files copy the same evaluator, which ANDs the or_group with the preceding comparison instead of ORing it. listJobs emits sortField < v OR (sortField = v AND id < cursorId), so under the fake no row can satisfy both branches and pages after the first come back empty.

• tests/jobPersistence.listJobs.test.js#L177-L203: change the or_group branch to disjunction — result = result || orResult (semantically, accumulate the non-group conditions and OR the group in), then confirm the traversal expectations at Lines 453-528 and 695-712 pass.
• tests/adminJobs.route.test.js#L185-L210: apply the same OR fix in evalConds, and re-check the multi-page tests at Lines 460-503, which currently dereference p3.body.meta after a cursor=null request would 400.

🤖 Prompt for 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.

In `@tests/jobPersistence.listJobs.test.js` around lines 177 - 203, The shared
in-memory evaluators incorrectly AND each or_group with prior conditions; update
evalConditions in tests/jobPersistence.listJobs.test.js lines 177-203 and
evalConds in tests/adminJobs.route.test.js lines 185-210 to accumulate the OR
group as a disjunction, preserving the intended cursor predicate behavior.
Recheck the traversal expectations in tests/jobPersistence.listJobs.test.js
lines 453-528 and 695-712, and the multi-page tests in
tests/adminJobs.route.test.js lines 460-503; these sites require no direct
changes.

src/utils/healthCursorPagination.js-36-94 (1)

36-94: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift

Two copies of the HMAC cursor implementation, already diverging. Both files independently implement secret resolution, base64url+HMAC-SHA256 signing, and constant-time verification; the copies differ in dev-secret constant, TTL enforcement, and the zero-length signature guard, so a future fix to one will silently miss the other.

• src/utils/healthCursorPagination.js#L36-L94: extract _resolveSecret/_sign/encode/decode into a shared module (e.g. src/utils/signedCursor.js) parameterised by payload validator and dev-secret label, and re-export the health-specific wrappers from it.
• src/workers/jobPersistence.js#L72-L165: replace _resolveJobCursorSecret/encodeJobCursor/decodeJobCursor with calls into that shared module, keeping JobCursorError and the LIST_JOBS_SORT_FIELDS payload check as the job-specific validator, and pick up the TTL enforcement the health version already has.

🤖 Prompt for 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.

In `@src/utils/healthCursorPagination.js` around lines 36 - 94, Extract the
duplicated secret resolution, HMAC signing, encoding, decoding, constant-time
verification, and TTL handling into a shared signed-cursor module parameterized
by payload validation and development-secret configuration. In
src/utils/healthCursorPagination.js lines 36-94, replace the local
_resolveSecret, _sign, and cursor encode/decode logic with health-specific
wrappers using the shared module. In src/workers/jobPersistence.js lines 72-165,
replace _resolveJobCursorSecret and the job cursor encode/decode implementation
with the shared module while preserving JobCursorError, LIST_JOBS_SORT_FIELDS
validation, and enabling the shared TTL enforcement.

src/routes/sme/metrics.js-116-150 (1)

116-150: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Inconsistent error envelope for cursor errors vs. tenant-validation errors.

validateMetricsRequest (Line 118) sends { error: 'Bad Request', message: '...' } / { error: 'Forbidden', message: '...' } on failure, but the CursorError branch here (Lines 144-148) sends a completely different shape: { error: { message: err.message } }. Both are 400/403 failures on the same endpoint, yet callers must handle two incompatible error contracts. The CursorError response also drops the data/meta/timestamp envelope fields present in every success response, unlike the RFC7807-style shape used for the equivalent CursorError case in src/app.js's /api/invoices handler.

🔧 Suggested fix — align with the existing tenant-validation shape

       if (err.name === 'CursorError' || err instanceof CursorError) {
         return res.status(400).json({
-          error: { message: err.message },
+          error: 'Bad Request',
+          message: err.message,
         });
       }

🤖 Prompt for 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.

In `@src/routes/sme/metrics.js` around lines 116 - 150, Update the CursorError
handling in the /metrics route to use the same error response envelope as
validateMetricsRequest, including the established error status/message fields
and required data, meta, and timestamp fields. Preserve the 400 status and
cursor error message while aligning with the endpoint’s existing validation
contract.

tests/invoice.pagination.test.js-59-74 (1)

59-74: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Fixtures only use string ids, hiding the encodeCursor id-type contract.

makeRow('a') produces id: 'a', so encodeCursor's "id must be a non-empty string" guard is never exercised with the numeric ids the invoices table actually produces (see getSmeInvoiceList, which stringifies lastRow.id). Add a case with makeRow(1) and hasMore=true to cover the real shape — see the related comment on src/services/invoiceService.js.

🤖 Prompt for 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.

In `@tests/invoice.pagination.test.js` around lines 59 - 74, Extend the pagination
fixtures around makeRow and makeCursor with a case using makeRow(1) and
hasMore=true, verifying cursor creation succeeds for the numeric database id
shape after the service stringifies it. Keep the existing string-id coverage
unchanged and target the behavior exercised by getSmeInvoiceList.

src/services/invoiceService.js-754-762 (1)

754-762: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Tiebreaker comparison coerces id to an integer and silently falls back to 0.

parseInt(String(cursorData.id), 10) || 0 yields 0 for any non-numeric id (UUID/prefixed string), making id < 0 always false. Rows sharing the same created_at as the cursor row are then dropped from the next page. It also compares a numeric literal against a possibly text column. Compare the id in its native form instead of coercing, matching the encode side which stores String(lastRow.id).

🐛 Proposed fix

-          this.where('created_at', cursorData.sortValue)
-            .andWhere('id', '<', parseInt(String(cursorData.id), 10) || 0);
+          this.where('created_at', cursorData.sortValue)
+            .andWhere('id', '<', cursorData.id);

🤖 Prompt for 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.

In `@src/services/invoiceService.js` around lines 754 - 762, Update the cursor
tiebreaker in the dataQuery filter to compare id using cursorData.id in its
native string form, removing parseInt and the fallback to 0. Preserve the
existing created_at and id ordering logic, matching the String(lastRow.id)
representation used when encoding the cursor.

src/services/invoiceStateMachine.js-226-252 (1)

226-252: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

reason length bound is only enforced for terminal targets, not universally.

MAX_TRANSITION_REASON_LENGTH is only checked inside TERMINAL_REASON_REQUIRED_STATES.includes(targetState). A transition to approved or linked_escrow with an arbitrarily long reason bypasses TRANSITION_REASON_TOO_LONG entirely and gets persisted verbatim into the audit-log metadata. This contradicts the documented contract ("capped at 1024 characters", see docs/invoice-lifecycle-api.md line 44 and docs/invoice-state.md line 236) and the independent bound declared in src/schemas/invoiceState.js.

🐛 Proposed fix

   const reason = normalizeTransitionReason(rawReason);

   if (isTerminalState(currentState)) {
     return {
       isValid: false,
       error: `Cannot transition from terminal state: ${currentState}`,
       code: 'TERMINAL_STATE',
     };
   }

+  if (reason && reason.length > MAX_TRANSITION_REASON_LENGTH) {
+    return {
+      isValid: false,
+      error: `Transition reason must be ${MAX_TRANSITION_REASON_LENGTH} characters or fewer`,
+      code: 'TRANSITION_REASON_TOO_LONG',
+    };
+  }
+
   if (TERMINAL_REASON_REQUIRED_STATES.includes(targetState)) {
     if (!reason) {
       return {
         isValid: false,
         error: `Reason is required for terminal transition to ${targetState}`,
         code: 'MISSING_TRANSITION_REASON',
       };
     }
-
-    if (reason.length > MAX_TRANSITION_REASON_LENGTH) {
-      return {
-        isValid: false,
-        error: `Transition reason must be ${MAX_TRANSITION_REASON_LENGTH} characters or fewer`,
-        code: 'TRANSITION_REASON_TOO_LONG',
-      };
-    }
   }

🤖 Prompt for 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.

In `@src/services/invoiceStateMachine.js` around lines 226 - 252, Enforce
MAX_TRANSITION_REASON_LENGTH for every transition reason in the transition
validation flow, not only when TERMINAL_REASON_REQUIRED_STATES includes
targetState. Preserve the existing missing-reason validation for required
terminal states, and return TRANSITION_REASON_TOO_LONG whenever a provided
reason exceeds the limit before it can be persisted.

tests/invoice.stateValidation.test.js-611-630 (1)

611-630: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Route integration tests call the wrong endpoint

postTransition() sends requests to /api/invoices/transition, but src/routes/invoiceStateRoutes.js only exposes POST /:id/transition under /api/invoices. These cases will 404 instead of exercising the transition handler or returning the asserted requiresKYC / RFC 7807 payloads. Update the helper/tests to include an invoice id.

🤖 Prompt for 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.

In `@tests/invoice.stateValidation.test.js` around lines 611 - 630, The route
integration helper postTransition() targets the collection endpoint instead of
the id-scoped transition route. Update postTransition() and its verified,
funded, and settled callers to include a valid invoice id so requests reach POST
/:id/transition under /api/invoices and preserve the existing assertions.

src/schemas/invoiceState.js-387-399 (1)

387-399: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Update the integration test route

src/routes/invoiceStateRoutes.js only exposes POST /api/invoices/:id/transition (mounted under /api/invoices), so the /api/invoices/transition assertions in tests/invoice.stateValidation.test.js miss the real handler. Point those tests at an invoice id or they won’t exercise this route.

🤖 Prompt for 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.

In `@src/schemas/invoiceState.js` around lines 387 - 399, Update the integration
tests in invoice.stateValidation.test.js to call the mounted POST
/api/invoices/:id/transition endpoint using a valid invoice id, replacing
assertions that target /api/invoices/transition. Keep the existing validation
expectations unchanged while ensuring requests reach the handler defined by
invoiceStateRoutes.js.

[coderabbitai]

🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win

adminAuth returns an undefined identifier and is never applied to any route.

Line 65 return out; references a nonexistent binding — the JWT fallback was lost in editing, so any non-API-key request throws ReferenceError: out is not defined. Worse, ESLint reports both authenticateToken and adminAuth as unused, meaning none of the four admin routes below actually attach auth, directly contradicting the @fileoverview claim that every route requires an admin JWT or API key. /api/admin/webhooks/* (replay, resolve, dead-letter listing) is currently open to anonymous callers.

🔒 Proposed fix

 function adminAuth(req, res, next) {
   if (req.headers['x-api-key']) {
     return _adminApiKeyMiddleware(req, res, next);
   }
-  return out;
+  return authenticateToken(req, res, next);
 }

Then attach it (plus tenant extraction) to each route, e.g.:

router.get('/dead-letters', adminAuth, extractTenant, async (req, res, next) => { /* … */ });
router.post('/replay/:id', adminAuth, extractTenant, async (req, res) => { /* … */ });
router.post('/replay', adminAuth, extractTenant, async (req, res) => { /* … */ });
router.post('/resolve/:id', adminAuth, extractTenant, async (req, res) => { /* … */ });

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	function adminAuth(req, res, next) {
	if (req.headers['x-api-key']) {
	return _adminApiKeyMiddleware(req, res, next);
	}
	return out;
	}
	function adminAuth(req, res, next) {
	if (req.headers['x-api-key']) {
	return _adminApiKeyMiddleware(req, res, next);
	}
	return authenticateToken(req, res, next);
	}

🧰 Tools

🪛 ESLint

[error] 61-61: 'adminAuth' is defined but never used. Allowed unused vars must match /^_/u.

(no-unused-vars)

---

[error] 65-65: 'out' is not defined.

(no-undef)

🤖 Prompt for 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.

In `@src/routes/adminWebhooks.js` around lines 61 - 66, Fix adminAuth so
non-API-key requests fall back to authenticateToken instead of returning the
undefined out identifier, then attach adminAuth and extractTenant to every admin
webhook route, including dead-letter listing, replay, and resolve handlers.
Ensure all four routes require either an admin API key or JWT plus tenant
extraction.

Source: Linters/SAST tools

[coderabbitai]

🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Confirm whether the apiKeys router is mounted in the production app and with what auth.
rg -nP --type=js -C4 "routes/apiKeys" src/
rg -nP --type=js -C3 "adminStack|adminAuth|authenticateApiKey" src/routes/apiKeys.js

Repository: Ukorstack/Liquifact-backend

Length of output: 1067

---

Protect API-key routes and redact secrets. listApiKeysHandler and getApiKeyHandler return plaintext key values, and these routes are mounted without any auth middleware. Gate them behind admin auth and return only a masked/fingerprinted key instead of the raw secret.

🧰 Tools

🪛 ESLint

[error] 93-94: Missing JSDoc comment.

(jsdoc/require-jsdoc)

---

[error] 103-104: Missing JSDoc comment.

(jsdoc/require-jsdoc)

🤖 Prompt for 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.

In `@src/routes/apiKeys.js` around lines 93 - 125, Protect the API-key list and
lookup routes by applying the existing admin-auth middleware to both `/api-keys`
and `/keys` route families, including parameterized endpoints. Update
`listApiKeysHandler` and `getApiKeyHandler` to replace each plaintext `key` with
the established masked or fingerprinted representation before responding, while
preserving the existing response structure and status behavior.

[coderabbitai]

🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win

Cursor pagination can never advance past page 1.

listHealthChecks() is called without snapshotTime, so it stamps every record with a fresh new Date(Date.now()).toISOString() on each request (see src/services/health.js Lines 564-566). The cursor encodes the previous request's timestamp, so on the follow-up request findIndex((c) => c.timestamp === afterTimestamp && c.id === afterId) never matches and the handler returns an empty page with hasMore: false — clients silently lose every record after limit.

Either carry the snapshot instant in the cursor and pass it back into listHealthChecks({ snapshotTime }), or key the cursor on id alone (the roster is a fixed, ordered set within a snapshot).

🐛 Minimal fix: match on the stable `id`

-      const cursorIdx = allChecks.findIndex(
-        (c) => c.timestamp === afterTimestamp && c.id === afterId,
-      );
+      const cursorIdx = allChecks.findIndex((c) => c.id === afterId);

Secondary concern on the same path: each page re-executes all six live dependency probes (Soroban RPC, DB, KYC, storage, reconciliation) and throws away everything outside the page, so paginating amplifies upstream load on an endpoint with no auth in front of it. Consider caching the snapshot for a short TTL keyed by the cursor's instant.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	// Fetch the full ordered list of health check records.
	const allChecks = await listHealthChecks();
	// Apply keyset filtering when a cursor was supplied.
	// Records are ordered by (timestamp ASC, id ASC); the cursor points to the
	// last item on the previous page so we skip everything up to and including
	// that item.
	let filteredChecks;
	if (afterTimestamp !== null && afterId !== null) {
	const cursorIdx = allChecks.findIndex(
	(c) => c.timestamp === afterTimestamp && c.id === afterId,
	);
	if (cursorIdx === -1) {
	// The cursor pointed to a record that no longer exists in the current
	// snapshot (e.g. a check was removed). Return an empty page so the
	// caller knows it has reached the end rather than silently restarting.
	filteredChecks = [];
	} else {
	filteredChecks = allChecks.slice(cursorIdx + 1);
	}
	} else {
	filteredChecks = allChecks;
	}
	// Slice to the requested page size + 1 to detect whether more pages exist.
	const page = filteredChecks.slice(0, limit);
	const hasMore = filteredChecks.length > limit;
	// Build the next cursor from the last item on this page.
	let nextCursor = null;
	if (hasMore && page.length > 0) {
	const last = page[page.length - 1];
	nextCursor = encodeHealthCursor({ timestamp: last.timestamp, id: last.id });
	}
	// Fetch the full ordered list of health check records.
	const allChecks = await listHealthChecks();
	// Apply keyset filtering when a cursor was supplied.
	// Records are ordered by (timestamp ASC, id ASC); the cursor points to the
	// last item on the previous page so we skip everything up to and including
	// that item.
	let filteredChecks;
	if (afterTimestamp !== null && afterId !== null) {
	const cursorIdx = allChecks.findIndex((c) => c.id === afterId);
	if (cursorIdx === -1) {
	// The cursor pointed to a record that no longer exists in the current
	// snapshot (e.g. a check was removed). Return an empty page so the
	// caller knows it has reached the end rather than silently restarting.
	filteredChecks = [];
	} else {
	filteredChecks = allChecks.slice(cursorIdx + 1);
	}
	} else {
	filteredChecks = allChecks;
	}
	// Slice to the requested page size + 1 to detect whether more pages exist.
	const page = filteredChecks.slice(0, limit);
	const hasMore = filteredChecks.length > limit;
	// Build the next cursor from the last item on this page.
	let nextCursor = null;
	if (hasMore && page.length > 0) {
	const last = page[page.length - 1];
	nextCursor = encodeHealthCursor({ timestamp: last.timestamp, id: last.id });
	}

🤖 Prompt for 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.

In `@src/routes/health.js` around lines 117 - 151, Update the cursor matching
logic in the health route to use the stable record id rather than requiring an
exact timestamp match, so pagination can advance across requests despite
refreshed timestamps. Preserve the existing ordered slicing and empty-page
behavior when the cursor id is absent; do not address the separate snapshot
caching concern.

[coderabbitai]

🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
rg -n '"zod"' package.json
rg -n 'invalid_type_error|required_error|errorMap|ZodIssueCode|z\.record\(' src/schemas/ | head -50

Repository: Ukorstack/Liquifact-backend

Length of output: 4528

---

src/schemas/config.js needs a Zod 4 migration. Update the module to the Zod 4 API:

• src/schemas/config.js#L39-L90 and the inline schemas at L113, L120-L123, L156, L189, L231-L234: replace invalid_type_error / required_error / errorMap with error.
• src/schemas/config.js#L270-L283: replace code: z.ZodIssueCode.custom with code: 'custom'.
• src/schemas/config.js#L316-L336: change the record schema to z.record(z.string(), z.unknown()); the current call shape is not valid for Zod 4.

📍 Affects 1 file

• src/schemas/config.js#L316-L336 (this comment)
• src/schemas/config.js#L270-L283
• src/schemas/config.js#L39-L90

🤖 Prompt for 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.

In `@src/schemas/config.js` around lines 316 - 336, Update src/schemas/config.js
across lines 39-90, 113, 120-123, 156, 189, 231-234, 270-283, and 316-336 for
Zod 4 compatibility: replace legacy invalid_type_error, required_error, and
errorMap options with the error API; use the string literal custom issue code
instead of z.ZodIssueCode.custom; and change runtimeConfigSchema.config to
z.record(z.string(), z.unknown()) while preserving its plain-object validation.

[coderabbitai]

🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win

Inconsistent id typing across the cursor encode/decode boundary. encodeCursor requires a non-empty string id and decodeCursor returns one, but this file passes raw ids in one function and integer-coerces them in the other; the tests only use string ids, so neither path is covered for the real numeric invoices.id.

• src/services/invoiceService.js#L249-L263: pass id: String(lastRow.id) to encodeCursor (apply the same change in the offset branch at Lines 281-285) so a non-string id cannot throw a generic Error and surface as a 500.
• src/services/invoiceService.js#L754-L762: compare the tiebreaker as .andWhere('id', '<', cursorData.id) instead of parseInt(String(cursorData.id), 10) || 0, which silently collapses non-numeric ids to 0 and drops rows sharing the cursor's created_at.
• tests/invoice.pagination.test.js#L59-L74: add a fixture with a numeric id (e.g. makeRow(1)) and hasMore=true to cover the real row shape.

📍 Affects 2 files

• src/services/invoiceService.js#L249-L263 (this comment)
• src/services/invoiceService.js#L754-L762
• tests/invoice.pagination.test.js#L59-L74

🤖 Prompt for 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.

In `@src/services/invoiceService.js` around lines 249 - 263, Normalize invoice
cursor IDs to strings when calling encodeCursor in the limit and offset
pagination branches, update the cursor tiebreaker query to compare directly with
cursorData.id instead of parseInt fallback coercion, and add a pagination
fixture using a numeric invoice ID with hasMore=true in
tests/invoice.pagination.test.js:59-74.

[coderabbitai]

Actionable comments posted: 3

Note

Due to the large number of review comments, Critical severity comments were prioritized as inline comments.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

src/services/invoiceService.js (1)

552-575: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Transition is not atomic: the audit log is committed before the status write.

executeTransition persists the STATE_TRANSITION audit entry, then updateInvoice runs separately. If the update fails (or the process dies in between) the audit trail claims a transition that never happened, and the caller still gets a 500. Wrap both writes in a single Knex transaction, or move audit-log creation after a successful status update.

🤖 Prompt for 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.

In `@src/services/invoiceService.js` around lines 552 - 575, The transition flow
around executeTransition and updateInvoice is non-atomic because the audit entry
is persisted before the invoice update. Use a single Knex transaction
encompassing both the STATE_TRANSITION audit write and status/metadata update,
ensuring both commit together or roll back together; preserve the existing
result return behavior.

src/metrics.js (1)

1077-1244: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win

Persistence metric symbols are defined but never exported — breaks persistenceMetrics.js.

PERSISTENCE_ENDPOINT_ENUM/PERSISTENCE_STATUS_CLASS_ENUM/PERSISTENCE_CAUSE_ENUM, normalizePersistenceEndpoint/normalizePersistenceStatusClass/normalizePersistenceCause, and persistenceRequestDurationSeconds/persistenceRequestsTotal/persistenceRequestErrorsTotal are all defined in this hunk but none of them appear in module.exports (Lines 1225-1244). ESLint's no-unused-vars hints for every one of these symbols confirm they are unreferenced within this file.

Cross-file evidence (src/middleware/persistenceMetrics.js's recordPersistenceOutcome) destructure-imports exactly these names from ../metrics and call .labels(...)/.observe(...)/.inc(...) on them directly — with the exports missing, those destructured bindings resolve to undefined, so the very first persistence-tracked request (e.g. SME invoice upload) would throw TypeError: Cannot read properties of undefined (reading 'labels').

The Biome duplicate-key hint on normalizeReminderReason (Lines 1240 and 1244) suggests the second occurrence was meant to hold one of these missing exports and was pasted incorrectly instead.

🐛 Proposed fix — add the missing exports and drop the duplicate key

   sorobanCircuitBreakerStateTransitionsTotal,
   apiKeyAuthDurationSeconds,
   apiKeyAuthErrorsTotal,
   API_KEY_ERROR_CAUSE_ENUM,
   API_KEY_OUTCOME_ENUM,
   classifyApiKeyOutcome,
   classifyApiKeyErrorCause,
+  PERSISTENCE_ENDPOINT_ENUM,
+  PERSISTENCE_STATUS_CLASS_ENUM,
+  PERSISTENCE_CAUSE_ENUM,
+  normalizePersistenceEndpoint,
+  normalizePersistenceStatusClass,
+  normalizePersistenceCause,
+  persistenceRequestDurationSeconds,
+  persistenceRequestsTotal,
+  persistenceRequestErrorsTotal,
   normalizeJobType,
   normalizeReminderReason,
   normalizeSorobanRpcMethod,
   normalizeSorobanRpcOutcome,
   normalizeSorobanRetryCause,
-  normalizeReminderReason,

Verification:

#!/bin/bash
rg -n "normalizePersistenceEndpoint|persistenceRequestDurationSeconds|PERSISTENCE_CAUSE_ENUM" src/middleware/persistenceMetrics.js
rg -n "module.exports" -A 30 src/metrics.js | rg -n "persistence|Persistence"

🤖 Prompt for 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.

In `@src/metrics.js` around lines 1077 - 1244, Export all persistence metric
enums, normalization helpers, and metric instances from module.exports in
metrics.js so persistenceMetrics.js can use them. Remove the duplicate
normalizeReminderReason entry while adding the missing persistence symbols,
preserving the existing export names and behavior.

Source: Linters/SAST tools

🟠 Major comments (19)

src/errors/mapError.js-109-117 (1)

109-117: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Forward normalized extensions to every problem response.

mapError() computes code, retryable, and retryHint, but problemJsonHandler forwards raw fields only for AppError. Consequently generic 429/503, parser, CORS, and upstream errors omit the documented extensions; an AppError relying on status-derived code also loses it.

• src/errors/mapError.js#L109-L117: preserve this normalized contract by passing mappedError.code, mappedError.retryable, and mappedError.retryHint to formatProblemDetails.
• src/utils/problemDetails.js#L4-L10: scope the “every error response” claim until all error paths use the builder.
• docs/RFC7807-Error-Handling.md#L97-L124: do not promise extensions on every response until the handler forwards mapped values.
• tests/problems.test.js#L571-L575: expect generic responses to contain the normalized extension fields after fixing the handler.

Proposed handler change

-    code: isAppError ? error.code : undefined,
-    retryable: isAppError ? error.retryable : undefined,
-    retryHint: isAppError ? error.retryHint : undefined,
+    code: mappedError.code,
+    retryable: mappedError.retryable,
+    retryHint: mappedError.retryHint,

🤖 Prompt for 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.

In `@src/errors/mapError.js` around lines 109 - 117, Update problemJsonHandler in
src/errors/mapError.js (lines 109-117) to pass mappedError.code,
mappedError.retryable, and mappedError.retryHint into formatProblemDetails for
every error path. In src/utils/problemDetails.js (lines 4-10) and
docs/RFC7807-Error-Handling.md (lines 97-124), limit claims about extensions to
responses that use the builder. Update tests/problems.test.js (lines 571-575) to
expect normalized extension fields on generic responses.

tests/escrow.read.test.js-218-234 (1)

218-234: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Align unmapped and blank-ID coverage with the resolver contract. resolveEscrowAddress throws when no active mapping exists, but these mocks/tests fabricate or accept mappings and normalize unmapped/blank IDs into successful neutral reads.

• tests/escrow.read.test.js#L218-L234: use an explicitly mapped ID with no projection to test the neutral stub; assert the chosen invalid/unmapped behavior separately.
• tests/integration/v1.escrow.indexer.test.js#L58-L65: throw the production-equivalent not-found error for unmapped IDs.
• tests/v1.escrow.read.test.js#L24-L30: do not synthesize escrow addresses for arbitrary invoice IDs.
• docs/escrow-read.md#L228-L235: make the documented validation/not-found contract match the enforced endpoint behavior.

🤖 Prompt for 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.

In `@tests/escrow.read.test.js` around lines 218 - 234, Align the escrow resolver
contract across all affected sites: in tests/escrow.read.test.js lines 218-234,
use an explicitly mapped invoice ID without projection for the neutral-stub case
and separately assert the invalid/unmapped behavior; in
tests/integration/v1.escrow.indexer.test.js lines 58-65, make the mock throw the
production-equivalent not-found error for unmapped IDs; in
tests/v1.escrow.read.test.js lines 24-30, stop synthesizing escrow addresses for
arbitrary invoice IDs; and in docs/escrow-read.md lines 228-235, document the
enforced validation and not-found behavior.

.kiro/specs/escrow-read-cursor-pagination/requirements.md-5-5 (1)

5-5: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift

Replace this with the escrow-read requirements.

This spec defines an InvestService pagination refactor, not escrow-read behavior. It will direct implementation and acceptance tests at unrelated files/routes; move it to the invest feature or rewrite it for the escrow endpoints.

🤖 Prompt for 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.

In @.kiro/specs/escrow-read-cursor-pagination/requirements.md at line 5, Replace
the requirements content with escrow-read behavior and acceptance criteria,
removing references to InvestService, listInvestments, investment routes, and
cursor pagination. Ensure the rewritten requirements target the escrow-read
endpoints, files, response shapes, and expected validation/error handling.

src/routes/adminConfig.js-160-164 (1)

160-164: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Echoing config back reflects submitted secrets in the response body.

webhook.secret and kyc.apiKey (see src/schemas/config.js Lines 112-115, 188-191) are returned verbatim. Reflected secrets end up in proxy access logs, browser devtools, and any client-side response caching. Redact secret-bearing fields, or return only section plus the non-secret keys that were accepted.

🤖 Prompt for 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.

In `@src/routes/adminConfig.js` around lines 160 - 164, Update the response
construction in the admin configuration validation handler to avoid returning
validatedConfig, since it may contain secrets such as webhook.secret and
kyc.apiKey. Return section plus only explicitly non-secret accepted
configuration fields, or apply the established redaction behavior before
res.status(200).json responds.

src/schemas/config.js-53-58 (1)

53-58: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

boundedUrl accepts http://, contradicting the documented HTTPS requirement.

The JSDoc for webhook.url (Line 98) and kyc.providerUrl (Line 174) both say HTTPS, but .url() allows any scheme — including http: (plaintext delivery of signed payloads / API keys) and, depending on the Zod version, non-HTTP schemes. Enforce the scheme explicitly.

🔒 Proposed fix

 function boundedUrl(label) {
   return z
     .string({ invalid_type_error: `${label} must be a string` })
     .max(2048, { message: `${label} must not exceed 2048 characters` })
-    .url({ message: `${label} must be a valid URL` });
+    .url({ message: `${label} must be a valid URL` })
+    .refine((v) => v.startsWith('https://'), {
+      message: `${label} must use https`,
+    });
 }

🤖 Prompt for 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.

In `@src/schemas/config.js` around lines 53 - 58, Update boundedUrl to enforce
HTTPS explicitly after URL validation, rejecting http and non-HTTP(S) schemes
while preserving the existing string type and 2048-character constraints and
label-specific error messages.

src/middleware/rateLimit.js-246-252 (1)

246-252: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Hash the API key before using it as a rate-limit bucket identifier.

apikey_${apiKey} embeds the raw credential in the Redis key (rate-limit:config:apikey_<secret>), where it is exposed to KEYS/SCAN/MONITOR/SLOWLOG and to any key-level monitoring dashboards — the same dashboards the JSDoc points at. A truncated SHA-256 preserves per-client bucketing and legibility without persisting secret material.

🔒 Proposed fix

+const crypto = require('node:crypto');
+
 function adminConfigKeyGenerator(req) {
   const apiKey = getApiKey(req);
   if (apiKey) {
-    return `apikey_${apiKey}`;
+    const digest = crypto.createHash('sha256').update(apiKey).digest('hex');
+    return `apikey_${digest.slice(0, 32)}`;
   }
   return req.ip || req.socket?.remoteAddress || '127.0.0.1';
 }

Note tests/unit/adminConfig.rateLimit.test.js Lines 242-266 assert the plaintext prefix and would need updating.

🤖 Prompt for 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.

In `@src/middleware/rateLimit.js` around lines 246 - 252, Update
adminConfigKeyGenerator to hash the API key with SHA-256 and use a truncated
digest in the apikey_ bucket identifier instead of embedding the raw credential.
Preserve distinct per-client buckets and the existing IP fallback, and update
affected tests to assert the hashed key format.

src/schemas/config.js-331-335 (1)

331-335: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Migrate these schemas to the Zod 4 API

• src/schemas/config.js#L319-L335 and the other primitive helpers in this file: replace invalid_type_error / required_error / errorMap with error.
• src/schemas/config.js#L331-L335: z.record() needs a real value schema; use z.record(z.string(), z.unknown()) instead of passing the options object in the second slot.

🤖 Prompt for 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.

In `@src/schemas/config.js` around lines 331 - 335, Migrate the affected schemas
in src/schemas/config.js at lines 319-335, 270-283, and 331-335 to Zod 4 error
options: replace invalid_type_error, required_error, and errorMap with error
while preserving existing messages. In the config schema at lines 331-335,
update z.record to provide z.string() as the key schema and z.unknown() as the
value schema, retaining the plain-object refinement.

src/services/kycService.js-795-804 (1)

795-804: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Shared root cause: in-memory mockKycRecords is written before the DB persist is awaited in all three status mutators. A rejected persistKycRecord leaves the process serving a status that was never persisted, and getKycStatus prefers the mock record when the DB row is absent.

• src/services/kycService.js#L795-L804: move mockKycRecords.set(smeId, record) to after the persistKycRecord await in verifySmeSafe.
• src/services/kycService.js#L876-L884: move mockKycRecords.set(smeId, record) to after the persistKycRecord await in exemptSmeFromKyc (exempted also passes the funding gate).
• src/services/kycService.js#L836-L844: move mockKycRecords.set(smeId, record) to after the persistKycRecord await in rejectSmeKyc for consistency.

🤖 Prompt for 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.

In `@src/services/kycService.js` around lines 795 - 804, In
src/services/kycService.js lines 795-804, move mockKycRecords.set in
verifySmeSafe to after the awaited persistKycRecord call; apply the same
ordering in exemptSmeFromKyc at lines 876-884 and rejectSmeKyc at lines 836-844.
Ensure each in-memory status is updated only after database persistence
succeeds.

src/services/kycService.js-620-647 (1)

620-647: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Shared root cause: strict response-signature mode is gated on config.apiSecret, so it silently does nothing when no secret is configured — while the docs promise the header is required.

• src/services/kycService.js#L620-L647: fail closed when verifyResponseSignature is true but apiSecret is missing, instead of skipping the whole verification block.
• docs/compliance.md#L358-L358: state that strict mode additionally requires KYC_PROVIDER_SECRET, matching the corrected behavior.

🤖 Prompt for 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.

In `@src/services/kycService.js` around lines 620 - 647, Update the
response-signature validation in src/services/kycService.js lines 620-647 so
verifyResponseSignature true with no apiSecret fails closed with the existing
missing-signature KycProviderError path, rather than skipping verification;
retain normal signature verification when apiSecret is configured. Update
docs/compliance.md line 358 to state that strict mode additionally requires
KYC_PROVIDER_SECRET.

src/services/kycService.js-548-686 (1)

548-686: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Timeout is per attempt only — no overall deadline for a request-path call.

config.timeoutMs bounds each fetch, but withRetry can issue 1 + maxRetries attempts plus backoff. Worst case with the clamp ceilings (30s timeout, 10 retries, 5s cap) is minutes of latency on an inbound request, since getKycStatus runs on the request thread via KYC gating. Consider computing a single deadline before the retry loop and deriving each attempt's timeout from the remaining budget (or bounding timeoutMs * (maxRetries + 1)).

🤖 Prompt for 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.

In `@src/services/kycService.js` around lines 548 - 686, Add an overall deadline
before the `sharedKycBreaker.execute`/`withRetry` flow and ensure each
`operation` attempt derives its AbortController timeout from the remaining
budget rather than always using `config.timeoutMs`. Preserve retry behavior
while preventing retries and backoff from exceeding the request-path deadline,
and stop retrying when the deadline is exhausted.

tests/indexerListing.test.js-197-264 (1)

197-264: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

The "cursor-mode" tests never pass a cursor — the keyset branch is untested.

Only pagination: { limit } is supplied in the tests at lines 198, 212, 231 and 251, so useCursor is false in listIndexerEvents and every one of these exercises the offset branch. Combined with the fake knex dropping the keyset callback (line 56, if (typeof fieldOrFn === 'function') { return q; }), the actual keyset predicate in src/services/indexerService.js lines 188–200 has no passing-path coverage — a wrong comparison operator or a swapped ASC/DESC tiebreaker would not be caught.

Suggest: capture the where(function(){…}) callback in the fake (invoke it against a recording builder) and add a round-trip test that feeds result.meta.nextCursor back in as pagination.cursor and asserts the second page excludes the first page's rows.

🤖 Prompt for 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.

In `@tests/indexerListing.test.js` around lines 197 - 264, Update the cursor-mode
tests around listIndexerEvents to actually pass a cursor: add a round-trip case
that requests the first page, feeds result.meta.nextCursor into
pagination.cursor for the next request, and verifies the second page excludes
the first page’s rows. Modify makeFakeKnex so its where(function(){…}) path
invokes the callback against a recording builder, allowing the keyset predicate
in the cursor pagination branch to be exercised, including sort-field and
ASC/DESC tie-breaker behavior.

tests/invest.list.test.js-110-147 (1)

110-147: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Mocking ../src/app reimplements the route under test, voiding integration coverage.

createApp is replaced with a hand-written handler that duplicates the real /api/invest/opportunities logic (the comment on line 120 concedes it only "mirrors" it). Any drift in the real route — parameter parsing, validation, response envelope — now passes here, and the inline error handler returns { error: message } instead of the app's RFC 7807 problem shape, so error-path assertions validate a contract that doesn't exist in production.

Mount the real router in a minimal app instead of re-implementing the handler:

♻️ Suggested direction

 jest.mock('../src/app', () => {
   const express = require('express');
-  const { authenticatedTenantStack } = require('../src/middleware/stacks');
-  const { listOpportunities } = require('../src/services/investService');
+  const investRouter = require('../src/routes/invest');
 
   return {
     createApp: () => {
       const app = express();
-      app.get('/api/invest/opportunities', ...authenticatedTenantStack, async (req, res, next) => {
-        // …duplicated handler…
-      });
+      app.use(express.json());
+      app.use('/api/invest', investRouter);
       app.use((error, _req, res, _next) => {
         res.status(error.status || 500).json({ error: error.message });
       });
       return app;
     },
   };
 });

🤖 Prompt for 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.

In `@tests/invest.list.test.js` around lines 110 - 147, Replace the
`jest.mock('../src/app')` hand-written `/api/invest/opportunities` handler with
a minimal Express test app that mounts the real opportunities router and uses
the production-compatible error middleware. Remove duplicated parsing, service
invocation, response shaping, and inline `{ error: ... }` handling so
`createApp` exercises the actual route and RFC 7807 error contract.

src/workers/jobPersistence.js-394-398 (1)

394-398: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Non-numeric limit produces NaN, not the default.

parseInt(rawLimit ?? LIST_JOBS_DEFAULT_LIMIT, 10) returns NaN for e.g. 'abc' or {}; Math.max(1, NaN) and Math.min(NaN, 100) are both NaN, so .limit(NaN) reaches Knex, rows.length > NaN is false (so hasMore is always false and the page is unbounded), and meta.limit serializes as null. The admin route happens to pre-validate, but listJobs is exported as a standalone API.

🐛 Proposed fix

-    const limit = Math.min(
-      Math.max(1, Number.isInteger(rawLimit) ? rawLimit : parseInt(rawLimit ?? LIST_JOBS_DEFAULT_LIMIT, 10)),
-      LIST_JOBS_MAX_LIMIT,
-    );
+    const parsedLimit = Number.isInteger(rawLimit) ? rawLimit : parseInt(rawLimit, 10);
+    const limit = Number.isFinite(parsedLimit)
+      ? Math.min(Math.max(1, parsedLimit), LIST_JOBS_MAX_LIMIT)
+      : LIST_JOBS_DEFAULT_LIMIT;

🤖 Prompt for 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.

In `@src/workers/jobPersistence.js` around lines 394 - 398, Update the limit
normalization in listJobs to detect non-numeric or non-finite parsed values and
fall back to LIST_JOBS_DEFAULT_LIMIT before applying the existing [1,
LIST_JOBS_MAX_LIMIT] clamp. Preserve valid integer handling and ensure
downstream query, hasMore, and metadata values always receive a finite limit.

src/routes/adminJobs.js-219-229 (1)

219-229: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift

Listing is not tenant-scoped despite the documented contract.

Line 8 and the Swagger block (Line 59) both state the endpoint is tenant-scoped, but listJobs accepts no tenant parameter and background_jobs is queried without any tenant predicate (see src/workers/jobPersistence.js lines 422-444). Any admin of one tenant sees every tenant's jobs. Either add a tenant filter or correct the documentation.

🔒 Sketch of the fix

     const result = await persistence.listJobs({
       limit:  rawLimit  !== undefined ? parseInt(rawLimit, 10)   : LIST_JOBS_DEFAULT_LIMIT,
       cursor: req.query.cursor,
       sortBy: rawSortBy,
       order:  rawOrder,
       status: rawStatus,
       type:   req.query.type,
+      tenantId: req.tenantId,
     });

listJobs would then need to apply .where('tenant_id', tenantId) when provided (and the column must exist on background_jobs).

🤖 Prompt for 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.

In `@src/routes/adminJobs.js` around lines 219 - 229, Make the admin job listing
tenant-scoped as documented: obtain the authenticated tenant identifier in the
route around createJobPersistence/listJobs, pass it to listJobs, and update
jobPersistence.listJobs to apply a tenant_id filter when provided. Ensure the
background_jobs schema supports tenant_id and preserve unscoped behavior only
where explicitly intended.

docs/api-keys.md-5-5 (1)

5-5: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Doc claims no key-management endpoint exists, but this PR adds one.

This document states There is no key-management HTTP endpoint (no create/list/revoke route) — keys are provisioned via an environment variable and validated at request time by middleware. However, src/routes/apiKeys.js (part of this same cohort) adds CRUD routes, and tests/unit/apiKeyAuth.test.js (Lines 390-476) exercises POST /api-keys, GET /api-keys/:key, 201/404/422 responses, and idempotent creation. As the dedicated registry-contract doc, this file should describe those endpoints instead of denying their existence.

Also applies to: 9-11

🤖 Prompt for 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.

In `@docs/api-keys.md` at line 5, Update the API key contract documentation to
describe the CRUD routes implemented by the apiKeys route module, including
create, list, and revoke/get behavior and relevant success/error responses.
Remove the claim that keys are only provisioned through an environment variable,
while preserving the middleware validation details that still apply.

src/services/storage.js-229-234 (1)

229-234: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

STORAGE_IN_MEMORY=true has no effect outside NODE_ENV=test.

uploadFile routes to the in-memory path when NODE_ENV==='test' || STORAGE_IN_MEMORY==='true' (line 230), but the callee uploadFileInMemory (line 422) only checks NODE_ENV==='test' — outside tests it falls through to the real PutObjectCommand/S3 call regardless of STORAGE_IN_MEMORY. The same gate mismatch exists in validateUploadedObject (line 355) and getFile (line 438), which also check only NODE_ENV==='test'. Net effect: the STORAGE_IN_MEMORY flag — presumably intended to let local dev run without AWS credentials — is silently a no-op outside the test environment.

Extract a single helper (e.g. _useInMemoryStore() returning NODE_ENV==='test' || STORAGE_IN_MEMORY==='true') and use it consistently in all four places.

♻️ Proposed fix

+  _useInMemoryStore() {
+    return process.env.NODE_ENV === 'test' || process.env.STORAGE_IN_MEMORY === 'true';
+  }
+
   async uploadFileInMemory({ key, body, mimeType }) {
-    if (process.env.NODE_ENV === 'test') {
+    if (this._useInMemoryStore()) {
       this._inMemoryStore.set(key, { body, mimeType });
       return;
     }
     const command = new PutObjectCommand({ Bucket: this.bucket, Key: key, Body: body, ContentType: mimeType });
     await s3Client.send(command);
   }

Apply the same substitution to validateUploadedObject and getFile.

Also applies to: 421-428

🤖 Prompt for 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.

In `@src/services/storage.js` around lines 229 - 234, Extract a shared
_useInMemoryStore() helper that returns true when NODE_ENV is test or
STORAGE_IN_MEMORY is 'true'. Use this helper consistently in uploadFile,
uploadFileInMemory, validateUploadedObject, and getFile so the in-memory path is
selected in both tests and local development.

src/services/storage.js-370-396 (1)

370-396: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Errors during the PDF magic-byte check are silently swallowed.

In the catch block (389-393), only err.code === 'INVALID_PDF_HEADER' is rethrown; any other error from GetObjectCommand/stream iteration (network failure, permission error, throttling) is dropped, and execution falls through to return { valid: ct === declaredMime && cl === declaredSize, ... } based solely on HeadObject metadata. This can report valid: true for an object whose actual PDF header was never verified because the check itself failed, defeating the purpose of the integrity check.

🐛 Proposed fix

       } catch (e) {
-        if (e.code === 'INVALID_PDF_HEADER') {
-          throw e;
-        }
+        throw e;
       }

🤖 Prompt for 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.

In `@src/services/storage.js` around lines 370 - 396, Update the PDF validation
try/catch in the storage validation method to rethrow every error from
GetObjectCommand or body stream iteration, while preserving the existing
INVALID_PDF_HEADER propagation. Do not allow execution to reach the
metadata-based return when the PDF header check fails for any reason.

src/services/storage.js-69-89 (1)

69-89: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

MAX_FILE_SIZE bypasses the validated INVOICE_FILE_MAX_SIZE config contract.

parseSize/MAX_FILE_SIZE read process.env.INVOICE_FILE_MAX_SIZE directly and silently fall back to DEFAULT_MAX_FILE_SIZE on malformed input. Meanwhile src/config/index.js's InvoiceFileMaxSizeSchema/getInvoiceFileMaxSize() (used by src/routes/invoiceFile.js) throws on the exact same malformed input. If storage.js is loaded before boot validation runs (e.g. in isolated test contexts, or if boot validation is ever bypassed), StorageService.maxFileSize can silently diverge from the value the route layer enforces/validates, with two different regexes (unit is optional here, mandatory in the Zod schema).

Use the shared getInvoiceFileMaxSize() (or export/reuse a single parser) instead of independently reading and parsing process.env here.

♻️ Proposed fix

+const { getInvoiceFileMaxSize } = require('../config');
+
 function parseSize(sizeStr) {
   if (typeof sizeStr !== 'string' || sizeStr.trim() === '') {
     return DEFAULT_MAX_FILE_SIZE;
   }
   const match = sizeStr.trim().match(/^(\d+(?:\.\d+)?)\s*(b|kb|mb|gb)?$/i);
   if (!match) {
     return DEFAULT_MAX_FILE_SIZE;
   }
   const value = parseFloat(match[1]);
   const unit = (match[2] || 'b').toLowerCase();
   const multipliers = { b: 1, kb: 1024, mb: 1024 ** 2, gb: 1024 ** 3 };
   return Math.floor(value * multipliers[unit]);
 }

-const MAX_FILE_SIZE = parseSize(process.env.INVOICE_FILE_MAX_SIZE || process.env.BODY_LIMIT_INVOICE || '5mb');
+const MAX_FILE_SIZE = parseSize(getInvoiceFileMaxSize() || process.env.BODY_LIMIT_INVOICE || '5mb');

🤖 Prompt for 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.

In `@src/services/storage.js` around lines 69 - 89, Update the MAX_FILE_SIZE
initialization in storage.js to use the shared getInvoiceFileMaxSize()
configuration accessor instead of parseSize and direct process.env reads. Remove
or stop using the independent parseSize fallback so storage and the invoice
route enforce the same validated InvoiceFileMaxSizeSchema contract.

docs/PR_DESCRIPTION_invoice_state.md-63-73 (1)

63-73: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

"No source code was modified" contradicts the actual diff scope.

This note claims the PR is documentation-only and that invoiceStateRoutes.js is "a minimal POST /transition stub," but src/services/invoiceStateMachine.js adds substantial new logic (VALID_TRANSITIONS, validateTransition, executeTransition, getTransitionHistory, canLinkToEscrow), and tests/invoice.state.test.js exercises a fully-implemented six-endpoint route set (state/transition/approve/link-escrow/reject/history) with tenant scoping and KYC gating. This note appears to be stale or copied from a different change and will mislead reviewers about the actual scope of this PR.

🤖 Prompt for 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.

In `@docs/PR_DESCRIPTION_invoice_state.md` around lines 63 - 73, The reviewer
notes inaccurately describe the change as documentation-only and the route
handler as a minimal stub. Update the “Notes for reviewers” section to reflect
the implemented invoice state machine and six-endpoint route set, including
tenant scoping and KYC gating; remove or revise the stale claims about no source
changes and an incomplete handler while preserving the
implementation-specification context where applicable.

[coderabbitai]

🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win

Admin webhook routes are unauthenticated — adminAuth is broken and never wired up.

Two compounding defects here:

1. return out; (Line 65) references an undeclared variable. This is almost certainly meant to be return authenticateToken(req, res, next); — as written, any request without an x-api-key header throws ReferenceError: out is not defined.
2. adminAuth is never applied anywhere in this file — there's no router.use(adminAuth) and it isn't passed to any of the four route handlers (/dead-letters, /replay/:id, /replay, /resolve/:id). ESLint's no-unused-vars on both authenticateToken and adminAuth corroborates this.

Combined, every admin webhook route currently runs with no authentication at all (contradicting the module's own header doc), and once wired up, the JWT fallback path would immediately crash on the out bug.

🔒 Proposed fix

 function adminAuth(req, res, next) {
   if (req.headers['x-api-key']) {
     return _adminApiKeyMiddleware(req, res, next);
   }
-  return out;
+  return authenticateToken(req, res, next);
 }
+
+router.use(adminAuth);

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	const { authenticateToken } = require('../middleware/auth');
	// Legacy src/middleware/apiKey.js has been retired in favour of the env-backed
	// registry authenticator. The implementation lives in apiKeyAuth.js and never
	// opens a SQLite connection per request — see issue #590.
	const { authenticateApiKey } = require('../middleware/apiKeyAuth');
	const logger = require('../logger');
	// ── Constants ────────────────────────────────────────────────────────────────
	/**
	* Maximum page size for the dead-letter listing endpoint.
	* @constant {number}
	*/
	const MAX_LIMIT = 100;
	/**
	* Default page size when `limit` is not supplied.
	* @constant {number}
	*/
	const DEFAULT_LIMIT = 20;
	/**
	* The cursor sort field used for dead-letter keyset pagination.
	* Dead-letters are always ordered by `created_at` descending, with `id` as
	* the tiebreaker. Because `cursorPagination.js` only allows fields in its
	* ALLOWED_SORT_FIELDS allowlist we encode the cursor with `created_at`.
	*
	* @constant {string}
	*/
	const DEAD_LETTER_SORT_FIELD = 'created_at';
	/**
	* Columns that are safe to return to the caller.
	* `webhook_url` is returned so operators know where delivery was attempted.
	* `payload` is returned (it is the event body, not a secret).
	* `webhook_secret` is NOT a column in the table — secrets live in
	* `tenants.settings` and are never persisted to `webhook_dead_letters`.
	* The `X-Signature` header stored in `last_error` might contain partial
	* signature data, but `last_error` is a plain error message string and safe
	* to surface. We explicitly exclude no columns here beyond what the table
	* holds — this comment documents the security decision.
	* Pre-built admin API key middleware (no required scope — any valid, non-revoked
	* key is accepted). Built once so the factory overhead is paid at module load
	* rather than on every request.
	*
	* @constant {string[]}
	* @type {import('express').RequestHandler}
	*/
	const SAFE_COLUMNS = [
	'id',
	'tenant_id',
	'invoice_id',
	'event',
	'webhook_url',
	'attempts',
	'last_error',
	'resolved',
	'resolved_at',
	'created_at',
	// payload is intentionally included — it is the event body, not a secret
	'payload',
	];
	// ── Apply admin auth + tenant extraction to every route ──────────────────────
	router.use(...adminStack);
	// ── Helper ───────────────────────────────────────────────────────────────────
	const _adminApiKeyMiddleware = authenticateApiKey();
	/**
	* Redacts secret material from a dead-letter row before sending it to the
	* caller. Currently the `webhook_dead_letters` table does not store HMAC
	* secrets (they live in `tenants.settings`), but as a defence-in-depth
	* measure we strip any key whose name matches the sensitive-field pattern
	* used throughout the codebase.
	* Accepts either a valid admin JWT or a valid X-API-Key.
	* Honours the existing X-API-KEY contract: when the header is present the
	* request is authenticated against the env-backed key registry; otherwise it
	* falls through to JWT auth.
	*
	* @param {Object} row - Raw DB row.
	* @returns {Object} Sanitised row safe for external consumption.
	* @type {import('express').RequestHandler}
	*/
	function redactRow(row) {
	const SENSITIVE_KEYS = /password|secret|token|apiKey|authorization|privateKey|seed|mnemonic/i;
	const out = {};
	for (const [k, v] of Object.entries(row)) {
	if (SENSITIVE_KEYS.test(k)) {
	// omit
	} else {
	out[k] = v;
	}
	function adminAuth(req, res, next) {
	if (req.headers['x-api-key']) {
	return _adminApiKeyMiddleware(req, res, next);
	}
	return out;
	}
	const { authenticateToken } = require('../middleware/auth');
	// Legacy src/middleware/apiKey.js has been retired in favour of the env-backed
	// registry authenticator. The implementation lives in apiKeyAuth.js and never
	// opens a SQLite connection per request — see issue `#590`.
	const { authenticateApiKey } = require('../middleware/apiKeyAuth');
	const logger = require('../logger');
	// ── Constants ────────────────────────────────────────────────────────────────
	/**
	* Pre-built admin API key middleware (no required scope — any valid, non-revoked
	* key is accepted). Built once so the factory overhead is paid at module load
	* rather than on every request.
	*
	* `@type` {import('express').RequestHandler}
	*/
	const _adminApiKeyMiddleware = authenticateApiKey();
	/**
	* Accepts either a valid admin JWT or a valid X-API-Key.
	* Honours the existing X-API-KEY contract: when the header is present the
	* request is authenticated against the env-backed key registry; otherwise it
	* falls through to JWT auth.
	*
	* `@type` {import('express').RequestHandler}
	*/
	function adminAuth(req, res, next) {
	if (req.headers['x-api-key']) {
	return _adminApiKeyMiddleware(req, res, next);
	}
	return authenticateToken(req, res, next);
	}
	router.use(adminAuth);

🧰 Tools

🪛 ESLint

[error] 35-35: 'authenticateToken' is assigned a value but never used. Allowed unused vars must match /^_/u.

(no-unused-vars)

---

[error] 61-61: 'adminAuth' is defined but never used. Allowed unused vars must match /^_/u.

(no-unused-vars)

---

[error] 65-65: 'out' is not defined.

(no-undef)

🤖 Prompt for 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.

In `@src/routes/adminWebhooks.js` around lines 35 - 66, Fix adminAuth by
delegating the no-X-API-Key path to authenticateToken(req, res, next) instead of
the undefined out variable, then apply adminAuth to all admin webhook routes
(/dead-letters, /replay/:id, /replay, and /resolve/:id), preferably through
router.use(adminAuth) before their definitions. Ensure every route requires
either valid API-key or JWT authentication.

Source: Linters/SAST tools

[coderabbitai]

🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift

State-mutating routes have no authentication; unauthenticated callers can drive the lifecycle.

Only extractTenant is mounted. POST /:id/transition, /approve, /reject never require req.user, and getActorFromRequest silently falls back to req.ip, so an anonymous caller with just an x-tenant-id header can approve or reject any invoice in that tenant and the audit trail records an IP instead of a principal. It also makes the documented MISSING_ACTOR 400 (docs/invoice-state.md line 724) unreachable.

Require an authenticated principal on the write routes (and ideally reject when no req.user is present rather than falling back to IP).

🔒 Suggested guard

 router.use(extractTenant);
+
+/** Rejects unauthenticated callers on state-mutating routes. */
+function requireActor(req, res, next) {
+  if (!req.user || !(req.user.id || req.user.sub)) {
+    return res.status(400).json(
+      responseHelper.error('Authenticated actor could not be resolved', 'MISSING_ACTOR'),
+    );
+  }
+  return next();
+}

Then apply requireActor (after your auth middleware) to /:id/transition, /:id/approve, /:id/reject, and /:id/link-escrow.

Also applies to: 110-119

🤖 Prompt for 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.

In `@src/routes/invoiceStateRoutes.js` around lines 31 - 47, Require an
authenticated principal on the state-mutating routes `/:id/transition`,
`/:id/approve`, `/:id/reject`, and `/:id/link-escrow` by applying `requireActor`
after the existing authentication middleware. Update `getActorFromRequest` to
reject missing `req.user` instead of falling back to the client IP, preserving
the documented `MISSING_ACTOR` response for unauthenticated requests.

[coderabbitai]

🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy lift

The new validation schema is never wired in, and the validation test suite targets a route that does not exist.

src/schemas/invoiceState.js (safeParseTransitionBody) is not imported here, so targetState/reason/actor/metadata bounds and unknown-key rejection are unenforced at the HTTP layer. Meanwhile tests/invoice.stateValidation.test.js posts to /api/invoices/transition (no :id) and asserts an RFC 7807 body with fieldErrors plus a { requiresKYC, state } success shape — none of which this router provides, so that suite cannot pass.

Wire safeParseTransitionBody into the transition handler and reconcile the test's path/response contract with the implemented routes.

🔧 Sketch for wiring validation

+const { safeParseTransitionBody } = require('../schemas/invoiceState');
@@
 router.post('/:id/transition', async (req, res, next) => {
   const { id } = req.params;
-  const { targetState, reason } = req.body || {};
-
   try {
-    if (!targetState) {
-      return res.status(400).json(
-        responseHelper.error('Target state is required', 'MISSING_TARGET_STATE'),
-      );
-    }
+    const parsed = safeParseTransitionBody(req.body);
+    if (!parsed.success) {
+      return res.status(400).json({
+        type: 'https://liquifact.io/problems/validation-error',
+        title: 'Invalid invoice-state request body',
+        status: 400,
+        code: 'INVOICE_STATE_VALIDATION_FAILED',
+        fieldErrors: parsed.fieldErrors,
+      });
+    }
+    const { targetState, reason } = parsed.data;

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	router.post('/:id/transition', async (req, res, next) => {
	const { id } = req.params;
	const { targetState, reason } = req.body || {};
	router.post('/transition', (req, res) => {
	const { targetState } = req.body;
	if (CAPITAL_MOVING_STATES.has(targetState)) {
	return res.status(200).json({ requiresKYC: true, state: targetState });
	try {
	if (!targetState) {
	return res.status(400).json(
	responseHelper.error('Target state is required', 'MISSING_TARGET_STATE'),
	);
	}
	return res.status(200).json({ requiresKYC: false, state: targetState });
	const actor = getActorFromRequest(req);
	const ipAddress = req.ip || (req.socket && req.socket.remoteAddress) || 'unknown';
	const userAgent = req.get('user-agent') || 'unknown';
	const result = await invoiceService.transitionInvoice(id, targetState, req.tenantId, {
	actor,
	reason,
	ipAddress,
	userAgent,
	metadata: {
	method: req.method,
	path: req.path,
	},
	});
	const { safeParseTransitionBody } = require('../schemas/invoiceState');
	router.post('/:id/transition', async (req, res, next) => {
	const { id } = req.params;
	try {
	const parsed = safeParseTransitionBody(req.body);
	if (!parsed.success) {
	return res.status(400).json({
	type: 'https://liquifact.io/problems/validation-error',
	title: 'Invalid invoice-state request body',
	status: 400,
	code: 'INVOICE_STATE_VALIDATION_FAILED',
	fieldErrors: parsed.fieldErrors,
	});
	}
	const { targetState, reason } = parsed.data;

🤖 Prompt for 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.

In `@src/routes/invoiceStateRoutes.js` around lines 110 - 134, Wire the invoice
transition request validation into the POST ':id/transition' handler by
importing and applying safeParseTransitionBody before calling
invoiceService.transitionInvoice, returning the established validation error
shape and rejecting unknown or out-of-bounds fields. Update
tests/invoice.stateValidation.test.js to use the implemented
'/api/invoices/:id/transition' route and assert this router’s actual success and
error response contracts.