Open
Conversation
PR Requirements WarningThis PR does not meet the contribution requirements. Missing: No linked issue found. To fix:
Exception: To bypass this requirement, you can:
Micro-fix requirements (must meet ALL):
Why is this required? See #472 for details. |
github-actions
bot
added
the
pr-requirements-warning
📝 WalkthroughWalkthroughThis pull request introduces a new Hacker News tool integration that fetches top stories and retrieves story details with comments via the public Firebase API, while simultaneously refining exception handling across multiple existing tools to be more specific about httpx errors. Changes
Poem
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tools/src/aden_tools/tools/hackernews_tool/README.md (1)
17-30: Document full parameter bounds and error shape for callers.Please add that
limit/comment_limitare clamped to a minimum of 1, and include the{"error": ...}response contract for failure cases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/src/aden_tools/tools/hackernews_tool/README.md` around lines 17 - 30, Update the README documentation for get_top_hn_stories and get_hn_story_details to explicitly state that the numeric parameters limit and comment_limit are clamped to a minimum value of 1 (i.e., values below 1 are treated as 1) and document any upper bounds if applicable; also add the failure response contract showing that callers will receive a JSON object shaped like {"error": "..."} on errors (include examples mentioning both parameter validation errors and network/fetch failures) so callers know to check for an error field when parsing responses.tools/tests/tools/test_hackernews_tool.py (1)
40-119: Good baseline tests; add edge-case coverage for bounds and malformed payloads.Consider adding cases for
limit/comment_limitclamping and unexpected JSON shapes (e.g., non-listtopstories, non-listkids) to lock in robustness behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/tests/tools/test_hackernews_tool.py` around lines 40 - 119, Add tests that exercise bounds and malformed payload handling for top_stories_tool and story_details_tool: create new test cases that (1) pass an excessively large and zero/negative limit to top_stories_tool and verify the returned "stories" count is clamped to expected min/max behavior, (2) mock the topstories endpoint to return a non-list (e.g., dict or None) and assert the tool returns an error or empty result per current behavior, (3) pass large and zero/negative comment_limit to story_details_tool and verify comments are clamped, and (4) mock story JSON where "kids" is non-list or missing and assert comments handling doesn’t crash and returns appropriate empty/comments error; use the same patch("httpx.Client.get") pattern and the existing MagicMock response setup as in test_get_top_stories_success and test_get_story_details_success to construct these edge-case responses while referencing top_stories_tool and story_details_tool in your new tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/src/aden_tools/tools/hackernews_tool/hackernews_tool.py`:
- Around line 47-55: Validate API payload shapes before slicing or dict access:
check resp.status_code/resp.ok and ensure resp.json() returns a list before
doing story_ids = resp.json()[:limit] (guard story_ids variable), and for each
item request check s_resp.ok and that s_resp.json() returns a dict before using
data = s_resp.json() and data.get("type"). Update the logic around the
variables/identifiers story_ids, resp, s_resp, data and uses of HN_API_BASE to
safely handle non-list or non-dict JSON (return or log a graceful error when the
shape is wrong) so malformed upstream responses don't raise.
---
Nitpick comments:
In `@tools/src/aden_tools/tools/hackernews_tool/README.md`:
- Around line 17-30: Update the README documentation for get_top_hn_stories and
get_hn_story_details to explicitly state that the numeric parameters limit and
comment_limit are clamped to a minimum value of 1 (i.e., values below 1 are
treated as 1) and document any upper bounds if applicable; also add the failure
response contract showing that callers will receive a JSON object shaped like
{"error": "..."} on errors (include examples mentioning both parameter
validation errors and network/fetch failures) so callers know to check for an
error field when parsing responses.
In `@tools/tests/tools/test_hackernews_tool.py`:
- Around line 40-119: Add tests that exercise bounds and malformed payload
handling for top_stories_tool and story_details_tool: create new test cases that
(1) pass an excessively large and zero/negative limit to top_stories_tool and
verify the returned "stories" count is clamped to expected min/max behavior, (2)
mock the topstories endpoint to return a non-list (e.g., dict or None) and
assert the tool returns an error or empty result per current behavior, (3) pass
large and zero/negative comment_limit to story_details_tool and verify comments
are clamped, and (4) mock story JSON where "kids" is non-list or missing and
assert comments handling doesn’t crash and returns appropriate empty/comments
error; use the same patch("httpx.Client.get") pattern and the existing MagicMock
response setup as in test_get_top_stories_success and
test_get_story_details_success to construct these edge-case responses while
referencing top_stories_tool and story_details_tool in your new tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cdfc4eba-9d8f-4708-899d-554e0f6ba60c
📒 Files selected for processing (10)
.gitignoretools/src/aden_tools/tools/__init__.pytools/src/aden_tools/tools/hackernews_tool/README.mdtools/src/aden_tools/tools/hackernews_tool/__init__.pytools/src/aden_tools/tools/hackernews_tool/hackernews_tool.pytools/src/aden_tools/tools/news_tool/news_tool.pytools/src/aden_tools/tools/openmeteo_tool/openmeteo_tool.pytools/src/aden_tools/tools/web_search_tool/web_search_tool.pytools/src/aden_tools/tools/wikipedia_tool/wikipedia_tool.pytools/tests/tools/test_hackernews_tool.py
💤 Files with no reviewable changes (2)
- tools/src/aden_tools/tools/wikipedia_tool/wikipedia_tool.py
- tools/src/aden_tools/tools/web_search_tool/web_search_tool.py
Comment on lines
+47
to
+55
| story_ids = resp.json()[:limit] | ||
|
|
||
| stories = [] | ||
| for sid in story_ids: | ||
| s_resp = client.get(f"{HN_API_BASE}/item/{sid}.json") | ||
| if s_resp.status_code == 200: | ||
| data = s_resp.json() | ||
| if data and data.get("type") == "story": | ||
| stories.append({ |
There was a problem hiding this comment.
Guard against unexpected API payload shapes before slicing or .get() access.
Current logic assumes top stories is always a list and item/comment payloads are always dicts. If upstream returns malformed/partial JSON, this can raise and bypass the graceful error responses.
🛠️ Proposed hardening patch
@@
- story_ids = resp.json()[:limit]
+ topstories_payload = resp.json()
+ if not isinstance(topstories_payload, list):
+ return {"error": "Unexpected HackerNews API response format for top stories."}
+ story_ids = topstories_payload[:limit]
@@
- data = s_resp.json()
- if data and data.get("type") == "story":
+ data = s_resp.json()
+ if isinstance(data, dict) and data.get("type") == "story":
@@
- data = resp.json()
-
- if not data or data.get("type") != "story":
+ data = resp.json()
+ if not isinstance(data, dict) or data.get("type") != "story":
return {"error": f"Story {story_id} not found or is not a story type."}
@@
- if include_comments and "kids" in data:
- kid_ids = data["kids"][:comment_limit]
+ if include_comments:
+ kid_ids = data.get("kids", [])
+ if not isinstance(kid_ids, list):
+ kid_ids = []
+ kid_ids = kid_ids[:comment_limit]
for kid_id in kid_ids:
@@
- if c_data and c_data.get("type") == "comment" and not c_data.get("deleted"):
+ if (
+ isinstance(c_data, dict)
+ and c_data.get("type") == "comment"
+ and not c_data.get("deleted")
+ ):
comments.append({Also applies to: 95-99, 113-119
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/src/aden_tools/tools/hackernews_tool/hackernews_tool.py` around lines
47 - 55, Validate API payload shapes before slicing or dict access: check
resp.status_code/resp.ok and ensure resp.json() returns a list before doing
story_ids = resp.json()[:limit] (guard story_ids variable), and for each item
request check s_resp.ok and that s_resp.json() returns a dict before using data
= s_resp.json() and data.get("type"). Update the logic around the
variables/identifiers story_ids, resp, s_resp, data and uses of HN_API_BASE to
safely handle non-list or non-dict JSON (return or log a graceful error when the
shape is wrong) so malformed upstream responses don't raise.
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
1 participant
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.
Description
Brief description of the changes in this PR.
Type of Change
Related Issues
Fixes #(issue number)
Changes Made
Testing
Describe the tests you ran to verify your changes:
cd core && pytest tests/)cd core && ruff check .)Checklist
Screenshots (if applicable)
Add screenshots to demonstrate UI changes.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes