Skip to content

Conversation

@jonathanpopham
Copy link
Collaborator

@jonathanpopham jonathanpopham commented Jan 15, 2026

Summary

  • Extract and display error details from API response body instead of just status codes
  • Add specific guidance for common errors:
    • Nested archives: Suggest using .gitattributes export-ignore
    • Size limits: Suggest excluding large files
    • Rate limits: Suggest waiting before retry
  • Handle 401, 413, 429, and 500 status codes with helpful messages

Context

This addresses the issue where users see generic "API error (500)" without understanding the root cause. The most common cause is nested archive files (.zip, .tar, etc.) triggering the API's zip bomb protection.

Test plan

  • Verify build succeeds
  • Test with a repo containing nested archives to verify error message
  • Test with a valid repo to ensure normal flow works

Summary by CodeRabbit

  • Bug Fixes
    • Improved error reporting with richer, status-specific messages and clear guidance for authentication failures, server errors, rate limits, and oversized archives.
    • Enhanced logging and diagnostics with safer serialization and redaction of sensitive values to provide clearer troubleshooting information without exposing secrets.

✏️ Tip: You can customize this high-level summary in your review settings.

- Extract and display error details from API response body
- Add specific guidance for common errors:
  - Nested archives: suggest using .gitattributes export-ignore
  - Size limits: suggest excluding large files
  - Rate limits: suggest waiting before retry
- Handle 401, 413, 429, and 500 status codes with helpful messages
@coderabbitai
Copy link

coderabbitai bot commented Jan 15, 2026

Walkthrough

Updated src/index.ts to add log-safe serialization and redaction, extract status and apiMessage from multiple error shapes, branch error handling by status (401, 500, 413, 429, other), emit structured debug logs, and call core.setFailed() with unified error/help text.

Changes

Cohort / File(s) Summary
Error handling & logging
src/index.ts
Added SENSITIVE_KEYS, MAX_VALUE_LENGTH, safeSerialize, and redactSensitive; enhanced catch flow to compute status from multiple fields, extract apiMessage from varied shapes, branch messages by status (401/500/413/429/other), emit structured debug logs, and call core.setFailed(errorMessage) with optional helpText.

Sequence Diagram(s)

sequenceDiagram
  participant Action as Action
  participant API as External API
  participant Handler as Error Handler (src/index.ts)
  participant Logger as GitHub Actions core

  Action->>API: make request
  API-->>Action: responds with error (status, body, headers)
  Action->>Handler: catch error
  Handler->>Handler: safeSerialize & redactSensitive(error/response)
  Handler->>Handler: determine status & apiMessage from multiple fields
  Handler->>Logger: core.debug/core.info structured logs (message, name, status, body, headers)
  Handler->>Logger: core.setFailed(errorMessage + optional helpText)
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🛠️ Logs trimmed, secrets tucked away,
Errors parsed so they clearly say,
Status found from many a place,
Helpful hints put in their place,
Fail with guidance — ship the day.

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title 'feat: Improve error messages with actionable guidance' accurately summarizes the main change—enhanced error handling with user-friendly guidance for common API errors.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

🧹 Recent nitpick comments
src/index.ts (2)

208-231: Consider using core.debug for response data too.

You've got headers going to core.debug (nice!), but error.response.data and error.body are still going to core.info on lines 212 and 217.

Here's the thing: API error responses sometimes echo back parts of the request or include details you might not want visible in CI logs (think: partial tokens in error messages, user identifiers, etc.).

-        core.info(`Response data: ${safeSerialize(error.response.data)}`);
+        core.debug(`Response data: ${safeSerialize(error.response.data)}`);
-        core.info(`Error body: ${safeSerialize(error.body)}`);
+        core.debug(`Error body: ${safeSerialize(error.body)}`);

This way, the detailed diagnostics are available when someone enables debug logging, but they won't show up by default in public CI logs.


264-271: String matching on API messages is a bit fragile.

This works today, but if the API changes its error message from "Nested archives" to "nested archive" or "Nested archive files", the helpful guidance won't trigger.

Not a big deal for now - just something to keep in mind. If you find users missing helpful tips, you might want to make these checks more flexible:

// Example: case-insensitive partial match
if (apiMessage.toLowerCase().includes('nested archive'))

Totally optional though - shipping working code > perfect code. 🚀


📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5eecf6f and 97e38c1.

⛔ Files ignored due to path filters (1)
  • dist/index.js is excluded by !**/dist/**
📒 Files selected for processing (1)
  • src/index.ts
🔇 Additional comments (6)
src/index.ts (6)

9-22: Nice work on the sensitive keys list!

This is a solid starting point for redaction. The Set gives you fast lookups, and you've covered the most common culprits like authorization, cookie, and various token fields.

One small thought: you might consider adding a few more fields that sometimes sneak through in error objects, like client_secret, private_key, or credentials. Not critical now, but worth keeping in mind if you see leaks in the future.


24-24: Reasonable truncation limit.

1000 chars is a good balance - enough to see what went wrong without flooding CI logs with massive JSON blobs.


30-65: Solid defensive serialization!

This function handles the tricky edge cases really well:

  • Circular refs → caught with WeakSet
  • BigInt → converted to string (since JSON.stringify chokes on BigInt by default)
  • Sensitive data → redacted inline
  • Huge payloads → truncated

One tiny thing to know: the WeakSet approach will mark any repeated object reference as [Circular], even if it's just a shared reference rather than a true cycle. For error logging purposes this is totally fine - you still get the data the first time it appears.


70-84: Good shallow redaction for headers.

This works well for HTTP headers which are typically flat key-value pairs. The fact that safeSerialize also redacts during serialization means you've got double coverage - belt and suspenders approach. 👍


240-255: Nice defensive message extraction!

I like how you're checking multiple locations for the error message - different HTTP clients structure errors differently (axios vs fetch vs SDK wrappers), so this covers your bases.

One thing to keep in mind: line 245's fallback to error?.response?.data raw could sometimes be an HTML error page (servers love returning HTML on 500s). The truncation helps, but users might occasionally see something like <!DOCTYPE html>.... Not a bug, just the nature of the beast. 🤷


284-289: Clean error output pattern!

I like that core.setFailed() gets just the error message while the help text goes separately to core.error(). Users see a clean failure reason, but the guidance is right there in the logs when they expand them.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@src/index.ts`:
- Around line 123-148: The current error-logging block uses core.info and direct
JSON.stringify calls that can leak secrets (Authorization/Cookie) and throw on
circular/BigInt values; change logging to use core.debug for sensitive details,
replace direct JSON.stringify calls with a safeSerialize helper (that catches
stringify errors and returns a deterministic placeholder like "[unserializable]"
or truncates large values), and redact sensitive header/body fields (e.g.,
Authorization, Cookie, set-cookie, access_token) before serializing; ensure all
uses in this block (references: core.info, JSON.stringify, error,
error.response, error.response.headers, error.response.data, error.body,
error.cause) are wrapped so they never throw and never log raw secret values.
🧹 Nitpick comments (1)
src/index.ts (1)

178-187: Make the 500‑error heuristics case‑insensitive.

API messages can vary in casing; lowercasing once avoids missing matches.

♻️ Small tweak
-    } else if (status === 500) {
+    } else if (status === 500) {
       errorMessage = apiMessage || 'Internal server error';
 
       // Check for common issues and provide guidance
-      if (apiMessage.includes('Nested archives')) {
+      const msg = apiMessage.toLowerCase();
+      if (msg.includes('nested archives')) {
         helpText = 'Your repository contains nested archive files (.zip, .tar, etc.). ' +
           'Add them to .gitattributes with "export-ignore" to exclude from analysis. ' +
           'Example: tests/fixtures/*.zip export-ignore';
-      } else if (apiMessage.includes('exceeds maximum')) {
+      } else if (msg.includes('exceeds maximum')) {
         helpText = 'Your repository or a file within it exceeds size limits. ' +
           'Consider excluding large files using .gitattributes with "export-ignore".';
       }
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b19aae4 and 5eecf6f.

⛔ Files ignored due to path filters (1)
  • dist/index.js is excluded by !**/dist/**
📒 Files selected for processing (1)
  • src/index.ts
🔇 Additional comments (1)
src/index.ts (1)

150-199: Nice, resilient message extraction chain.

The multi-source apiMessage fallback plus status-based messaging is clear and should make errors much more actionable.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.

- Add safeSerialize helper to handle circular refs, BigInt, and truncate large values
- Add redactSensitive helper to redact Authorization, Cookie, token fields
- Use core.debug for sensitive details (headers, cause)
- Use core.info for safe error summaries (status, message, data)
- Wrap all serialization in try/catch to prevent throws
- Add SENSITIVE_KEYS set for consistent redaction
@jonathanpopham jonathanpopham merged commit 40c85ec into main Jan 15, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants