shell OOM

[sky22333]

Summary by CodeRabbit

• Bug Fixes

  • Improved error handling for script file processing—now returns appropriate error responses on failure instead of attempting fallback processing.

• Improvements

  • Added 10 MB size limit for shell file processing to protect against excessively large files.
  • Enhanced compressed content handling with improved gzip support for better compatibility.

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

[coderabbitai]

📝 Walkthrough

Walkthrough

The changes modify error handling in the GitHub proxy flow for shell scripts—when smart processing fails, the handler now returns a Bad Gateway error instead of falling back to original content. Additionally, shell processing utilities are refactored to operate on byte slices, accept io.Reader instead of io.ReadCloser, introduce a 10 MB size limit, and improve error propagation.

Changes

Cohort / File(s)	Summary
Error Handling & Early Termination src/handlers/github.go	Modifies proxy flow to return Bad Gateway with formatted error on smart processing failure instead of falling back to original response body
Shell Processing Refactoring src/utils/proxy_shell.go	Adds exported constant MaxShellSize (10 MB); changes ProcessSmart and readShellContent signatures to accept io.Reader instead of io.ReadCloser; refactors content handling to operate on byte slices; introduces size-limiting logic; converts content type checks from string-based to bytes.Contains; enhances gzip decompression handling

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Poem

🐰 A shell-bound rabbit hops with glee,
Ten megabytes is plenty, don't you see?
With bytes instead of strings, we read and flow,
When smart bits stumble, errors steal the show,
No fallback tricks—just truth, direct and clear!

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

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.
Title check	❓ Inconclusive	The title 'shell OOM' is vague and does not clearly convey the main changes: adding a size limit (MaxShellSize) to prevent out-of-memory issues and refactoring error handling in shell processing.	Consider a more descriptive title such as 'Add MaxShellSize limit for shell script processing' or 'Prevent OOM by limiting shell content size' that better explains the specific changes made.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

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

✨ Finishing touches

• 📝 Generate docstrings

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

src/handlers/github.go (1)

206-208: Consider logging the copy error.

When io.Copy fails, the error is silently discarded. While returning early is appropriate, logging the error would help with debugging and observability.

♻️ Suggested improvement

 // 输出处理后的内容
 if _, err := io.Copy(c.Writer, processedBody); err != nil {
+	fmt.Printf("复制处理后内容失败: %v\n", err)
 	return
 }

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c6d95e6 and 685388f.

📒 Files selected for processing (2)

• src/handlers/github.go
• src/utils/proxy_shell.go

🔇 Additional comments (4)

src/utils/proxy_shell.go (4)

15-16: LGTM: Size limit addresses the OOM issue.

The 10 MB limit is a reasonable safeguard against out-of-memory conditions when processing shell scripts. This directly addresses the PR objective ("shell OOM").

---

19-35: Improved signature and efficiency.

The changes improve the function in several ways:

• Accepting io.Reader instead of io.ReadCloser is cleaner; the caller handles resource cleanup
• Byte-based content checks (line 29) are more efficient than string-based checks
• Direct error propagation (line 22) provides clearer error semantics

The function correctly handles the common case (non-GitHub content) by early-returning with the original bytes.

---

61-71: Size limiting implementation is correct.

The size limiting logic is well-implemented:

1. Line 61-62: Read up to MaxShellSize + 1 bytes using LimitReader
2. Line 64-67: Read all available data within the limit
3. Line 69-71: Check if data exceeds MaxShellSize

This approach correctly detects oversized content without reading the entire file into memory, which effectively prevents the OOM issue mentioned in the PR title.

---

41-59: Compression handling is robust.

The peek-and-restore pattern correctly handles both gzip-compressed and uncompressed content:

• Lines 42-46: Safely peek at the first 2 bytes to detect gzip magic numbers
• Lines 48-55: Create a gzip reader when compression is detected
• Line 49: Use io.MultiReader to restore peeked bytes, ensuring no data is lost
• Line 54: Properly defer cleanup of the gzip reader
• Line 57: Fallback handles the case where isCompressed is true but content isn't actually gzipped

The implementation handles edge cases well (e.g., fewer than 2 bytes available, EOF during peek).

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

cat -n src/handlers/github.go | sed -n '160,190p'

Repository: sky22333/hubproxy

Length of output: 1228

---

🏁 Script executed:

# Find the ProcessSmart function definition
rg -n "ProcessSmart" src/

Repository: sky22333/hubproxy

Length of output: 365

---

🏁 Script executed:

# Check the file size first to see if we need to read selectively
wc -l src/handlers/github.go

Repository: sky22333/hubproxy

Length of output: 87

---

🏁 Script executed:

cat -n src/utils/proxy_shell.go

Repository: sky22333/hubproxy

Length of output: 3571

---

🏁 Script executed:

# Check if there's a git log showing the change
git log --oneline -20 src/handlers/github.go 2>/dev/null || echo "Git history not available"

Repository: sky22333/hubproxy

Length of output: 78

---

🏁 Script executed:

# Look for any fallback patterns or conditional error handling in github.go
cat -n src/handlers/github.go | sed -n '165,210p'

Repository: sky22333/hubproxy

Length of output: 1526

---

🏁 Script executed:

# Check if there's a fallback behavior in the else branch (for non-.sh/.ps1 files)
cat -n src/handlers/github.go | sed -n '209,232p'

Repository: sky22333/hubproxy

Length of output: 680

---

🏁 Script executed:

# Look for any recent changes or comments about previous behavior
git show HEAD src/handlers/github.go 2>/dev/null | head -100 || echo "Git show not available"

Repository: sky22333/hubproxy

Length of output: 3210

---

Consider differentiating error handling by error type.

The error handling treats all ProcessSmart errors uniformly (returning 502). Since different failures have different implications—size limit exceeded (the OOM prevention goal) versus transient decompression or read errors—consider whether transient errors warrant different treatment such as retry logic or fallback behavior, while size-limit errors appropriately trigger 502.