Skip to content

Comments

fix: capture async stack only for rejections with native error object…#5

Open
MitchLewis930 wants to merge 1 commit intopr_025_beforefrom
pr_025_after
Open

fix: capture async stack only for rejections with native error object…#5
MitchLewis930 wants to merge 1 commit intopr_025_beforefrom
pr_025_after

Conversation

@MitchLewis930
Copy link

@MitchLewis930 MitchLewis930 commented Jan 30, 2026

User description

PR_025


PR Type

Bug fix


Description

  • Capture async stack traces only for native Error objects

  • Refactor stack trace capture logic with improved null safety

  • Simplify error handling with ternary operators

  • Update test to use async/await pattern with proper server setup


Diagram Walkthrough

flowchart LR
  A["Error caught in request"] --> B{"Is native Error?"}
  B -->|Yes| C["Capture stack trace"]
  C --> D["Append async stack"]
  B -->|No| E["Skip stack processing"]
  D --> F["Throw error"]
  E --> F
Loading

File Walkthrough

Relevant files
Bug fix
Axios.js
Add Error type check for stack trace capture                         

lib/core/Axios.js

  • Add type check to only process native Error objects with instanceof
    Error
  • Refactor stack trace capture using ternary operator for cleaner code
  • Improve null safety by checking if err.stack and dummy.stack exist
    before processing
  • Simplify logic with early assignment in ternary expression
+14/-11 
Tests
http.js
Refactor HTTP error stack trace test to async/await           

test/unit/adapters/http.js

  • Convert test function to async/await pattern for better readability
  • Replace callback-based server creation with startHTTPServer helper
  • Simplify assertion flow by removing nested callbacks and using return
    statement
  • Improve test structure with proper async/await handling
+16/-15 

@qodo-code-review
Copy link

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status:
Non-descriptive variable: The newly introduced variable dummy is a generic name that makes the stack-capture intent
less self-documenting without reading surrounding comments.

Referred Code
let dummy;

Error.captureStackTrace ? Error.captureStackTrace(dummy = {}) : (dummy = new Error());

// slice off the Error: ... line
const stack = dummy.stack ? dummy.stack.replace(/^.+\n/, '') : '';

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status:
Stack trace exposure: The change appends an async stack trace to err.stack, which could be returned to callers
and potentially exposed to end-users depending on how the library consumer surfaces
errors.

Referred Code
if (err instanceof Error) {
  let dummy;

  Error.captureStackTrace ? Error.captureStackTrace(dummy = {}) : (dummy = new Error());

  // slice off the Error: ... line
  const stack = dummy.stack ? dummy.stack.replace(/^.+\n/, '') : '';

  if (!err.stack) {
    err.stack = stack;
    // match without the 2 top stack lines
  } else if (stack && !String(err.stack).endsWith(stack.replace(/^.+\n.+\n/, ''))) {
    err.stack += '\n' + stack
  }
}

throw err;

Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Consider a more robust stack trace solution

The suggestion highlights the fragility of manipulating stack traces with string
operations due to non-standard formats across JavaScript engines. It recommends
exploring more robust alternatives for async stack tracing.

Examples:

lib/core/Axios.js [42-56]
      if (err instanceof Error) {
        let dummy;

        Error.captureStackTrace ? Error.captureStackTrace(dummy = {}) : (dummy = new Error());

        // slice off the Error: ... line
        const stack = dummy.stack ? dummy.stack.replace(/^.+\n/, '') : '';

        if (!err.stack) {
          err.stack = stack;

 ... (clipped 5 lines)

Solution Walkthrough:

Before:

try {
  // ...
} catch (err) {
  if (err instanceof Error) {
    let dummy;
    Error.captureStackTrace ? Error.captureStackTrace(dummy = {}) : (dummy = new Error());

    // Manipulate stack strings to find the async portion
    const stack = dummy.stack ? dummy.stack.replace(/^.+\n/, '') : '';
    
    // Append the async stack if it's not already present using string matching
    if (stack && !String(err.stack).endsWith(stack.replace(/^.+\n.+\n/, ''))) {
      err.stack += '\n' + stack;
    }
  }
  throw err;
}

After:

import { getAsyncStackTrace } from 'some-robust-stack-tracing-library';

try {
  // ...
} catch (err) {
  if (err instanceof Error) {
    // Use a robust, non-string-parsing method to get the async stack trace
    const asyncStack = getAsyncStackTrace();
    
    // Append it to the original error's stack if needed
    if (asyncStack && !isStackAlreadyAppended(err.stack, asyncStack)) {
        err.stack += '\n' + asyncStack;
    }
  }
  throw err;
}
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that manipulating stack traces via string operations is a fragile design, and while the PR adds safety checks, it doesn't address this underlying architectural weakness.

Medium
Possible issue
Preserve error message on missing stack

When err.stack is undefined, prefix the new stack with err.toString() to
preserve the original error message line.

lib/core/Axios.js [45-55]

-Error.captureStackTrace ? Error.captureStackTrace(dummy = {}) : (dummy = new Error());
-
-// slice off the Error: ... line
 const stack = dummy.stack ? dummy.stack.replace(/^.+\n/, '') : '';
-
 if (!err.stack) {
-  err.stack = stack;
+  err.stack = `${err.toString()}\n${stack}`;
 } else if (stack && !String(err.stack).endsWith(stack.replace(/^.+\n.+\n/, ''))) {
-  err.stack += '\n' + stack
+  err.stack += '\n' + stack;
 }
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that if err.stack is missing, the error message (e.g., 'Error: Request failed with status code 404') is lost. Prepending err.toString() restores this important information, improving error reporting.

Medium
General
Simplify dummy stack initialization

Replace the ternary operator used for dummy stack initialization with a clearer
if/else block to improve code readability.

lib/core/Axios.js [43-45]

-let dummy;
+const dummy = {};
+if (Error.captureStackTrace) {
+  Error.captureStackTrace(dummy);
+} else {
+  dummy.stack = new Error().stack;
+}
 
-Error.captureStackTrace ? Error.captureStackTrace(dummy = {}) : (dummy = new Error());
-
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly points out that the ternary with side-effect assignments is hard to read and replaces it with a standard if/else block, which significantly improves code clarity and maintainability.

Low
  • More

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants