[fix] Add error handling to get_top_hackernews_stories#7343
Open
lawrence3699 wants to merge 1 commit intoagno-agi:mainfrom
Open
[fix] Add error handling to get_top_hackernews_stories#7343lawrence3699 wants to merge 1 commit intoagno-agi:mainfrom
lawrence3699 wants to merge 1 commit intoagno-agi:mainfrom
Conversation
get_user_details wraps its logic in try/except and returns a
descriptive error string when the HN API is unreachable, but
get_top_hackernews_stories has no error handling at all. This causes
two issues:
1. If the Firebase API returns an error or is unreachable, the method
raises an unhandled exception instead of returning an error message.
2. story["by"] crashes with TypeError when the API returns null for
deleted/dead items, and with KeyError if the "by" field is absent
(e.g., job postings).
Add try/except matching the pattern in get_user_details, skip null
stories, and use .get("by", "unknown") for the username field.
Contributor
PR TriageMissing issue link: Please link the issue this PR addresses using |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds robust error handling to get_top_hackernews_stories to prevent unhandled exceptions when the Hacker News Firebase API fails or returns unexpected/null story payloads.
Changes:
- Wrap
get_top_hackernews_storiesintry/exceptand return a descriptive error string on failure. - Skip null (deleted) story payloads and safely derive
usernameviastory.get("by", "unknown"). - Add unit tests covering API failures, null story skipping, and missing
byfallback behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| libs/agno/agno/tools/hackernews.py | Adds exception handling and null/missing-field resilience in top stories fetch. |
| libs/agno/tests/unit/tools/test_hackernews.py | Adds targeted tests for error handling and edge cases introduced by HN API responses. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I noticed
get_top_hackernews_storieshas no error handling, while its siblingget_user_detailsin the same file wraps everything in try/except and returns a descriptive error string. When the HN Firebase API is unreachable or returns unexpected data,get_user_detailslets the agent explain the problem to the user, butget_top_hackernews_storiesjust raises an unhandled exception.There are two concrete failures:
API errors go unhandled. If the Firebase API returns an error or times out, the method crashes instead of returning an error string.
get_user_detailshandles this on line 75.Deleted stories crash the loop. The HN API returns
nullfor deleted items (https://hacker-news.firebaseio.com/v0/item/0.json→null). Whenstory_response.json()returnsNone,story["by"]raisesTypeError: 'NoneType' object is not subscriptable. Job postings can also lack abyfield, causingKeyError.Reproduction:
The fix adds a try/except matching
get_user_details, skips null stories, and uses.get("by", "unknown")instead of direct key access.Tests added:
test_get_top_stories_error_handling— API failure returns error stringtest_get_top_stories_null_story_skipped— null items are skippedtest_get_top_stories_missing_by_field— missingbyfalls back to "unknown"All 28 tests pass.
Fixes #7352