feat(paper-search): add SerpAPI and SearchAPI provider support#278
feat(paper-search): add SerpAPI and SearchAPI provider support#278gcasilva wants to merge 6 commits into
Conversation
Add a 'provider' config field to the Google Scholar paper search tool so developers can choose between Serper (default), SerpAPI, or SearchAPI as the backend. All three query Google Scholar and return results normalized to the same shape (title, year, snippet, link, publication info, citations), so the agent-facing tool behavior is identical regardless of provider. Changes: - paper_search.py: add PaperSearchProvider StrEnum, provider dispatch in search(), _search_serpapi/_search_searchapi clients, _normalize_serpapi/ _normalize_searchapi mappers, _extract_year regex for publication strings, _parse_year_range shared helper - register.py: add provider/serpapi_api_key/searchapi_api_key config fields, _PROVIDER_KEY_INFO dispatch table, _resolve_api_key helper, provider-aware stub error messages - pyproject.toml: bump 1.0.0 -> 1.1.0, fix httpx -> aiohttp dep (code already used aiohttp), update description - tests: 21 -> 58 tests covering provider init, year extraction, normalization, fetch payload construction, pagination (incl. SearchAPI 1-based page), provider dispatch, and end-to-end search per provider - docs: update source README, root README API keys table, deploy/.env.example with all three providers Fully backward compatible: provider defaults to 'serper', serper_api_key field and SERPER_API_KEY env var path unchanged, existing configs work without modification. Signed-off-by: Gabriel Costa <gcasilva@outlook.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe paper search tool now supports Serper, SerpAPI, and SearchAPI via a provider selector. The implementation, registration flow, tests, package metadata, and documentation were updated to use provider-specific API keys and normalized search results. ChangesMulti-provider paper search
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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.
Inline comments:
In `@sources/google_scholar_paper_search/README.md`:
- Around line 5-9: The table header in the README incorrectly labels the second
column as "`_type` value" when it actually contains provider identifier values
like serper, serpapi, and searchapi. Change the column header from "`_type`
value" to accurately reflect that this column represents the provider field
value or provider identifier that users should use in their configuration, not
the _type value which is always paper_search.
In `@sources/google_scholar_paper_search/src/paper_search.py`:
- Around line 149-151: The exception handlers at lines 149-151, 324-327, and
410-413 in the paper_search.py file are logging and returning raw exception
messages that could contain sensitive provider credentials or request details
from upstream APIs. Replace the raw exception string representations (str(e))
with sanitized, generic error messages that do not expose implementation
details. For example, instead of returning the actual exception content, return
a simple message like "Paper search failed: Unable to fetch results" or "Paper
search failed: Service temporarily unavailable". This applies to all three
exception handler blocks mentioned in the comment.
In `@sources/google_scholar_paper_search/src/register.py`:
- Around line 80-85: The `_resolve_api_key()` function is persisting secrets
into global `os.environ` on lines 83-84, which can leak config secrets across
workflows and registrations. Remove the lines that set os.environ with the
secret value from the SecretStr, and instead return the resolved api_key value
directly by returning either the environment variable value or the
config_value.get_secret_value() result. Update all callers of
`_resolve_api_key()` to use the returned api_key directly when constructing
tools, rather than relying on the modified environment variables. Apply this
same change to the similar code pattern at lines 127-131.
In `@sources/google_scholar_paper_search/tests/test_paper_search.py`:
- Around line 856-867: The test_search_serpapi_handles_error method currently
requires the raw provider error text ("SerpAPI Error") to be exposed in the
user-visible result, which violates security guidelines by leaking upstream
exception details and potentially sensitive provider payload data. Remove the
assertion that checks for "SerpAPI Error" in the result string, and instead
verify that the search method handles the error gracefully by returning a
generic, sanitized error message that does not expose raw provider exception
details to users.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 2d4cfae4-d7d1-4ebf-a0ab-4dd0765c22d1
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
README.mddeploy/.env.examplesources/google_scholar_paper_search/README.mdsources/google_scholar_paper_search/pyproject.tomlsources/google_scholar_paper_search/src/paper_search.pysources/google_scholar_paper_search/src/register.pysources/google_scholar_paper_search/tests/conftest.pysources/google_scholar_paper_search/tests/test_paper_search.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
{src/aiq_agent/knowledge/**,sources/**}
⚙️ CodeRabbit configuration file
{src/aiq_agent/knowledge/**,sources/**}: Review data-source and knowledge-layer changes for optional dependency boundaries, external API error handling,
retry/rate-limit behavior, deterministic tests, and registration consistency. New source packages should include
package metadata, plugin registration when applicable, and source-level tests.
Files:
sources/google_scholar_paper_search/pyproject.tomlsources/google_scholar_paper_search/README.mdsources/google_scholar_paper_search/src/register.pysources/google_scholar_paper_search/tests/test_paper_search.pysources/google_scholar_paper_search/tests/conftest.pysources/google_scholar_paper_search/src/paper_search.py
**
⚙️ CodeRabbit configuration file
**:AI-Q Agent Guidance
Repository-global instructions for coding agents and for humans reviewing
agent-authored changes. These rules apply to every task in this repository.
Task-specific runbooks live in.agents/skills/— load the
relevant skill before starting a workflow it covers.Project overview
AI-Q is an NVIDIA AI Blueprint: an enterprise research agent built on the
NeMo Agent Toolkit (NAT). The deployed product is a research blueprint, not
a general skill runtime. New retrieval sources and tools are NAT functions;
agent behavior is driven by workflow YAML, Jinja2 prompts, and a data-source
registry — not by hard-coded logic.Primary boundaries:
- Backend Python package:
src/aiq_agent/.- Data-source and tool packages:
sources/(each is its own package).- Frontends and tooling:
frontends/(web UI infrontends/ui/, eval harnesses
infrontends/benchmarks/).- Configs, deployment, docs:
configs/,deploy/,docs/.Stay inside this repository. If your workspace also contains adjacent repos
(for example a sibling NeMo-Relay checkout), do not edit them as part of an AI-Q
change. Treatsources/*as independent packages: prefer the smallest change
scoped to the package you are touching.Repository structure
Path Purpose src/aiq_agent/Backend agent, FastAPI extensions, auth, observability, knowledge sources/Data-source / tool packages (e.g. tavily_web_search,google_scholar_paper_search)configs/Workflow YAML configs (e.g. config_cli_default.yml)frontends/ui/Next.js / React / TypeScript / Tailwind / KUI web UI frontends/benchmarks/Eval harnesses: freshqa,deepsearch_qa,deepresearch_benchdeploy/Docker Compose and Helm/Kubernetes assets; deploy/.envfor secretsdocs/source/...
Files:
sources/google_scholar_paper_search/pyproject.tomlREADME.mdsources/google_scholar_paper_search/README.mdsources/google_scholar_paper_search/src/register.pysources/google_scholar_paper_search/tests/test_paper_search.pysources/google_scholar_paper_search/tests/conftest.pysources/google_scholar_paper_search/src/paper_search.py
{docs/**,README.md,CONTRIBUTING.md,SECURITY.md,CODE-OF-CONDUCT.md}
⚙️ CodeRabbit configuration file
{docs/**,README.md,CONTRIBUTING.md,SECURITY.md,CODE-OF-CONDUCT.md}: Review documentation for command accuracy, branch-name consistency, current CI and copy-pr-bot behavior, public
vs internal boundary clarity, stale examples, and links that no longer match the repository layout.
Files:
README.md
**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run ruff check and ruff format validation for Python code changes
**/*.py: Python code must be linted and formatted with Ruff using line length 120, target Python 3.11, rule sets E,F,W,I,PL,UP, and isort force-single-line configuration
Never commit secrets, tokens, or environment-specific hostnames; use environment variables and SecretStr instead, resolving API keys at runtime
Never print or log secret values, including in tool output or error messages
Missing-secret paths must degrade gracefully (stub/skip), not crash or leak
Do not hand-reformat unrelated code when making changes; match the existing import and formatting style
Files:
sources/google_scholar_paper_search/src/register.pysources/google_scholar_paper_search/tests/test_paper_search.pysources/google_scholar_paper_search/tests/conftest.pysources/google_scholar_paper_search/src/paper_search.py
sources/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
New tools and data sources must be NAT functions registered with
@register_functiondecorator
Files:
sources/google_scholar_paper_search/src/register.pysources/google_scholar_paper_search/tests/test_paper_search.pysources/google_scholar_paper_search/tests/conftest.pysources/google_scholar_paper_search/src/paper_search.py
**/*test*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run pytest for all behavior changes in Python code
Files:
sources/google_scholar_paper_search/tests/test_paper_search.pysources/google_scholar_paper_search/tests/conftest.py
🔇 Additional comments (9)
README.md (1)
109-109: LGTM!Also applies to: 209-219
deploy/.env.example (1)
23-28: LGTM!sources/google_scholar_paper_search/README.md (2)
13-90: LGTM!Prerequisites, YAML configuration examples (lines 49–79), and configuration options table are all correct and well-structured. Each provider example correctly pairs
provider: {serper|serpapi|searchapi}with its matching API key field.
113-113: LGTM!Complete example, troubleshooting section, and disabling instructions all accurately reflect provider-aware configuration and correctly reference the matching env var for the chosen provider.
Also applies to: 126-128, 164-166, 181-185, 196-196
sources/google_scholar_paper_search/tests/conftest.py (1)
23-53: LGTM!Also applies to: 79-156
sources/google_scholar_paper_search/tests/test_paper_search.py (1)
23-853: LGTM!sources/google_scholar_paper_search/src/paper_search.py (1)
15-105: LGTM!Also applies to: 153-215, 224-294, 329-381, 415-466
sources/google_scholar_paper_search/src/register.py (1)
30-79: LGTM!Also applies to: 86-126, 132-160
sources/google_scholar_paper_search/pyproject.toml (1)
26-33: LGTM!
Sanitize error handling and fix key resolution per review: - paper_search.py: remove raw response body from provider exceptions (Serper/SerpAPI/SearchAPI) to prevent API key leakage via error messages; top-level search() handler now returns a generic message without echoing exception text - register.py: _resolve_api_key no longer persists SecretStr values to os.environ (avoids cross-workflow secret leakage); tool constructor now uses the resolved api_key instead of reading config fields, which fixes a bug where env-only setups would pass None to the provider - tests: update error-handling assertions to verify sanitized output (assert raw exception text is NOT present) - README: fix table header from '_type value' to 'provider value' Signed-off-by: Gabriel Costa <gcasilva@outlook.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
sources/google_scholar_paper_search/tests/test_paper_search.py (1)
811-822: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winStrengthen SearchAPI dispatch test to enforce exclusive routing
At Line 811, this test only asserts
_search_searchapiis called. It should also assert_search_serperand_search_serpapiare not called, so regressions that fan out to multiple providers are caught.Suggested patch
@@ async def test_search_dispatches_to_searchapi(self, searchapi_tool): """Test that search() dispatches to _search_searchapi.""" - with patch.object( - searchapi_tool, - "_search_searchapi", - new_callable=AsyncMock, - return_value=[], - ) as mock_searchapi: + with ( + patch.object( + searchapi_tool, + "_search_searchapi", + new_callable=AsyncMock, + return_value=[], + ) as mock_searchapi, + patch.object( + searchapi_tool, + "_search_serper", + new_callable=AsyncMock, + ) as mock_serper, + patch.object( + searchapi_tool, + "_search_serpapi", + new_callable=AsyncMock, + ) as mock_serpapi, + ): await searchapi_tool.search("test query") mock_searchapi.assert_called_once() + mock_serper.assert_not_called() + mock_serpapi.assert_not_called()🤖 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 `@sources/google_scholar_paper_search/tests/test_paper_search.py` around lines 811 - 822, The test_search_dispatches_to_searchapi method currently only verifies that _search_searchapi is called but does not verify that other search provider methods are NOT called. Patch and mock both _search_serper and _search_serpapi methods in the same way as _search_searchapi, then add assertions after the search call to verify that these alternative provider methods are not called (using assert_not_called on each mock). This ensures the dispatch correctly routes only to SearchAPI and prevents regressions that might accidentally fan out to multiple providers.sources/google_scholar_paper_search/src/register.py (1)
110-113:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep the missing-key stub’s
yearschema aligned with the real tool.Line 112 narrows the stub to
str | None, while the real_paper_searchacceptsstr | int | NoneandPaperSearchTool.search()handles non-string years. In missing-key mode, an otherwise valid integer year can be rejected before the graceful unavailable message is returned.Proposed fix
async def _paper_search_stub( query: str = Field(..., validation_alias=AliasChoices("query", "question")), - year: str | None = None, + year: str | int | None = None, ) -> str:As per coding guidelines, “Missing-secret paths must degrade gracefully (stub/skip), not crash or leak.”
🤖 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 `@sources/google_scholar_paper_search/src/register.py` around lines 110 - 113, The _paper_search_stub function has a narrower type annotation for the year parameter (str | None) compared to the real _paper_search function (str | int | None). This causes valid integer years to be rejected by the schema validation before the graceful unavailable message can be returned. Update the year parameter type in _paper_search_stub from str | None to str | int | None to match the real tool's signature and ensure consistent handling of both string and integer year inputs in missing-key mode.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@sources/google_scholar_paper_search/src/register.py`:
- Around line 110-113: The _paper_search_stub function has a narrower type
annotation for the year parameter (str | None) compared to the real
_paper_search function (str | int | None). This causes valid integer years to be
rejected by the schema validation before the graceful unavailable message can be
returned. Update the year parameter type in _paper_search_stub from str | None
to str | int | None to match the real tool's signature and ensure consistent
handling of both string and integer year inputs in missing-key mode.
In `@sources/google_scholar_paper_search/tests/test_paper_search.py`:
- Around line 811-822: The test_search_dispatches_to_searchapi method currently
only verifies that _search_searchapi is called but does not verify that other
search provider methods are NOT called. Patch and mock both _search_serper and
_search_serpapi methods in the same way as _search_searchapi, then add
assertions after the search call to verify that these alternative provider
methods are not called (using assert_not_called on each mock). This ensures the
dispatch correctly routes only to SearchAPI and prevents regressions that might
accidentally fan out to multiple providers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 643c7133-89ac-48fd-b4c5-b37e76419851
📒 Files selected for processing (4)
sources/google_scholar_paper_search/README.mdsources/google_scholar_paper_search/src/paper_search.pysources/google_scholar_paper_search/src/register.pysources/google_scholar_paper_search/tests/test_paper_search.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run ruff check and ruff format validation for Python code changes
**/*.py: Python code must be linted and formatted with Ruff using line length 120, target Python 3.11, rule sets E,F,W,I,PL,UP, and isort force-single-line configuration
Never commit secrets, tokens, or environment-specific hostnames; use environment variables and SecretStr instead, resolving API keys at runtime
Never print or log secret values, including in tool output or error messages
Missing-secret paths must degrade gracefully (stub/skip), not crash or leak
Do not hand-reformat unrelated code when making changes; match the existing import and formatting style
Files:
sources/google_scholar_paper_search/src/register.pysources/google_scholar_paper_search/tests/test_paper_search.pysources/google_scholar_paper_search/src/paper_search.py
sources/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
New tools and data sources must be NAT functions registered with
@register_functiondecorator
Files:
sources/google_scholar_paper_search/src/register.pysources/google_scholar_paper_search/tests/test_paper_search.pysources/google_scholar_paper_search/src/paper_search.py
{src/aiq_agent/knowledge/**,sources/**}
⚙️ CodeRabbit configuration file
{src/aiq_agent/knowledge/**,sources/**}: Review data-source and knowledge-layer changes for optional dependency boundaries, external API error handling,
retry/rate-limit behavior, deterministic tests, and registration consistency. New source packages should include
package metadata, plugin registration when applicable, and source-level tests.
Files:
sources/google_scholar_paper_search/src/register.pysources/google_scholar_paper_search/tests/test_paper_search.pysources/google_scholar_paper_search/README.mdsources/google_scholar_paper_search/src/paper_search.py
**
⚙️ CodeRabbit configuration file
**:AI-Q Agent Guidance
Repository-global instructions for coding agents and for humans reviewing
agent-authored changes. These rules apply to every task in this repository.
Task-specific runbooks live in.agents/skills/— load the
relevant skill before starting a workflow it covers.Project overview
AI-Q is an NVIDIA AI Blueprint: an enterprise research agent built on the
NeMo Agent Toolkit (NAT). The deployed product is a research blueprint, not
a general skill runtime. New retrieval sources and tools are NAT functions;
agent behavior is driven by workflow YAML, Jinja2 prompts, and a data-source
registry — not by hard-coded logic.Primary boundaries:
- Backend Python package:
src/aiq_agent/.- Data-source and tool packages:
sources/(each is its own package).- Frontends and tooling:
frontends/(web UI infrontends/ui/, eval harnesses
infrontends/benchmarks/).- Configs, deployment, docs:
configs/,deploy/,docs/.Stay inside this repository. If your workspace also contains adjacent repos
(for example a sibling NeMo-Relay checkout), do not edit them as part of an AI-Q
change. Treatsources/*as independent packages: prefer the smallest change
scoped to the package you are touching.Repository structure
Path Purpose src/aiq_agent/Backend agent, FastAPI extensions, auth, observability, knowledge sources/Data-source / tool packages (e.g. tavily_web_search,google_scholar_paper_search)configs/Workflow YAML configs (e.g. config_cli_default.yml)frontends/ui/Next.js / React / TypeScript / Tailwind / KUI web UI frontends/benchmarks/Eval harnesses: freshqa,deepsearch_qa,deepresearch_benchdeploy/Docker Compose and Helm/Kubernetes assets; deploy/.envfor secretsdocs/source/...
Files:
sources/google_scholar_paper_search/src/register.pysources/google_scholar_paper_search/tests/test_paper_search.pysources/google_scholar_paper_search/README.mdsources/google_scholar_paper_search/src/paper_search.py
**/*test*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run pytest for all behavior changes in Python code
Files:
sources/google_scholar_paper_search/tests/test_paper_search.py
🔇 Additional comments (7)
sources/google_scholar_paper_search/README.md (2)
5-5: The table header issue from the prior review has been resolved. Line 5 now correctly shows| Provider |providervalue | Env var | Sign-up |, accurately reflecting that the column contains theproviderfield values (serper,serpapi,searchapi), not_typevalues.
3-26: LGTM!The documentation correctly documents the multi-provider feature with accurate provider identifiers, environment variable names, configuration field names, and examples that all align with the upstream PaperSearchToolConfig schema and registration contracts.
Also applies to: 49-90, 113-145, 154-166, 181-198
sources/google_scholar_paper_search/src/paper_search.py (3)
149-151: Previous sanitization concern still applies to exception logging.
logger.exception(...)still records exception details; keep provider-failure logs sanitized rather than emitting traceback text from lower-level HTTP/client exceptions.Proposed adjustment
except Exception: - logger.exception("Paper search failed for provider '%s'", self.provider.value) + logger.error("Paper search failed for provider '%s'", self.provider.value) return f"Paper search failed: unable to fetch results from {self.provider.value}."
76-145: LGTM!
153-463: LGTM!sources/google_scholar_paper_search/src/register.py (2)
46-88: LGTM!
130-163: LGTM!
- register.py: align stub year type to str | int | None to match the real _paper_search signature, so integer years aren't rejected by schema validation before the graceful unavailable message - paper_search.py: use logger.error instead of logger.exception to avoid logging tracebacks that could include the request URL (which carries the api_key query param for SerpAPI/SearchAPI) - tests: strengthen test_search_dispatches_to_searchapi to assert _search_serper and _search_serpapi are not called, matching the SerpAPI dispatch test pattern Signed-off-by: Gabriel Costa <gcasilva@outlook.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sources/google_scholar_paper_search/src/paper_search.py (1)
107-112:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winType annotation for
yearshould bestr | int | None.The method accepts and converts integer years at lines 130-131, but the signature declares
year: str | None. This misaligns with callers inregister.pythat passstr | int | None. Update the annotation for consistency and static analysis accuracy.async def search( self, query: str, - year: str | None = None, + year: str | int | None = None, ) -> str:🤖 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 `@sources/google_scholar_paper_search/src/paper_search.py` around lines 107 - 112, The type annotation for the year parameter in the search method is incomplete and does not match the actual accepted types or how callers use it. Update the type annotation for the year parameter in the search method signature from str | None to str | int | None to accurately reflect that the method handles both string and integer year values, as evidenced by the conversion logic at lines 130-131 and usage patterns from callers in register.py.
🤖 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.
Outside diff comments:
In `@sources/google_scholar_paper_search/src/paper_search.py`:
- Around line 107-112: The type annotation for the year parameter in the search
method is incomplete and does not match the actual accepted types or how callers
use it. Update the type annotation for the year parameter in the search method
signature from str | None to str | int | None to accurately reflect that the
method handles both string and integer year values, as evidenced by the
conversion logic at lines 130-131 and usage patterns from callers in
register.py.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 58412864-adea-4646-a5c5-eef333972d8d
📒 Files selected for processing (3)
sources/google_scholar_paper_search/src/paper_search.pysources/google_scholar_paper_search/src/register.pysources/google_scholar_paper_search/tests/test_paper_search.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run ruff check and ruff format validation for Python code changes
**/*.py: Python code must be linted and formatted with Ruff using line length 120, target Python 3.11, rule sets E,F,W,I,PL,UP, and isort force-single-line configuration
Never commit secrets, tokens, or environment-specific hostnames; use environment variables and SecretStr instead, resolving API keys at runtime
Never print or log secret values, including in tool output or error messages
Missing-secret paths must degrade gracefully (stub/skip), not crash or leak
Do not hand-reformat unrelated code when making changes; match the existing import and formatting style
Files:
sources/google_scholar_paper_search/src/register.pysources/google_scholar_paper_search/src/paper_search.pysources/google_scholar_paper_search/tests/test_paper_search.py
sources/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
New tools and data sources must be NAT functions registered with
@register_functiondecorator
Files:
sources/google_scholar_paper_search/src/register.pysources/google_scholar_paper_search/src/paper_search.pysources/google_scholar_paper_search/tests/test_paper_search.py
{src/aiq_agent/knowledge/**,sources/**}
⚙️ CodeRabbit configuration file
{src/aiq_agent/knowledge/**,sources/**}: Review data-source and knowledge-layer changes for optional dependency boundaries, external API error handling,
retry/rate-limit behavior, deterministic tests, and registration consistency. New source packages should include
package metadata, plugin registration when applicable, and source-level tests.
Files:
sources/google_scholar_paper_search/src/register.pysources/google_scholar_paper_search/src/paper_search.pysources/google_scholar_paper_search/tests/test_paper_search.py
**
⚙️ CodeRabbit configuration file
**:AI-Q Agent Guidance
Repository-global instructions for coding agents and for humans reviewing
agent-authored changes. These rules apply to every task in this repository.
Task-specific runbooks live in.agents/skills/— load the
relevant skill before starting a workflow it covers.Project overview
AI-Q is an NVIDIA AI Blueprint: an enterprise research agent built on the
NeMo Agent Toolkit (NAT). The deployed product is a research blueprint, not
a general skill runtime. New retrieval sources and tools are NAT functions;
agent behavior is driven by workflow YAML, Jinja2 prompts, and a data-source
registry — not by hard-coded logic.Primary boundaries:
- Backend Python package:
src/aiq_agent/.- Data-source and tool packages:
sources/(each is its own package).- Frontends and tooling:
frontends/(web UI infrontends/ui/, eval harnesses
infrontends/benchmarks/).- Configs, deployment, docs:
configs/,deploy/,docs/.Stay inside this repository. If your workspace also contains adjacent repos
(for example a sibling NeMo-Relay checkout), do not edit them as part of an AI-Q
change. Treatsources/*as independent packages: prefer the smallest change
scoped to the package you are touching.Repository structure
Path Purpose src/aiq_agent/Backend agent, FastAPI extensions, auth, observability, knowledge sources/Data-source / tool packages (e.g. tavily_web_search,google_scholar_paper_search)configs/Workflow YAML configs (e.g. config_cli_default.yml)frontends/ui/Next.js / React / TypeScript / Tailwind / KUI web UI frontends/benchmarks/Eval harnesses: freshqa,deepsearch_qa,deepresearch_benchdeploy/Docker Compose and Helm/Kubernetes assets; deploy/.envfor secretsdocs/source/...
Files:
sources/google_scholar_paper_search/src/register.pysources/google_scholar_paper_search/src/paper_search.pysources/google_scholar_paper_search/tests/test_paper_search.py
**/*test*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run pytest for all behavior changes in Python code
Files:
sources/google_scholar_paper_search/tests/test_paper_search.py
🔇 Additional comments (10)
sources/google_scholar_paper_search/tests/test_paper_search.py (1)
62-66: LGTM!Also applies to: 68-73, 75-80, 82-86, 181-192, 813-835
sources/google_scholar_paper_search/src/paper_search.py (6)
76-104: LGTM!
146-182: LGTM!
184-213: LGTM!
216-293: LGTM!
295-378: LGTM!
380-463: LGTM!sources/google_scholar_paper_search/src/register.py (3)
46-69: LGTM!
92-128: LGTM!
130-163: LGTM!
|
@AjayThorve What is needed to run the other validations for this PR? Thanks |
|
/ok to test b1d95de |
Mark the new SerpAPI/SearchAPI placeholder and test-fixture api-key strings with inline '# pragma: allowlist secret' comments (the repo's established convention, 29 existing usages incl. docs/source/extending/adding-a-tool.md). Refresh .secrets.baseline line numbers shifted by this PR. None of the flagged strings are real secrets. Signed-off-by: Gabriel Costa <gcasilva@outlook.com>
|
@AjayThorve there were false positives related to secret strings used in the google scholar testing that were being caught on the lint validation test that I've fixed with the # pragma: allowlist secret in the last commit. Can you please re-run /ok to test |
|
@AjayThorve can you please re-run /ok to test |
|
Hi @AjayThorve, following up to re-run /ok to test, thanks! |
|
Hi @AjayThorve, following up to re-run /ok to test, all tests executed successfully locally, so we just need to re-run this on the pipeline as well. Appreciate your support so we can provide these additional Google Scholar backends for developers! Thanks! |
|
/ok to test 047538f |
|
gcasilva can you resolve the conflicts? |
AjayThorve
left a comment
AjayThorve
left a comment
There was a problem hiding this comment.
Two findings from current-head code inspection and local runtime validation.
| """ | ||
| if not isinstance(publication_info, str): | ||
| return "Unknown Year" | ||
| match = _YEAR_RE.search(publication_info) |
There was a problem hiding this comment.
[P1] Parse the actual publication year, not the first four-digit token.
_YEAR_RE.search() selects the first 19xx/20xx occurrence. With an official SerpAPI-shaped summary such as Science Progress in the Twentieth Century (1919-1933 …, 1926 - JSTOR, this normalizes the publication year to 1919 instead of 1926; it also forces every pre-1900 paper to Unknown Year. That violates the advertised common result shape and feeds factually incorrect metadata to the agent. Please parse the provider's publication-year position—typically the final year before the source suffix—with a defensive fallback, and add tests covering embedded date ranges, arXiv identifiers, and pre-1900 publications.
There was a problem hiding this comment.
Addressed in b078d1f.
_extract_year now takes the final year token before the source/host suffix instead of the first 19xx/20xx match, so a summary like (1919-1933, 1926 - JSTOR) now yields 1926 instead of 1919. The year window is widened from 19xx/20xx to 1500-2099 so pre-1900 works are no longer forced to Unknown Year (e.g. 1687 and 1859 now resolve). A negative lookahead also prevents an arXiv id such as 1910.12345 from matching as the year 1910.
Added tests covering date ranges, arXiv identifiers (ignored in favor of the real year, or Unknown when no year is present), and pre-1900 publications. Full suite: 64 passed.
| async def _paper_search_stub( | ||
| query: str = Field(..., validation_alias=AliasChoices("query", "question")), | ||
| year: str | None = None, | ||
| year: str | int | None = None, |
There was a problem hiding this comment.
[P2] Preserve a meaningful description for the missing-key stub.
The stub has no docstring, but FunctionInfo.from_fn() receives _paper_search_stub.__doc__, so the resulting FunctionInfo.description is None. I reproduced NAT warning that it falls back to the instance name paper_search_tool, weakening tool selection in the graceful-degradation path. Please pass an explicit provider-aware description (or restore a useful docstring) and add a registration-level test asserting the missing-key function's description.
There was a problem hiding this comment.
Addressed in b078d1f.
The missing-key stub now passes an explicit provider-aware description to FunctionInfo.from_fn instead of _paper_search_stub.doc (which was None). The description names the provider and the unconfigured env var, so tool selection no longer falls back to the instance name.
Added TestRegisterMissingKeyStub, parametrized over all three providers, asserting the FunctionInfo description is non-None and contains both the provider value and the missing env var.
Extract the publication year as the final year token before the source suffix instead of the first 19xx/20xx match, so date ranges and pre-1900 papers no longer yield a wrong or Unknown year. Widen the year window to 1500-2099 and reject arXiv-id leading digits via a negative lookahead. Give the missing-API-key stub an explicit provider-aware description so FunctionInfo.description is non-None in the graceful-degradation path. Adds year-parsing tests (date ranges, arXiv ids, pre-1900) and a registration-level test asserting the stub description. Signed-off-by: Gabriel Costa <gcasilva@outlook.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
sources/google_scholar_paper_search/src/paper_search.py (2)
136-151: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not log or return raw search queries.
queryis user-controlled and may contain secrets or customer data; keep provider/status in logs and timeout output without echoing the full query. As per coding guidelines, "Never print or log secret values, including in tool output or error messages."Proposed fix
- logger.info(f"Paper search ({self.provider.value}) for: {query}") + logger.info("Paper search (%s) started", self.provider.value) @@ - logger.error(f"Paper search timed out for query: {query}") - return f"Paper search timed out after {self.timeout}s for query: {query}" + logger.error("Paper search timed out for provider '%s'", self.provider.value) + return f"Paper search timed out after {self.timeout}s."🤖 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 `@sources/google_scholar_paper_search/src/paper_search.py` around lines 136 - 151, The timeout path in paper_search.py is echoing the user-controlled query in both the logger.error call and the returned message, which can leak secrets or customer data. Update PaperSearch.search (and any related timeout handling) to log only the provider/status and omit the raw query, and change the timeout return string to a generic message that references the timeout duration without including query contents.Source: Coding guidelines
139-146: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSkip requests when the selected provider key is missing.
Registration stubs handle missing keys, but direct
PaperSearchToolusage still sends requests with empty credentials. Add a selected-key guard before dispatch so missing-secret paths fail locally without calling external APIs. As per coding guidelines, "Missing-secret paths must degrade gracefully (stub/skip), not crash or leak."Proposed fix
+ selected_api_key = { + PaperSearchProvider.SERPER: self.serper_api_key, + PaperSearchProvider.SERPAPI: self.serpapi_api_key, + PaperSearchProvider.SEARCHAPI: self.searchapi_api_key, + }[self.provider] + if not selected_api_key: + logger.warning("Paper search skipped because provider '%s' is missing an API key", self.provider.value) + return f"Paper search is unavailable because the {self.provider.value} API key is not configured." + try:Also applies to: 246-246, 319-319, 404-404
🤖 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 `@sources/google_scholar_paper_search/src/paper_search.py` around lines 139 - 146, Missing provider credentials are still allowing direct PaperSearchTool searches to call external APIs instead of skipping locally. Add a selected-key guard in PaperSearchTool’s dispatch flow before the provider switch in the search method so that when the chosen provider secret is absent, the request is short-circuited and handled gracefully without invoking _search_serper, _search_serpapi, or _search_searchapi. Apply the same selected-key check to the other affected call sites so missing-secret paths degrade as a stub/skip rather than making a network request.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@sources/google_scholar_paper_search/src/paper_search.py`:
- Around line 136-151: The timeout path in paper_search.py is echoing the
user-controlled query in both the logger.error call and the returned message,
which can leak secrets or customer data. Update PaperSearch.search (and any
related timeout handling) to log only the provider/status and omit the raw
query, and change the timeout return string to a generic message that references
the timeout duration without including query contents.
- Around line 139-146: Missing provider credentials are still allowing direct
PaperSearchTool searches to call external APIs instead of skipping locally. Add
a selected-key guard in PaperSearchTool’s dispatch flow before the provider
switch in the search method so that when the chosen provider secret is absent,
the request is short-circuited and handled gracefully without invoking
_search_serper, _search_serpapi, or _search_searchapi. Apply the same
selected-key check to the other affected call sites so missing-secret paths
degrade as a stub/skip rather than making a network request.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: f0ab4c85-aa73-49e6-a479-3a144396c960
📒 Files selected for processing (3)
sources/google_scholar_paper_search/src/paper_search.pysources/google_scholar_paper_search/src/register.pysources/google_scholar_paper_search/tests/test_paper_search.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run ruff check and ruff format validation for Python code changes
**/*.py: Python code must be linted and formatted with Ruff using line length 120, target Python 3.11, rule sets E,F,W,I,PL,UP, and isort force-single-line configuration
Never commit secrets, tokens, or environment-specific hostnames; use environment variables and SecretStr instead, resolving API keys at runtime
Never print or log secret values, including in tool output or error messages
Missing-secret paths must degrade gracefully (stub/skip), not crash or leak
Do not hand-reformat unrelated code when making changes; match the existing import and formatting style
Files:
sources/google_scholar_paper_search/src/register.pysources/google_scholar_paper_search/tests/test_paper_search.pysources/google_scholar_paper_search/src/paper_search.py
sources/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
New tools and data sources must be NAT functions registered with
@register_functiondecorator
Files:
sources/google_scholar_paper_search/src/register.pysources/google_scholar_paper_search/tests/test_paper_search.pysources/google_scholar_paper_search/src/paper_search.py
**
⚙️ CodeRabbit configuration file
**:AI-Q Agent Guidance
Repository-global instructions for coding agents and for humans reviewing
agent-authored changes. These rules apply to every task in this repository.
Task-specific runbooks live in.agents/skills/— load the
relevant skill before starting a workflow it covers.Project overview
AI-Q is an NVIDIA AI Blueprint: an enterprise research agent built on the
NeMo Agent Toolkit (NAT). The deployed product is a research blueprint, not
a general skill runtime. New retrieval sources and tools are NAT functions;
agent behavior is driven by workflow YAML, Jinja2 prompts, and a data-source
registry — not by hard-coded logic.Primary boundaries:
- Backend Python package:
src/aiq_agent/.- Data-source and tool packages:
sources/(each is its own package).- Frontends and tooling:
frontends/(web UI infrontends/ui/, eval harnesses
infrontends/benchmarks/).- Configs, deployment, docs:
configs/,deploy/,docs/.Stay inside this repository. If your workspace also contains adjacent repos
(for example a sibling NeMo-Relay checkout), do not edit them as part of an AI-Q
change. Treatsources/*as independent packages: prefer the smallest change
scoped to the package you are touching.Repository structure
Path Purpose src/aiq_agent/Backend agent, FastAPI extensions, auth, observability, knowledge sources/Data-source / tool packages (e.g. tavily_web_search,google_scholar_paper_search)configs/Workflow YAML configs (e.g. config_cli_default.yml)frontends/ui/Next.js / React / TypeScript / Tailwind / KUI web UI frontends/benchmarks/Eval harnesses: freshqa,deepsearch_qa,deepresearch_benchdeploy/Docker Compose and Helm/Kubernetes assets; deploy/.envfor secretsdocs/source/...
Files:
sources/google_scholar_paper_search/src/register.pysources/google_scholar_paper_search/tests/test_paper_search.pysources/google_scholar_paper_search/src/paper_search.py
{src/aiq_agent/knowledge/**,sources/**}
⚙️ CodeRabbit configuration file
{src/aiq_agent/knowledge/**,sources/**}: Review data-source and knowledge-layer changes for optional dependency boundaries, external API error handling,
retry/rate-limit behavior, deterministic tests, and registration consistency. New source packages should include
package metadata, plugin registration when applicable, and source-level tests.
Files:
sources/google_scholar_paper_search/src/register.pysources/google_scholar_paper_search/tests/test_paper_search.pysources/google_scholar_paper_search/src/paper_search.py
**/*test*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run pytest for all behavior changes in Python code
Files:
sources/google_scholar_paper_search/tests/test_paper_search.py
🪛 ast-grep (0.44.0)
sources/google_scholar_paper_search/src/paper_search.py
[warning] 221-221: XPath query is request-/variable-derived; use parameterized XPath to prevent injection.
Context: _YEAR_RE.findall(summary)
Note: [CWE-643] Improper Neutralization of Data within XPath Expressions ('XPath Injection').
(xpath-injection-python)
🔇 Additional comments (10)
sources/google_scholar_paper_search/src/paper_search.py (1)
37-41: LGTM!Also applies to: 79-135, 152-223, 234-303, 305-473
sources/google_scholar_paper_search/src/register.py (1)
46-69: LGTM!Also applies to: 92-168
sources/google_scholar_paper_search/tests/test_paper_search.py (8)
22-22: LGTM!Also applies to: 63-88
182-193: LGTM! Correctly verifies the sanitized error message per the earlier fix (no longer leaks raw exception text).
480-483: LGTM!
486-674: LGTM! Normalization and pagination tests correctly exercise both SerpAPI and SearchAPI response shapes, including default/missing-field fallbacks.
675-789: LGTM! Request-parameter tests correctly assert provider-specific auth placement (header vs query param) and pagination conventions (0-based vs 1-based).
790-852: LGTM! Dispatch tests correctly assert mutual exclusivity of provider routing.
899-926: LGTM! Registration test correctly resets the module-level warn guard and validates the stub description is non-Noneand provider/env-var aware, addressing the commit's stated fix forFunctionInfo.descriptionnever beingNone.
461-479: 🎯 Functional CorrectnessNo issue: arXiv IDs are excluded from year matching.
_extract_yeardrops the trailing host suffix, and_YEAR_RErejects the1910.12345form, so arXiv-only summaries still returnUnknown Year.> Likely an incorrect or invalid review comment.
|
@gcasilva This is a good extension to the existing paper_search source. I’m comfortable with the provider-selection surface here, especially since Serper remains the default and existing configs keep working. Two things before merge:
If we don’t have CI credentials, I’m still okay merging this as a best-effort/community-supported provider extension, provided the docs make that maintenance boundary clear. The unit coverage is strong enough for the normalization/config surface; live provider CI would be the bar for us owning provider drift. |
Thanks @AjayThorve , on bullet point 2, not sure how I would be able to provide CI credentials for SerpAPI/SearchAPI, as I'm not part of those companies, I'm just an individual developer so I cannot provide my own credentials for your CI to use for testing, how have this been resolved for Serper? Can't you use the same process? |
The TimeoutError handler no longer logs or returns the raw query; it reports only the timeout duration and provider, so an input query is not echoed into logs or tool output on a timeout. Direct PaperSearchTool construction now short-circuits when the selected provider's API key is missing, returning a provider-named unavailable message instead of dispatching an upstream call with an empty credential. This matches the register stub's graceful-degradation path. Adds a regression assertion that the query is absent from the timeout result, a parametrized short-circuit test across all three providers, and a no-dispatch test confirming _search_* is never reached. Signed-off-by: Gabriel Costa <gcasilva@outlook.com>
|
@AjayThorve Both direct-tool edge cases from bullet 1 are addressed in cde2c3f.
Tests: added a regression assertion that the query is absent from the timeout result, a parametrized short-circuit test across all three providers, and a no-dispatch test confirming |
2 participants
Overview
This PR adds a
providerconfig field to the Google Scholar paper search tool (sources/google_scholar_paper_search/) so developers can choose between three backend APIs:providervalueserperSERPER_API_KEYserpapiSERPAPI_API_KEYsearchapiSEARCHAPI_API_KEYAll three query Google Scholar and return results normalized to the same shape (title, year, snippet, link, publication info, citations), so the agent-facing tool behavior is identical regardless of provider.
Why: The tool was previously hard-wired to Serper. Organizations that already have SerpAPI or SearchAPI subscriptions — or that need provider-specific features — can now use their preferred backend without forking the tool.
Backward compatibility:
providerdefaults toserper, the existingserper_api_keyconfig field andSERPER_API_KEYenv var path are unchanged, and all existing workflow configs work without modification.Bonus fix:
pyproject.tomllistedhttpxas a dependency but the code importsaiohttp. Fixed to declareaiohttp>=3.8.0.Validation
Ran the narrowest commands covering the changed package, plus full-repo lint:
Lint (whole repo):
Tests (changed package):
Test coverage expanded from 21 → 58 tests, covering:
Provider init, enum dispatch, and backward-compatible defaults
Year extraction from publication strings (SerpAPI/SearchAPI embed the year in the summary)
Response normalization for SerpAPI (
publication_info.summary,inline_links.cited_by.total) and SearchAPI (publication,inline_links.cited_by.total)Fetch payload construction per provider (POST vs GET,
X-API-KEYheader vsapi_keyquery param)Pagination including SearchAPI's 1-based
pageparameter (vs Serper/SerpAPIstartoffset)End-to-end
search()success and error handling per providerI ran the relevant local checks or explained why they are not applicable.
I added or updated tests for behavior changes.
I updated documentation for user-facing or contributor-facing changes.
I confirmed this PR does not include secrets, credentials, or internal-only data.
I certify this contribution under the Developer Certificate of Origin (DCO) and signed my commits with
git commit -sor an equivalent sign-off.Where should reviewers start?
sources/google_scholar_paper_search/src/paper_search.py— thePaperSearchProviderenum (line 41) and thesearch()dispatch method (line 107) show the design._normalize_serpapi(line 363) and_normalize_searchapi(line 448) show how each provider's raw response maps to the common shape consumed byformat_results.The key design decision was a single
_type: paper_searchwith an enumproviderfield rather than three separate registered types (paper_search_serper/serpapi/searchapi). This keeps every existing config working unchanged while adding the provider choice as an opt-in field.Related Issues
Summary by CodeRabbit