feat: add stocks & finance skill (Yahoo Finance, no API key) - #2014
feat: add stocks & finance skill (Yahoo Finance, no API key)#2014Mibayy wants to merge 1 commit into
Conversation
5 commands: quote, search, history, compare, crypto Zero dependencies, Python stdlib only. Supports multi-symbol queries and crypto prices.
|
Orb Code Review (powered by GLM-4.7 on Orb Cloud) SummaryThis PR adds a new optional finance skill to the Hermes Agent project that provides real-time stock and cryptocurrency data via Yahoo Finance API. The implementation includes 5 commands (quote, search, history, compare, crypto), uses only Python standard library (no external dependencies), and follows the established pattern for optional skills in the project. ArchitectureThe PR follows the existing Hermes Agent skill structure: File Structure:
Key Components:
Data Flow:
The architecture is clean and follows the pattern established by other skills (e.g., blockchain/base). The use of only standard library ( IssuesCriticalNo critical issues found. The core functionality is sound and the code is well-structured. Warningsstocks_client.py:596 — Division by zero risk — Potential total_return = ((valid_closes[-1] - valid_closes[0]) / valid_closes[0]) * 100
# What if valid_closes[0] is 0.0?Suggested fix: Add validation before division: if len(valid_closes) >= 2:
first_close = valid_closes[0]
if first_close and first_close != 0:
total_return = ((valid_closes[-1] - first_close) / first_close) * 100
stats["total_return_pct"] = fmt_pct(total_return)
else:
stats["total_return_pct"] = Nonestocks_client.py:672 — Division by zero risk — Similar issue in compare command: perf = ((price_f - low_f) / low_f) * 100
# What if low_f is 0.0?Suggested fix: Add zero check: if price_f and low_f and low_f > 0:
perf = ((price_f - low_f) / low_f) * 100
entry["52w_performance_pct"] = fmt_pct(perf)stocks_client.py:127 — Global state not thread-safe — The crumb is stored in a global variable: _crumb: str | None = None
# If this module is used in a multi-threaded context,
# concurrent access to _crumb could cause race conditionsSuggested fix: Consider using thread-local storage or a singleton pattern: import threading
_crumb_lock = threading.Lock()
_crumb: str | None = None
def _fetch_crumb() -> str | None:
global _crumb
with _crumb_lock:
if _crumb is not None:
return _crumb
# ... fetch logic ...stocks_client.py:305 — Missing input validation — Search query not validated: def yf_search(query: str, count: int = 5) -> dict | None:
params = {"q": query, "quotesCount": count, "newsCount": 0}
# No validation on query length or contentSuggested fix: Add basic validation: def yf_search(query: str, count: int = 5) -> dict | None:
if not query or len(query.strip()) == 0:
return None
if len(query) > 100:
query = query[:100] # Truncate excessively long queries
query = query.strip()
# ... rest of function ...Suggestionsstocks_client.py:291-302 — Code duplication in crumb handling — Each Yahoo Finance API function duplicates crumb fetching logic: def yf_chart(symbol: str, interval: str = "1d", range_: str = "1d") -> dict | None:
params = {"interval": interval, "range": range_}
crumb = _fetch_crumb() # Duplicated in yf_search and yf_quote_summary
if crumb:
params["crumb"] = crumbSuggested fix: Use the existing def yf_chart(symbol: str, interval: str = "1d", range_: str = "1d") -> dict | None:
params = {"interval": interval, "range": range_}
url = yf_url(f"/v8/finance/chart/{urllib.parse.quote(symbol)}", params)
return fetch_url(url)stocks_client.py:296-301 — Inconsistent URL construction — Direct URL building instead of using helper: qs = urllib.parse.urlencode(params)
url = f"{YF_BASE}/v8/finance/chart/{urllib.parse.quote(symbol)}?{qs}"
data = fetch_url(url)
if data is None:
url2 = f"{YF_BASE2}/v8/finance/chart/{urllib.parse.quote(symbol)}?{qs}"
data = fetch_url(url2)Suggested fix: Consolidate fallback logic into a helper function: def _fetch_with_fallback(path: str, params: dict) -> dict | None:
"""Try query1, fallback to query2 on failure."""
url = yf_url(path, params)
data = fetch_url(url)
if data is None:
fallback_url = url.replace(YF_BASE, YF_BASE2)
data = fetch_url(fallback_url)
return dataMissing rate limiting — No client-side rate limiting to protect Yahoo Finance API: # Multiple symbols in one call could hit rate limits
def cmd_quote(symbols: list[str]) -> None:
for sym in symbols:
# No delay between requests
chart_data = yf_chart(sym, interval="1d", range_="1d")
qs_data = yf_quote_summary(sym)Suggested fix: Add rate limiting: import time
REQUEST_DELAY = 0.5 # 500ms between requests
_last_request_time = 0
def _rate_limit() -> None:
global _last_request_time
now = time.time()
if now - _last_request_time < REQUEST_DELAY:
time.sleep(REQUEST_DELAY - (now - _last_request_time))
_last_request_time = time.time()
def cmd_quote(symbols: list[str]) -> None:
for sym in symbols:
_rate_limit()
chart_data = yf_chart(sym, interval="1d", range_="1d")
# ...SKILL.md:80 — Incomplete error message extraction — Could fail if error description is missing: err = safe_get(chart_data, "chart", "error", "description") or "Unknown error"
# What if the error structure is different?Suggested fix: Provide more robust error handling: chart_error = safe_get(chart_data, "chart", "error")
if isinstance(chart_error, dict):
err = chart_error.get("description") or chart_error.get("message") or "Unknown error"
else:
err = "Unknown error"Missing test coverage — No unit tests for the new skill: # While other skills don't have tests either,
# adding basic tests would improve confidence:
# - Test formatting functions (fmt_price, fmt_pct, etc.)
# - Test safe_get utility
# - Mock HTTP requests and test command handlersSuggested fix: Add basic test file: # optional-skills/finance/tests/test_stocks_client.py
import unittest
from scripts.stocks_client import fmt_price, fmt_pct, safe_get
class TestFormatting(unittest.TestCase):
def test_fmt_price(self):
self.assertEqual(fmt_price(123.456), "123.46")
self.assertIsNone(fmt_price(None))
def test_safe_get(self):
data = {"a": {"b": {"c": 42}}}
self.assertEqual(safe_get(data, "a", "b", "c"), 42)
self.assertIsNone(safe_get(data, "a", "x", "y"))Consider adding logging — No logging for debugging or monitoring: # Adding logging would help with debugging production issues
import logging
logger = logging.getLogger(__name__)
def fetch_url(url: str, headers: dict | None = None, retries: int = MAX_RETRIES):
logger.debug(f"Fetching {url}")
# ...SKILL.md could include more context — Missing information about data freshness: ## Pitfalls
- Yahoo Finance API is unofficial and may change without notice.
- market_cap and pe_ratio may return null (require session crumb).
- Rate limits: add delays between bulk requests.
+ - Data latency: Prices may be delayed 15-20 minutes for free tier.
+ - Market hours: Some data may not be available outside trading hours.Cross-file ImpactNew Files Added:
Modified Files: None Dependencies: No new dependencies (uses Python stdlib only) Breaking Changes: None. This is a new optional skill that doesn't affect existing functionality. Public API: The skill is invoked via CLI, not as a Python module, so there's no public Python API to consider. Test Coverage: No tests included (consistent with other skills in optional-skills) Documentation: SKILL.md is comprehensive and follows the established pattern Assessment✅ Approve This is a well-implemented addition to the Hermes Agent project that provides valuable finance capabilities: Strengths:
Minor Areas for Improvement:
Why This is Good:
The implementation demonstrates good understanding of both the Yahoo Finance API and the Hermes Agent architecture. While there are some minor improvements that could be made (division by zero checks, thread safety, code deduplication), none of these are blocking issues and the core functionality is sound. The skill is ready for production use and provides real value to users. |
alt-glitch
added
type/feature
|
Your commit was cherry-picked onto current main with your authorship preserved in git log. Polished on top: relocated to optional-skills/finance/stocks/, tightened SKILL.md description to <=60 chars, added platforms gating, fixed install paths to match the actual hub install location. Thanks for the contribution! |
4 participants
Summary
Adds a stocks and finance skill using Yahoo Finance — no API key required.
Features
quote,search,history,compare,cryptoUsage
Files
optional-skills/finance/SKILL.mdoptional-skills/finance/scripts/stocks_client.py(~755 lines)Tested live — quote AAPL returns $249.94, search, history, crypto all working.