feat: Apify web search provider - #1
Open
JanHranicky wants to merge 15 commits into
Open
Conversation
…hema, tests Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add _run_website_content_crawler helper and replace the extract() stub with the full async implementation: per-URL crawl via asyncio.to_thread, 60s wait_for guard, pre/post-redirect website policy checks, format selection (markdown/html/both), and per-URL error items on any failure. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add _run_actor_blocking() shared helper — eliminates duplicated .start() → log → wait_for_finish() → status check → dataset fetch pattern that existed in both _run_wcc_crawl and _run_website_content_crawler - Add _RAG_ACTOR / _WCC_ACTOR constants for repeated actor ID strings - Extract check_apify_api_key() so is_available() delegates to it, matching the pattern used by all other providers - Save run_id before reassigning run to avoid ambiguous variable reuse - Add status check in _run_actor_blocking: non-SUCCEEDED runs log a warning and return [] rather than silently returning empty results - search() simplified to call _run_actor_blocking directly - Add module docstring and # --- section separators matching Firecrawl style; add __init__.py module docstring Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
JanHranicky
marked this pull request as ready for review
May 26, 2026 11:44
JanHranicky
changed the title
drobnikj
requested changes
May 27, 2026
drobnikj
left a comment
drobnikj
left a comment
Member
There was a problem hiding this comment.
One question otherwise fine.
🔎 Lint report:
|
| Rule | Count |
|---|---|
unresolved-import |
1 |
First entries
tools/apify_client.py:27: [unresolved-import] unresolved-import: Cannot resolve imported module `apify_client`
✅ Fixed issues: none
Unchanged: 4929 pre-existing issues carried over.
Diagnostics are surfaced as warnings — this check never fails the build.
drobnikj
approved these changes
May 29, 2026
drobnikj
left a comment
drobnikj
left a comment
Member
There was a problem hiding this comment.
Just two note, but I fine with the version, pre approving 💪
| started = client.actor(actor_id).start(run_input=run_input) | ||
| run_id = started.id | ||
| logger.info("Apify %s started — https://console.apify.com/actors/runs/%s", actor_id, run_id) | ||
| run = client.run(run_id).wait_for_finish() |
Member
There was a problem hiding this comment.
Note: wait_for_finish() is called without wait_secs, so it blocks indefinitely. When the outer asyncio.wait_for(timeout=60) (extract) or timeout=300 (crawl) fires, Python cannot interrupt the thread spawned by asyncio.to_thread — it keeps blocking until the SDK call returns (potentially many minutes), and the Apify Actor run continues consuming the user's budget the entire time.
Not sure if it is good approach, just double check.
| def _normalize_rag_search_results(items: List[Any], limit: int) -> List[Dict[str, Any]]: | ||
| """Normalize RAG Web Browser dataset items to the registry web search shape.""" | ||
| results: List[Dict[str, Any]] = [] | ||
| for item in items[:limit]: |
Member
There was a problem hiding this comment.
Note: slicing items[:limit] before filtering non-dicts means a malformed early item silently shrinks the result set. If the Actor returns e.g. [non-dict, dict, dict, dict] with limit=3, you return 2 results even though 3 valid ones exist.
Moves _get_apify_client / check_apify_api_key / reset_client_for_tests out of the web-search provider into a standalone module so the upcoming Actor-execution tools (apify_tool.py) can import the client without depending on the provider — enabling both to be submitted as independent PRs targeting main. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
JanHranicky
pushed a commit
that referenced
this pull request
Jun 4, 2026
…s reached After key #1 is marked exhausted the retry still called the API with key #1 due to env-var bias in _get_cached_client / resolve_api_key_provider_credentials. Fix: peek the pool and pass the active entry's key as explicit_api_key. Secondary: api_key_hint in mark_exhausted_and_rotate pins the correct entry under concurrent CLI+gateway calls; _is_payment_error matches GoUsageLimitError; extract_api_error_context parses "Resets in Xhr Ymin".
JanHranicky
pushed a commit
that referenced
this pull request
Jun 4, 2026
…ookies
Mission-control style deploys reverse-proxy the dashboard at a path
prefix (e.g. mission-control.tilos.com/hermes/* -> :9119) and inject
X-Forwarded-Prefix: /hermes on every request. The SPA mount already
honoured this for asset URLs and the bootstrap __HERMES_BASE_PATH__,
but the OAuth gate didn't:
1. The gate's Location: header to /login and the 401 envelope's
login_url were built bare ("/login?next=..."). Under a /hermes
prefix the browser follows that to mission-control.tilos.com/login
which the proxy doesn't route to the dashboard.
2. _redirect_uri (the OAuth callback URL handed to the IDP) used
request.url_for() which doesn't honour X-Forwarded-Prefix
(Starlette/uvicorn only proxy_headers Host + Proto + For). The
IDP redirects back to /auth/callback instead of /hermes/auth/
callback → 404 in the user's browser.
3. Cookies were set with Path=/ which leaks them to other apps on
the same origin and won't be sent back on requests under the
prefix in the first place.
Fix threads the normalised prefix through every boundary:
* New hermes_cli/dashboard_auth/prefix.py — single source of truth
for X-Forwarded-Prefix parsing. web_server._normalise_prefix
becomes a re-export so the SPA mount, the gate, and the cookies
helper all agree.
* middleware._unauth_response builds login_url = f"{prefix}/login".
* routes._redirect_uri splices the prefix into the path component
of the IDP-bound URL (with full validation of the header).
* cookies.{set,clear}_{session,pkce}_cookie now take prefix="".
Path attribute switches to /hermes when set; cookie name switches
name variant (see below). Every caller passes the request's
normalised prefix.
Cookie hardening (Teknium's lesser-note #1 in the PR review): adopt
the __Host- / __Secure- cookie name prefixes per draft-west-cookie-
prefixes. The variant is selected from (use_https, prefix):
* Loopback HTTP → bare "hermes_session_at" (both prefixes require
Secure, incompatible with HTTP).
* HTTPS, direct deploy (Path=/) → "__Host-hermes_session_at".
Strongest spec: bound to exact origin, no Domain attribute, Secure
required.
* HTTPS, behind a proxy prefix (Path=/hermes) →
"__Secure-hermes_session_at". __Host- forbids Path != "/"; the
explicit Path=/hermes covers same-origin app isolation.
Setter and reader BOTH consult the prefix because the cookie *name*
changes — a reader that looked up the bare name when the setter wrote
__Secure- would never find the value. The reader falls back across
all three variants so a request whose shape changed mid-session (e.g.
post-deploy from no-prefix to /hermes) still picks up the existing
cookie until it expires.
Test coverage:
- tests/hermes_cli/test_dashboard_auth_prefix.py — new file. 11 tests
pinning:
• Location: /hermes/login on the gate's HTML redirect
• 401 envelope login_url carries the prefix
• Malformed X-Forwarded-Prefix is ignored (header-injection
defence; the script-tag value is normalised to empty string)
• _redirect_uri splices /hermes into the path (the property
that prevents the IDP-returns-to-404 failure)
• PKCE cookie uses Path=/hermes + __Secure- when proxied
• Session cookies use __Host- when direct, __Secure- when
proxied, bare on loopback HTTP
• End-to-end round trip with hand-managed PKCE cookie carriage
(TestClient can't simulate a Path=/hermes cookie automatically)
- tests/hermes_cli/test_dashboard_auth_cookies.py — rewritten to pin
each (use_https, prefix) shape produces its expected cookie name,
plus reader-side coverage that __Host- and __Secure- variants are
both recognised.
- Existing tests across middleware / 401-reauth / etc. updated to
match the new cookie names (substring contains instead of
startswith).
Mutation-tested: reverting _unauth_response to build the bare
"/login" URL trips exactly the two tests that pin the prefix
carriage, confirming the suite discriminates the regression.
JanHranicky
pushed a commit
that referenced
this pull request
Jun 4, 2026
Two CI flakes surfaced on PR NousResearch#34572 (both in files this PR doesn't touch; pre-existing host-dependent flakes): 1. test_process_registry::TestPopenLeakOnSetupFailure — the failure-cleanup tests use a fake proc.pid (8888/9999) and assert proc.kill() runs. But spawn_local's primary cleanup is os.killpg(os.getpgid(pid), SIGKILL), falling back to proc.kill() only on ProcessLookupError/PermissionError/ OSError. When the fake PID happens to exist on a busy host, os.getpgid succeeds, os.killpg fires against an UNRELATED real process group, and proc.kill() is never reached -> flaky AssertionError (and a real risk of SIGKILLing an innocent process group from a unit test). Patch os.getpgid to raise ProcessLookupError so the fallback path runs deterministically and no real killpg is ever issued. 2. test_web_server::test_resize_escape_is_forwarded — the receive loop calls the blocking conn.receive_bytes() with no exception guard. Once the child prints its winsize and exits, the PTY closes; on a missed-marker run the next recv blocks until the 30s pytest-timeout instead of failing fast. Add a try/except break (matching the working sibling tests) and bump the child's pre-read sleep 0.15s -> 0.5s so the resize reliably lands first. Verified: 4/4 pass across 3 consecutive runs; root cause for #1 reproduced (os.getpgid(1) succeeds -> old code skips proc.kill).
JanHranicky
pushed a commit
that referenced
this pull request
Jun 4, 2026
Seven Copilot inline review comments on NousResearch#37679, four worth landing in a polish pass before merge: 1. _dispose_unused_adapter signature: 'BasePlatformAdapter' -> 'BasePlatformAdapter | None'. The function explicitly handles None and the reconnect watcher calls it with None in the except arm, so the annotation now matches the actual contract. 2. (duplicate of #1 on a different line) — same fix. 3. except Exception in _dispose_unused_adapter — the reviewer asked about asyncio.CancelledError swallowing. On Python 3.8+ (Hermes requires 3.13, see pyproject.toml), CancelledError inherits from BaseException, NOT Exception, so the existing 'except Exception' does NOT swallow task cancellation. Added an explicit comment explaining the contract so future readers don't repeat the analysis. We don't re-raise because the watcher loop intentionally treats dispose failures as best-effort: a failed dispose on an unowned adapter should not take down the watcher that's keeping the gateway alive. 4. _response_store = None after close in api_server.py — the reviewer flagged this for idempotency. Decided to keep the non-None state intentionally: setting it to None cascades to ~9 callers that access self._response_store without a None check, and 'close() is idempotent on a closed sqlite3 Connection' means the current code is already safe. The type stays stable; LSP doesn't flag a cascade of reportOptionalMemberAccess errors. (This matches the pre-existing pattern in the codebase — e.g. _mark_disconnected doesn't reset state to None either.) 5. _build_adapter_with_store: reviewer worried about disconnect() failing on the self.name property if __init__ wasn't called. Already handled: we set 'adapter.platform = Platform.API_SERVER' so the 'self.platform.value.title()' property returns 'Api_Server' without raising. The exception-swallowing branch in disconnect() does call self.name via the logger.debug format, so this is a real path that needs the platform attribute, and we have it. 6. test_disconnect_closes_response_store: bare 'pytest.raises(Exception)' -> 'pytest.raises(sqlite3.ProgrammingError)'. The bare Exception matcher would silently accept AttributeError, OperationalError, env-related issues, etc. The specific exception type ('Cannot operate on a closed database') is the actual signal we want — proves the SQLite conn is closed, not just that *something* raised. 7. test_nonretryable_failure_disposes_unowned_adapter: assertion tightened from '>= 1' to '== 1' on adapter._disconnect_calls. The docstring said 'exactly once', the assertion now matches. Catches the hypothetical 'watcher disposes the same adapter twice' regression that '>=' would have missed.
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
4 participants
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.
Adds Apify as a fourth web backend alongside Firecrawl, Tavily, and Exa. Configure with web.backend: apify + APIFY_API_TOKEN.
What it does
Crawl notes
TLDR: Crawl is implemented but without Agents cannot use it due to Hermes limitations
The implemented
WebSearchProviderhascrawlmethod, unfortunately it isn't being detected and agents cannot use it. When developing I temporatily enabled it by changing the registration logic, but imo for the upstream repo this is too extensive of an addition. So I reverted the change and will file Hermes ticket.Implementation notes
Scope
Plugin-only. Changes to tools/web_tools.py and hermes_cli/config.py are minimal — adding "apify" to the backend detection sets and APIFY_API_TOKEN to the optional env var list. No new tools registered, no core behavior changed.
Tests
58 tests in tests/plugins/web/test_web_search_provider_plugins.py, all mocked, no live Apify calls.