Skip to content

Conversation

@LearningCircuit
Copy link
Owner

Summary

  • Fixes the "Argument list too long" error (exit code 126) that occurs when processing large PR diffs
  • Changes API call to pipe JSON payload through stdin instead of command-line argument
  • Resolves issue preventing AI review workflow from completing on large PRs

Changes

  • Build JSON payload using jq -n with proper argument passing (--arg and --argjson)
  • Pipe payload to curl using --data-binary @- instead of -d flag
  • Avoids system argument size limit (~2MB on Linux)

Test Plan

  • Test with small PR (existing functionality should work)
  • Test with large PR that previously failed with "Argument list too long"
  • Verify AI review completes successfully

Changed the API call to OpenRouter to pipe JSON payload through stdin
instead of passing it as a command-line argument. This avoids hitting
the system's argument size limit when processing large PR diffs.

- Build JSON payload using jq with proper argument passing
- Pipe payload to curl using --data-binary @- instead of -d flag
- Fixes issue where PRs with large diffs failed with exit code 126
@LearningCircuit LearningCircuit added the ai_code_review Friendly AI Code Review label Nov 12, 2025
@github-actions
Copy link

AI Code Review

This is a solid fix that correctly addresses the argument list length limitation by piping JSON through stdin. The use of jq with --arg and --argjson improves security and reliability over manual string concatenation.


🔒 Security

  • Improved: Using jq --arg and --argjson eliminates potential shell injection vulnerabilities from manual JSON string construction
  • No security concerns introduced

⚡ Performance

  • Significantly improved: Piping large payloads via stdin avoids memory overhead of massive command-line strings and bypasses OS argument size limits (~2MB on Linux)
  • More efficient handling of large diffs

🐛 Code Quality & Best Practices

Critical Issues:

  • Missing error handling: No check for jq command success. If jq fails (e.g., invalid --argjson values), JSON_PAYLOAD will be empty or contain error text, which then gets sent to the API
    • Fix: Add set -e at script top or explicitly check jq exit code: if ! JSON_PAYLOAD=$(jq ...); then echo "Failed to build JSON payload"; exit 1; fi
  • Cannot verify defaults: If AI_TEMPERATURE or AI_MAX_TOKENS are empty or non-numeric, jq --argjson will fail. Please confirm these variables have validated defaults

Improvements Made:

  • ✅ Proper JSON construction using jq instead of manual escaping
  • ✅ Eliminates brittle $(echo "$PROMPT" | jq -Rs .) pattern
  • --data-binary @- preserves payload integrity including newlines and special characters

Recommendations:

  • Add set -euo pipefail for robust error handling (if not already present)
  • Validate AI_TEMPERATURE and AI_MAX_TOKENS are numeric before jq call
  • Consider adding a payload size warning log for debugging: echo "Sending $(echo "$JSON_PAYLOAD" | wc -c) bytes to API"

Test Plan Assessment

  • ✅ Tests cover both small and large PR scenarios
  • ⚠️ Missing test for jq failure scenario
  • ⚠️ Missing test for invalid temperature/max_tokens values

Verdict: ✅ Approved with recommendations

The fix correctly solves the stated problem. Add error handling for jq before merging to prevent silent failures.


Review by Friendly AI Reviewer - made with ❤️

@github-actions github-actions bot added enhancement New feature or request bug Something isn't working and removed ai_code_review Friendly AI Code Review labels Nov 12, 2025
@LearningCircuit LearningCircuit merged commit d03e896 into main Nov 12, 2025
1 check passed
@LearningCircuit LearningCircuit deleted the fix/curl-argument-list-too-long branch November 12, 2025 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants