fix: browser UA, crypto unresolved-query, stock bad-mode (1.6.3) - #183
Merged
Conversation
…ock rejects bad mode (1.6.3) Three fixes found sweeping the scrapers and browser backend: - Browser backend ignored config.user_agent: with render_js=True it always sent user_agents[0], and screenshot() sent no UA at all. Both now share the HTTP backend's contract via a new ScraperConfig.pick_user_agent() (override wins, else rotate). Fixes a real config divergence between the two backends. - CryptoScraper: a query whose terms all failed to resolve to a CoinGecko id dropped the ids filter and returned the unfiltered top market — wrong data, no error. It now returns an error result instead (sync + async). - StockScraper: an unrecognized mode silently coerced to a quote; it now returns a ScrapeError naming the allowed modes (sync + async), mirroring IMDB. Regression-proven tests for each; 597 passed, ruff clean. Bump to 1.6.3.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
HttpClient._pick_ua() can crash with IndexError when user_agents=[] and no user_agent is configured, due to random.choice([]) in the new fallback logic.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR delivers three correctness fixes across the browser backend and two scrapers (crypto + stock), plus version/changelog updates for the 1.6.3 release, with regression tests covering the reported silent-failure behaviors.
Changes:
- Unifies User-Agent selection across HTTP + Playwright via
ScraperConfig.pick_user_agent()and ensures browser requests/screenshot contexts honorconfig.user_agent. - Makes
CryptoScraperreturn an explicit error (and no markets fetch) when a specific query resolves to no CoinGecko ids. - Validates
StockScrapermodeand returns a structuredScrapeErrorfor unknown values, instead of silently falling back to quote.
File summaries
| File | Description |
|---|---|
| tests/test_scrapers/test_stock.py | Adds regression test ensuring unknown mode errors instead of returning a quote. |
| tests/test_scrapers/test_data_apis.py | Adds regression test ensuring unresolvable crypto queries error and do not call markets endpoint. |
| tests/test_core/test_config.py | Adds tests for ScraperConfig.pick_user_agent() override/rotation/empty behaviors. |
| tests/test_core/test_browser.py | Adds tests verifying Playwright context gets the configured UA for get_html() and screenshot(). |
| src/pyscrappy/scrapers/stock.py | Rejects unknown mode values and returns a ScrapeResult containing a ScrapeError. |
| src/pyscrappy/scrapers/crypto.py | Errors early when query resolves to no ids to prevent “top market” fallback. |
| src/pyscrappy/core/http.py | Switches UA picking to ScraperConfig.pick_user_agent() (but introduces an empty-list edge case). |
| src/pyscrappy/core/config.py | Introduces pick_user_agent() shared by HTTP and browser backends. |
| src/pyscrappy/core/browser.py | Uses pick_user_agent() for browser contexts, including screenshot(). |
| src/pyscrappy/init.py | Bumps library version to 1.6.3. |
| server.json | Bumps server/package version metadata to 1.6.3. |
| pyproject.toml | Bumps project version to 1.6.3. |
| CHANGELOG.md | Adds 1.6.3 release notes documenting the fixes/enhancement. |
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
… instead Address review of mldsveda#183: _pick_ua's fallback did random.choice(user_agents), which raises IndexError when ScraperConfig(user_agent=None, user_agents=[]). Delegate fully to ScraperConfig.pick_user_agent() (no duplicated random.choice) and have _merge_headers omit the User-Agent header entirely when none is configured, rather than setting it to None. Bring the async client onto the same contract (it had the identical latent crash + duplication) and drop its now-unused random import.
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.
Three correctness fixes found sweeping the browser backend and scrapers. Each is wrong behavior with no error signal, and each has a regression-proven test.
Fixes
Browser backend ignored
config.user_agent. Withrender_js=True, a configured singleuser_agentwas silently dropped (the browser always sentuser_agents[0]), andscreenshot()sent no User-Agent at all (Playwright's headless-Chrome default, which many anti-bot systems block). The HTTP backend already honors the override via_pick_ua; the browser did not. Both now share one contract through a newScraperConfig.pick_user_agent()(a configureduser_agentwins, otherwiseuser_agentsrotates).Crypto: a specific-coin query that resolves to nothing returned the top market.
get_crypto(query="bitcon")(typo) or any query where no term resolved to a CoinGecko id dropped theidsfilter in_build_markets_urland returned the unfiltered top-20 by market cap, with no error. An agent could not tell the lookup failed. It now returns an empty result with a "No coins matched query" error (sync + async).Enhancement
Stock: unknown
modeis rejected.scrape_stock(mode="quotes")(typo) previously fell through a bareelseto a quote. It now returns aScrapeErrornaming the allowed modes (quote,history,profile), mirroring the validation IMDB already does (sync + async).Tests
pick_user_agentoverride / rotate / empty.get_htmlandscreenshotpass the configured UA to the browser context.597 passed, ruff clean. Bumped to 1.6.3 across all four version sites + CHANGELOG.
I deliberately left two target-markup-dependent issues out of this PR (generic pagination missing a visited-URL guard; IMDB early-exit on a sparse middle page) as candidates for separate issues rather than adhoc behavior changes.