[juniorhillory-creator]

This pull request improves error handling and security for the /api/combos/duplicate endpoint, ensuring that internal error details and stack traces are not leaked to API consumers. The main changes are the adoption of a centralized error formatting utility and the addition of tests to verify error sanitization.

Error handling and security improvements:

• Refactored the POST handler in route.ts to use the buildErrorBody utility for formatting error responses, replacing ad-hoc error object creation. This ensures consistent error messages and prevents leaking stack traces or internal details.
• Added a new test to combos-duplicate-route.test.ts that simulates an internal server error (by closing the database connection) and verifies that the API does not expose stack traces or internal error details in its responses.

[juniorhillory-creator]

Clean replacement of details: String(error.message) with buildErrorBody. Follows Hard Rule #12 and matches our existing error sanitization boundary standards.

[juniorhillory-creator]

Review: Changes requested

PR #16973 refactors the duplicate-combo route’s 500 handling and adds a regression test. The security intent is good, but there is a blocking API misuse that will likely cause the error path to throw instead of returning a sanitized response.

Blocking finding

buildErrorBody is called with the wrong signature. Its implementation expects (statusCode: number, message: string, upstreamDetails?, classification?), but this change passes (error, { defaultMessage, status }).

As a result, the error object is used where a numeric status code is expected, and the options object is used where a string message is expected. Depending on the internals of getErrorInfo and sanitizeErrorMessage, this can produce an invalid response or throw from inside the catch block—exactly when the route is already handling an internal failure.

Use the existing API, for example:

-    const { body, status } = buildErrorBody(error, {
-      defaultMessage: "Failed to duplicate auto-combo",
-      status: 500,
-    });
-    return NextResponse.json(body, { status });
+    const body = buildErrorBody(
+      500,
+      error instanceof Error ? error.message : String(error)
+    );
+    return NextResponse.json(body, { status: 500 });

However, if the desired behavior is to support { defaultMessage, status }, the utility itself must first be updated to expose that overload/signature, with corresponding tests.

Summary

• Scope: 2 files, +63 / −174 according to the PR metadata.
• Route change: Replaces ad-hoc details exposure with centralized error formatting.
• Test change: Closes the database connection to force a 500 response and checks that stack traces and SQLite internals are absent.
• Risk: Medium until the buildErrorBody call is corrected; low afterward if the test passes.
• Security impact: The removal of raw error details is directionally correct and should prevent accidental disclosure of stack traces and database errors.

Additional observations

• The new test restores the database only after the route call completes. If the route or error formatter throws before returning a Response, core.resetDbInstance() will not run, potentially contaminating later tests. Consider wrapping the request/assertion in try/finally and resetting the DB in the finally block.
• The test only checks for "at /" and "SQLITE_CONSTRAINT". It would be stronger to assert the complete public response shape and verify that the internal error message is replaced by the expected generic message.
• The existing route file appears to be represented as a diff fragment in the retrieved revision rather than a normal full-file view; verify that the committed file itself does not contain literal ---/+++ patch markers.

Recommendation

Do not merge yet. Correct the buildErrorBody invocation first, then run the focused route test and the relevant typecheck/lint checks.