Include stack traces in catchSpectralRunErrors for improved debugging#845
Include stack traces in catchSpectralRunErrors for improved debugging#845Copilot wants to merge 8 commits into
Conversation
Agent-Logs-Url: https://github.com/Azure/azure-openapi-validator/sessions/c4858aed-3151-47c9-8dfd-dbcde93b9efc Co-authored-by: mikeharder <9459391+mikeharder@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR enhances error reporting in the AutoRest Spectral integration by adding stack traces to the fatal message emitted from catchSpectralRunErrors, improving debuggability when Spectral validation fails.
Changes:
- Append the top-level caught error’s stack trace to the aggregated error output (when present).
- Append stack traces for constituent errors when the caught error is an
AggregateError-like object. - Adjust error-message aggregation logic used to build the fatal output.
| // Initialize an array to collect error messages | ||
| const errorMessages: string[] = [error] | ||
|
|
||
| if (error && error.stack) { | ||
| errorMessages.push(`Stack:\n${error.stack}`) | ||
| } | ||
|
|
There was a problem hiding this comment.
errorMessages is declared as string[] but initialized with the raw caught error value. Since catch can receive non-Error values (and callers pass unknown), this can produce unhelpful output like [object Object] and makes the array content inconsistent. Consider stringifying the top-level error explicitly (e.g., prefer error.message/String(error)), then optionally append stack traces.
| error.errors.forEach((error: any, index: number) => { | ||
| // Push each error message into the array | ||
| errorMessages.push(`Error ${index + 1}: ${error.message}`) | ||
| if (error.stack) { | ||
| errorMessages.push(`Stack ${index + 1}:\n${error.stack}`) |
There was a problem hiding this comment.
The inner forEach callback parameter is still named error, which shadows the outer error parameter and contradicts the intent described in the PR. Rename the inner variable (e.g., innerError) to avoid confusion and make it clear which error’s stack/message is being logged.
| error.errors.forEach((error: any, index: number) => { | |
| // Push each error message into the array | |
| errorMessages.push(`Error ${index + 1}: ${error.message}`) | |
| if (error.stack) { | |
| errorMessages.push(`Stack ${index + 1}:\n${error.stack}`) | |
| error.errors.forEach((innerError: any, index: number) => { | |
| // Push each error message into the array | |
| errorMessages.push(`Error ${index + 1}: ${innerError.message}`) | |
| if (innerError.stack) { | |
| errorMessages.push(`Stack ${index + 1}:\n${innerError.stack}`) |
| const errorMessages: string[] = [error] | ||
|
|
||
| if (error && error.stack) { | ||
| errorMessages.push(`Stack:\n${error.stack}`) | ||
| } | ||
|
|
||
| // Check if "error" contains the "errors" property | ||
| if (error && error.errors && Array.isArray(error.errors)) { | ||
| error.errors.forEach((error: any, index: number) => { | ||
| // Push each error message into the array | ||
| errorMessages.push(`Error ${index + 1}: ${error.message}`) | ||
| if (error.stack) { | ||
| errorMessages.push(`Stack ${index + 1}:\n${error.stack}`) |
There was a problem hiding this comment.
When formatting constituent AggregateError entries, the code assumes each item has message (and now stack). Since AggregateError.errors can contain arbitrary values, error.message may be undefined and degrade the output. Consider falling back to String(error) when message is missing, and only include a stack trace when it’s a string.
| const errorMessages: string[] = [error] | |
| if (error && error.stack) { | |
| errorMessages.push(`Stack:\n${error.stack}`) | |
| } | |
| // Check if "error" contains the "errors" property | |
| if (error && error.errors && Array.isArray(error.errors)) { | |
| error.errors.forEach((error: any, index: number) => { | |
| // Push each error message into the array | |
| errorMessages.push(`Error ${index + 1}: ${error.message}`) | |
| if (error.stack) { | |
| errorMessages.push(`Stack ${index + 1}:\n${error.stack}`) | |
| const errorMessages: string[] = [] | |
| // Top-level error message (fall back to String(error) if no .message) | |
| const topLevelMessage = | |
| error && typeof error === "object" && typeof (error as any).message === "string" | |
| ? (error as any).message | |
| : String(error) | |
| errorMessages.push(topLevelMessage) | |
| // Top-level stack trace (include only if it's a string) | |
| if (error && typeof (error as any).stack === "string") { | |
| errorMessages.push(`Stack:\n${(error as any).stack}`) | |
| } | |
| // Check if "error" contains the "errors" property (AggregateError) | |
| if (error && Array.isArray((error as any).errors)) { | |
| ;(error as any).errors.forEach((innerError: any, index: number) => { | |
| // Derive each constituent error message with a safe fallback | |
| const innerMessage = | |
| innerError && typeof innerError === "object" && typeof innerError.message === "string" | |
| ? innerError.message | |
| : String(innerError) | |
| errorMessages.push(`Error ${index + 1}: ${innerMessage}`) | |
| // Include stack trace only when it's a string | |
| if (innerError && typeof innerError.stack === "string") { | |
| errorMessages.push(`Stack ${index + 1}:\n${innerError.stack}`) |
|
stack traces were less useful than expected |
Summary
Updates
catchSpectralRunErrorsinspectral-plugin-utils.tsto include stack traces from caught errors. Previously only error messages were logged, which hindered debugging when Spectral validation failures occurred.Changes
error.stack)AggregateError(innerError.stack)errortoinnerErrorto avoid shadowing the outer parameterSecurity Summary
No security vulnerabilities were introduced or discovered. CodeQL analysis returned zero alerts.