Skip to content

fix(docker): add pypdf and honor stream scraping strategy #2130

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
nightcityblade wants to merge 2 commits into
base: develop
Choose a base branch
from
+82 −5
Open

fix(docker): add pypdf and honor stream scraping strategy #2130

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
05de127
fix(docker): support PDF scraping by default
Aug 7, 2026
10130de
test(docker): address PDF review feedback
Aug 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions deploy/docker/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -865,15 +865,20 @@ async def handle_stream_crawl_request(
# mirroring handle_crawl_request. The streaming path previously skipped
# this, leaving /crawl/stream (and /crawl with stream=true) unguarded.
urls = _normalize_and_validate_seeds(urls)
browser_config = BrowserConfig.load(browser_config, provenance=Provenance.UNTRUSTED)
browser_config = BrowserConfig.load(
browser_config, provenance=Provenance.UNTRUSTED
)
# browser_config.verbose = True # Set to False or remove for production stress testing
browser_config.verbose = False
from egress_broker import enforce_egress

enforce_egress(browser_config)
crawler_config = CrawlerRunConfig.load(crawler_config, provenance=Provenance.UNTRUSTED)
crawler_config = CrawlerRunConfig.load(
crawler_config, provenance=Provenance.UNTRUSTED
)
from governor import clamp_deep_crawl

clamp_deep_crawl(crawler_config)
crawler_config.scraping_strategy = LXMLWebScrapingStrategy()
crawler_config.stream = True

# Deep crawl streaming supports exactly one start URL
Expand Down Expand Up @@ -941,7 +946,7 @@ async def handle_stream_crawl_request(
status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
detail=str(e)
)

async def handle_crawl_job(
redis,
background_tasks: BackgroundTasks,
Expand Down Expand Up @@ -1023,4 +1028,4 @@ async def _runner():
except HTTPException:
await redis.delete(f"task:{task_id}")
raise
return {"task_id": task_id}
return {"task_id": task_id}
1 change: 1 addition & 0 deletions deploy/docker/requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,4 @@ PyJWT==2.10.1
mcp>=1.18.0
websockets>=15.0.1
httpx[http2]>=0.27.2
pypdf

@SohamKukreti SohamKukreti Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pypdf
pypdf>=6.0.0

@nightcityblade nightcityblade Aug 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to pypdf>=6.0.0 in 10130de, matching the library's root requirement.

31 changes: 31 additions & 0 deletions tests/test_issue_2127_docker_pdf.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import ast
from pathlib import Path

ROOT = Path(__file__).resolve().parent.parent


def test_default_docker_dependencies_include_pypdf():
requirements = (
(ROOT / "deploy" / "docker" / "requirements.txt").read_text().splitlines()
)

assert "pypdf" in requirements

@SohamKukreti SohamKukreti Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This exact-line match breaks as soon as the requirement gets a version spec (e.g. pypdf>=6.0.0, requested above) — the test would fail on a correct change. Parsing the requirement names makes it robust:

from packaging.requirements import Requirement, InvalidRequirement

def test_default_docker_dependencies_include_pypdf():
    lines = (ROOT / "deploy" / "docker" / "requirements.txt").read_text().splitlines()
    names = set()
    for line in lines:
        line = line.strip()
        if not line or line.startswith(("#", "-")):
            continue
        try:
            names.add(Requirement(line).name)
        except InvalidRequirement:
            continue  # pip-specific syntax (inline comments, paths, etc.)
    assert "pypdf" in names

@nightcityblade nightcityblade Aug 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in 10130de. The regression now parses each valid requirement with packaging.requirements.Requirement, so version constraints do not make a correct dependency entry fail the test.



def test_stream_handler_preserves_requested_scraping_strategy():
tree = ast.parse((ROOT / "deploy" / "docker" / "api.py").read_text())
handler = next(
node
for node in tree.body
if isinstance(node, ast.AsyncFunctionDef)
and node.name == "handle_stream_crawl_request"
)
assigned_attributes = {
target.attr
for node in ast.walk(handler)
if isinstance(node, ast.Assign)
for target in node.targets
if isinstance(target, ast.Attribute)
}

assert "scraping_strategy" not in assigned_attributes

@SohamKukreti SohamKukreti Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This asserts on the shape of the source rather than the behavior, which makes it both bypassable and over-strict — I verified both directions locally:

  • It passes with the bug reintroduced via setattr(crawler_config, "scraping_strategy", LXMLWebScrapingStrategy()) or via an annotated assignment (crawler_config.scraping_strategy: object = ... is an ast.AnnAssign, which this walk doesn't catch).
  • It fails on legitimate code: a future default-only-if-unset pattern like crawler_config.scraping_strategy = crawler_config.scraping_strategy or LXMLWebScrapingStrategy() preserves client strategies but trips this assertion — so the test would actively obstruct the correct implementation if we ever want an explicit server-side default here.

A behavioral test would guard the actual contract. For example: load the issue's payload through CrawlerRunConfig.load(..., provenance=Provenance.UNTRUSTED), run it through the same config-processing the stream handler does, and assert the config still holds a PDFContentScrapingStrategy. That stays green for any implementation that preserves the client's strategy and red for any that clobbers it, regardless of syntax.

@nightcityblade nightcityblade Aug 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced the AST assertion in 10130de with an async behavioral test. It invokes the real handle_stream_crawl_request, deserializes the issue's PDFContentScrapingStrategy config through the handler, and verifies that the resulting strategy is passed to crawler.arun_many.

Loading