fix(agent): port QWeather to ToolBase so it works as an Agent tool - #16692
Conversation
QWeather still extended the legacy ComponentBase/_run/be_output API that was removed in the agent redesign, so adding it to an Agent raised AttributeError (it had no get_meta()) and it was non-functional as a tool -- the same defect fixed for AkShare in infiniflow#16417. - QWeatherParam now extends ToolParamBase with a meta exposing a `query` (location) parameter and a get_input_form(). - QWeather now extends ToolBase and implements _invoke(**kwargs) with the standard retry loop, cancellation checks, set_output("formalized_content"), and thoughts(); config (api key, lang, type, time_period) is preserved. - Add test/unit_test/agent/component/test_qweather.py covering the restored meta, param validation, the weather/indices branches, empty-query short-circuit, and API-error messaging.
dosubot
Bot
added
the
size:L
📝 WalkthroughWalkthroughThe QWeather tool component is refactored from ComponentBase to ToolBase architecture, changing ChangesQWeather ToolBase Migration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant QWeather
participant GeoAPI
participant WeatherAPI
Caller->>QWeather: _invoke(query)
QWeather->>GeoAPI: lookup location
GeoAPI-->>QWeather: location result
alt lookup failed
QWeather->>QWeather: _error_message(code)
QWeather->>QWeather: _finish(error content)
else lookup succeeded
QWeather->>WeatherAPI: fetch weather/indices/airquality
WeatherAPI-->>QWeather: data
QWeather->>QWeather: _finish(formatted content)
end
QWeather-->>Caller: content
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
agent/tools/qweather.py (2)
130-133: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPass
locationviaparams=instead of string concatenation to avoid query-parameter injection.
locationis model/user-supplied and is concatenated directly into the query string. Whilerequestspercent-encodes spaces/non-ASCII, it does not encode reserved characters like&/=, so a value such asBeijing&key=...could inject or override query parameters. Passing structured params letsrequestsencode each value safely (applies to the weather/indices/air URLs at Lines 145, 156, 166 as well).♻️ Example fix for the geo lookup
- lookup = requests.get( - url="https://geoapi.qweather.com/v2/city/lookup?location=" + location + "&key=" + self._param.web_apikey, - timeout=DEFAULT_TIMEOUT, - ).json() + lookup = requests.get( + url="https://geoapi.qweather.com/v2/city/lookup", + params={"location": location, "key": self._param.web_apikey}, + timeout=DEFAULT_TIMEOUT, + ).json()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agent/tools/qweather.py` around lines 130 - 133, The QWeather request URLs are building query strings by concatenating the user-supplied location directly, which can allow query-parameter injection. Update the request construction in the lookup flow and the other QWeather helpers that build weather, indices, and air URLs to use requests.get(..., params=...) instead of string concatenation, passing location and the API key as structured parameters so requests handles encoding safely.
184-184: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid
assert False; raise or return explicitly.Under
python -Othis assertion is stripped, so the function silently returnsNoneon the fall-through path (reachable ifmax_retriesis negative). Prefer an explicit return of the current output or anAssertionError.♻️ Proposed change
- assert False, self.output() + raise AssertionError(self.output())🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agent/tools/qweather.py` at line 184, The fall-through path in the retry logic uses an assert to signal an impossible state, but that can be stripped under optimized Python and silently return None. Update the relevant fallback in qweather.py (the retry/response handling around self.output()) to use an explicit raise AssertionError or return the current output directly, and ensure the behavior is defined even when max_retries is negative.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@agent/tools/qweather.py`:
- Around line 130-133: The QWeather request URLs are building query strings by
concatenating the user-supplied location directly, which can allow
query-parameter injection. Update the request construction in the lookup flow
and the other QWeather helpers that build weather, indices, and air URLs to use
requests.get(..., params=...) instead of string concatenation, passing location
and the API key as structured parameters so requests handles encoding safely.
- Line 184: The fall-through path in the retry logic uses an assert to signal an
impossible state, but that can be stripped under optimized Python and silently
return None. Update the relevant fallback in qweather.py (the retry/response
handling around self.output()) to use an explicit raise AssertionError or return
the current output directly, and ensure the behavior is defined even when
max_retries is negative.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f5f798d0-534a-4d99-a41d-fec4818c217b
📒 Files selected for processing (2)
agent/tools/qweather.pytest/unit_test/agent/component/test_qweather.py
2 participants
Summary
Port the QWeather agent tool to the modern
ToolBase/_invokeinterface. It was still written against the removed legacyComponentBase/_run/be_outputAPI, so it was non-functional as an Agent tool — adding it to an Agent raisedAttributeErrorbecause it had noget_meta(). This is the same defect that was fixed for the AkShare tool in #16417.Changes
QWeatherParamnow extendsToolParamBasewith ametaexposing aquery(location) parameter, and addsget_input_form(). Existing config (web_apikey,lang,type,user_type,time_period) is preserved.QWeathernow extendsToolBaseand implements_invoke(**kwargs)with the standard retry loop, cancellation checks,set_output("formalized_content", ...), andthoughts(). The weather / indices / air-quality branches and the API error-code messages are kept.test/unit_test/agent/component/test_qweather.pycovering the restoredmeta, param validation, the weather-now and multi-day and indices branches, the empty-query short-circuit, and the location-lookup error message.Testing
ruff check agent/tools/qweather.py test/unit_test/agent/component/test_qweather.py— cleanruff format --check— cleanpytest test/unit_test/agent/component/test_qweather.py