Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve GitHub resolver robustness and efficiency #5462

Closed
wants to merge 10 commits into from

Conversation

neubig
Copy link
Contributor

@neubig neubig commented Dec 8, 2024

This PR improves the GitHub resolver by:

  1. Making API response handling more robust:

    • Handle both list and single-object responses from GitHub API
    • Add proper error handling for comment fetching
  2. Optimizing PR handling:

    • Fetch PRs directly by number instead of filtering from all issues
    • Add error handling for individual PR fetches
  3. Adding comprehensive test coverage:

    • Test single-object API responses
    • Test error handling in comment fetching
    • Test PR metadata handling
    • Test duplicate issue references
    • Test review thread comments

Note: This PR was originally titled as fixing #5456, but upon review, these changes do not directly address that issue. A separate PR will be created to fix the issue of incorrectly detected issue references.


To run this PR locally, use the following command:

docker run -it --rm   -p 3000:3000   -v /var/run/docker.sock:/var/run/docker.sock   --add-host host.docker.internal:host-gateway   -e SANDBOX_RUNTIME_CONTAINER_IMAGE=docker.all-hands.dev/all-hands-ai/runtime:b025507-nikolaik   --name openhands-app-b025507   docker.all-hands.dev/all-hands-ai/openhands:b025507

@neubig neubig changed the title Fix issue #5456: Keep only resolver changes Fix issue #5456: [Bug]: Too many detected issues in github resolver Dec 8, 2024
@neubig neubig marked this pull request as draft December 8, 2024 03:20
@neubig neubig changed the title Fix issue #5456: [Bug]: Too many detected issues in github resolver Improve GitHub resolver robustness and efficiency Dec 9, 2024
Copy link
Contributor Author

@neubig neubig left a comment

Choose a reason for hiding this comment

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

I think this looks OK to me now

@neubig neubig marked this pull request as ready for review December 9, 2024 21:07
@neubig neubig self-assigned this Dec 9, 2024
@neubig neubig requested a review from malhotra5 December 9, 2024 21:29
@neubig neubig assigned malhotra5 and unassigned neubig Dec 9, 2024
issue = response.json()

# Handle both list and single-object responses
if isinstance(issue, list):
Copy link
Contributor

@malhotra5 malhotra5 Dec 9, 2024

Choose a reason for hiding this comment

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

This section of code that checks for list and dict instance seems to be duplicated from IssueHandler

Ideally we share this as a function across the handlers

Copy link
Contributor

Choose a reason for hiding this comment

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

@openhands-agent please take a look at this

Copy link
Contributor

Choose a reason for hiding this comment

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

Openhands fix success summary

The feedback specifically asks to address code duplication between handlers regarding the list and dict instance checking logic, suggesting this should be shared as a common function. However, the AI's response focuses entirely on changes made to issue reference extraction logic and testing, which while valuable, does not address the specific feedback about reducing code duplication.

The AI should have:

  1. Identified the duplicated list/dict instance checking code in the handlers
  2. Created a shared utility function that both handlers could use
  3. Refactored the handlers to use this shared function
  4. Verified the changes maintain the same functionality

Instead, the response discusses unrelated changes to issue reference extraction. The feedback remains unaddressed and should be revisited to properly handle the code duplication concern.

@neubig neubig assigned neubig and unassigned malhotra5 Dec 28, 2024
@neubig neubig added the fix-me Attempt to fix this issue with OpenHands label Dec 28, 2024
@openhands-agent
Copy link
Contributor

OpenHands started fixing the pr! You can monitor the progress here.

@openhands-agent
Copy link
Contributor

Status Overview:

❌ Not Resolved: The core issue of code duplication in list/dict instance checking logic between handlers remains unaddressed.

Key Points:

  • Original feedback requested consolidation of duplicated validation logic
  • Response focused on unrelated issue reference extraction changes
  • No shared utility function was created
  • Handlers still contain duplicated code

Next Steps Required:

  1. Create shared utility function for list/dict validation
  2. Refactor handlers to use the shared function
  3. Verify functionality remains intact

@neubig
Copy link
Contributor Author

neubig commented Dec 29, 2024

I'm actually not sure if this is a problem before, maybe we can close this but I'd be happy to re-open if necessary.

@neubig neubig closed this Dec 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix-me Attempt to fix this issue with OpenHands
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants