feat(aiq-3.0): add initial version of GSF as a data source - #427
Conversation
WalkthroughAdds the ChangesGSF data source
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant gsf_function_group
participant GSFClient
participant GSFHTTPAPI
Caller->>gsf_function_group: invoke catalog_search or text_to_sql
gsf_function_group->>GSFClient: resolve authentication and submit request
GSFClient->>GSFHTTPAPI: send authenticated HTTP request
GSFHTTPAPI-->>GSFClient: return JSON or SSE response
GSFClient-->>gsf_function_group: return typed response or GSFError
gsf_function_group-->>Caller: return JSON result or GSFToolError
Possibly related PRs
🚥 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: 20
🤖 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 @.agents/skills/aiq-configure-workflow/scripts/validate_config.py:
- Around line 232-235: Update the function_groups handling in the configuration
validator to inspect the raw data.get("function_groups") value before applying
any empty fallback, and report a validation error or reject every non-mapping
value, including empty lists and scalars. Preserve valid mapping behavior and
add regression tests covering valid groups plus invalid list and scalar
declarations; run the requested Ruff checks and pytest.
In `@sources/gsf/pyproject.toml`:
- Around line 4-6: Update the [build-system] requires declaration in
pyproject.toml to remove the unused setuptools-scm>=8 dependency while
preserving setuptools >= 83 and the existing static version configuration.
In `@sources/gsf/README.md`:
- Line 28: Update the public README examples’ GSF_BASE_URL values to remove the
internal gsf hostname, using an unset-variable form or a neutral local-only
endpoint consistently at both occurrences.
- Around line 48-55: Update the password-auth lifecycle wording in the GSF
README around the local-development configuration and password-mode explanation
to match GSFClient.from_config: state that the session is created when the
function group/client is initialized, not only when a tool is invoked. Keep the
environment-variable credential setup and configuration example unchanged.
- Around line 81-85: Remove the documented database-scoping guarantee from the
catalog search flow and reject any non-empty database_name before calling the
GSF question-entity-coverage endpoint. Update the relevant request or validation
logic to fail clearly, revise the README contract accordingly, and add a
regression test covering rejection of database-scoped searches.
In `@sources/gsf/src/gsf/client.py`:
- Around line 34-35: Update GSFClient.__init__ and GSFFunctionGroupConfig to
accept a max_connections setting defaulting to 20, create explicit httpx.Limits
using that value, and pass the limits when constructing AsyncClient so the
documented shared pool is bounded.
- Around line 437-441: Update the response parsing assignments in the client
method containing _normalize_rows and _normalize_columns to use truthiness-based
fallback between the primary and alias keys, so null or empty primary values
consult sql_response_from_db and sql_columns. Preserve the existing
normalization, truncation, and column-processing behavior.
- Around line 262-300: Update _sign_in_with_password to send the same Origin and
Referer headers as _sign_out_password_session, using self._base_url for both
values. Preserve the existing sign-in error handling and request payload, and
update the relevant mock transport assertion so the sign-in headers are
verified.
- Line 26: Define the forwarded-header allowlist once and reuse it in both
filters: in sources/gsf/src/gsf/client.py:26, rename _FORWARDED_HEADER_NAMES to
the shared public name FORWARDED_HEADER_NAMES while retaining the existing set;
in sources/gsf/src/gsf/register.py:33, remove _TRACE_HEADER_NAMES and import and
use FORWARDED_HEADER_NAMES from the client module.
- Around line 225-232: Update the retry handling around _http_error to honor a
present Retry-After header for 429 responses, clamping the parsed delay to 30
seconds; fall back to exponential backoff with randomized jitter, also capped at
30 seconds, via a _retry_delay helper and the required random import. Pass the
response header into the delay calculation before asyncio.sleep, and update
test_client_retries_rate_limit_then_succeeds to assert the new delay behavior.
- Around line 349-364: Replace str.splitlines() in the SSE parsing loop with
splitting logic that recognizes only CR, LF, and CRLF terminators. Preserve the
existing handling of blank lines, comments, data accumulation, and event parsing
while ensuring U+2028, U+2029, and other non-SSE separators remain inside data
lines.
- Around line 238-246: Restrict the retry branch in the HTTP timeout handling
around the capability request to httpx.ConnectTimeout only, so httpx.ReadTimeout
is immediately converted to the existing GSFError without resending
chat/completions. Add a regression test covering a read timeout and verifying no
retry occurs.
In `@sources/gsf/src/gsf/models.py`:
- Around line 43-51: Update CatalogSearchResponse.coverage to tolerate an
omitted upstream value by assigning a default of 0.0 while preserving its
non-nullable float type and existing ge=0/le=1 constraints, so
_normalize_catalog_search can still return candidates when coverage is missing.
In `@sources/gsf/src/gsf/register.py`:
- Around line 144-153: The query_context tool is registered despite always
returning CAPABILITY_UNAVAILABLE. Remove its registration and associated
schema/description from the function group until the capability is implemented;
if retaining the registration to reserve the name, explicitly document that
intent in query_context’s docstring.
- Around line 109-116: Update both catch-all handlers in
sources/gsf/src/gsf/register.py: lines 109-116 and 135-142, changing the
logger.error calls in the catalog-search and text-to-SQL tool closures to
logger.exception so unexpected failures include traceback and exception details.
No other handler behavior needs to change.
- Around line 76-84: Update _request_trace_headers to emit a debug-level log
containing the caught exception before returning {} from its broad exception
handler; preserve the existing best-effort empty-header fallback and avoid
changing successful header propagation.
- Line 16: Ensure the package containing gsf.register declares a compatible
aiq-agent runtime dependency so the module-level get_auth_token import resolves
during standalone NAT plugin discovery; alternatively, move that import into the
bearer-token authentication path and raise GSFError only when that
authentication is requested.
In `@sources/gsf/tests/test_client.py`:
- Around line 290-307: Add tests covering the remaining _post error paths:
verify repeated 429 responses raise GSFErrorCode.RATE_LIMITED after max_retries
+ 1 attempts, and verify TimeoutException and TransportError are retried then
mapped to TIMEOUT and UPSTREAM_ERROR respectively, including sleep and
response-close behavior. Also add a constructor test for GSFClient confirming
password_auth_email without a password raises ValueError.
In `@sources/gsf/tests/test_models.py`:
- Around line 29-41: Add a test case in
test_catalog_search_response_validates_coverage for constructing
CatalogSearchResponse without the coverage field, using the existing candidate
fixture shape, and assert that the response validates successfully with the
model’s intended default behavior.
In `@sources/gsf/tests/test_register.py`:
- Around line 66-76: Add direct unit coverage for
gsf.register._request_trace_headers without patching it out: provide mixed
metadata headers and assert only allowlisted headers such as traceparent and
x-request-id are returned, excluding authorization and cookie. Also verify it
returns an empty mapping when Context.get is unavailable, while preserving the
existing test setup elsewhere.
🪄 Autofix
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: 400c6372-19c9-47d5-998d-6df61738f9ad
⛔ Files ignored due to path filters (2)
mcp/uv.lockis excluded by!**/*.lockuv.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
.agents/skills/aiq-configure-workflow/scripts/validate_config.pypyproject.tomlscripts/setup.shsources/gsf/README.mdsources/gsf/pyproject.tomlsources/gsf/src/gsf/__init__.pysources/gsf/src/gsf/client.pysources/gsf/src/gsf/errors.pysources/gsf/src/gsf/models.pysources/gsf/src/gsf/register.pysources/gsf/tests/__init__.pysources/gsf/tests/conftest.pysources/gsf/tests/test_client.pysources/gsf/tests/test_models.pysources/gsf/tests/test_register.py
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Pytest and Coverage
- GitHub Check: Lint and Hooks
🧰 Additional context used
📓 Path-based instructions (7)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Format and lint Python with Ruff using a 120-character line length, Python 3.11 target, rules E/F/W/I/PL/UP, and single-line imports; avoid reformatting unrelated code.
Never print or log secret values, including through tool output or error messages.
**/*.py: Runuv run ruff check .anduv run ruff format --check .for root Python changes.
Runuv run pytestfor root project Python changes.
Files:
sources/gsf/tests/__init__.pysources/gsf/src/gsf/__init__.pysources/gsf/tests/test_register.pysources/gsf/src/gsf/errors.pysources/gsf/tests/test_models.pysources/gsf/tests/conftest.pysources/gsf/src/gsf/models.pysources/gsf/src/gsf/register.pysources/gsf/tests/test_client.pysources/gsf/src/gsf/client.py
sources/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Implement new tools and data sources as NeMo Agent Toolkit functions registered with
@register_function; configuration schemas must inherit fromFunctionBaseConfig.
Files:
sources/gsf/tests/__init__.pysources/gsf/src/gsf/__init__.pysources/gsf/tests/test_register.pysources/gsf/src/gsf/errors.pysources/gsf/tests/test_models.pysources/gsf/tests/conftest.pysources/gsf/src/gsf/models.pysources/gsf/src/gsf/register.pysources/gsf/tests/test_client.pysources/gsf/src/gsf/client.py
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Keep changes inside this repository, avoid editing adjacent repositories, and scope changes to the smallest relevant independent package, especially undersources/.
Run the narrowest relevant validation command first and broaden to the full suite only when a change crosses shared boundaries.
Keep pull requests scoped, exclude unrelated files and generated artifacts, never include secrets, and provide validation commands and results.
**/*: Do not include secrets, credentials, private hostnames, internal-only logs, customer data, or generated local artifacts.
Add or update tests for behavior changes.
Files:
sources/gsf/tests/__init__.pypyproject.tomlscripts/setup.shsources/gsf/README.mdsources/gsf/src/gsf/__init__.pysources/gsf/tests/test_register.pysources/gsf/src/gsf/errors.pysources/gsf/tests/test_models.pysources/gsf/pyproject.tomlsources/gsf/tests/conftest.pysources/gsf/src/gsf/models.pysources/gsf/src/gsf/register.pysources/gsf/tests/test_client.pysources/gsf/src/gsf/client.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/gsf/tests/__init__.pysources/gsf/README.mdsources/gsf/src/gsf/__init__.pysources/gsf/tests/test_register.pysources/gsf/src/gsf/errors.pysources/gsf/tests/test_models.pysources/gsf/pyproject.tomlsources/gsf/tests/conftest.pysources/gsf/src/gsf/models.pysources/gsf/src/gsf/register.pysources/gsf/tests/test_client.pysources/gsf/src/gsf/client.py
{.github/**,ci/**,.pre-commit-config.yaml,pyproject.toml,uv.lock,mcp/pyproject.toml,mcp/uv.lock}
⚙️ CodeRabbit configuration file
{.github/**,ci/**,.pre-commit-config.yaml,pyproject.toml,uv.lock,mcp/pyproject.toml,mcp/uv.lock}: Review automation and packaging changes for least-privilege permissions, pinned versions where appropriate,
copy-pr-bot pull-request/ branch behavior, reproducible uv/npm setup, secret handling, and consistency with
the documented validation matrix.
Files:
pyproject.toml
.agents/skills/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Use repository maintainer skills from
.agents/skills/for applicable workflows; keep API-consumer skills inskills/separate because they are not deployed application runtime skills.
Files:
.agents/skills/aiq-configure-workflow/scripts/validate_config.py
{skills/**,.agents/skills/**,.claude/skills/**,.github/skill-eval/**}
⚙️ CodeRabbit configuration file
{skills/**,.agents/skills/**,.claude/skills/**,.github/skill-eval/**}: Review Agent Skill and skill-eval changes for valid skill metadata, deterministic eval specs, safe handling of
credentials, and clear generated-output boundaries. Do not flag SKILL.md files for missing SPDX headers when the
entrypoint intentionally starts with YAML frontmatter.
Files:
.agents/skills/aiq-configure-workflow/scripts/validate_config.py
🪛 ast-grep (0.45.0)
sources/gsf/tests/test_client.py
[info] 24-24: use jsonify instead of json.dumps for JSON output
Context: json.dumps({'type': 'result', 'answer': answer})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🔇 Additional comments (28)
.agents/skills/aiq-configure-workflow/scripts/validate_config.py (2)
122-123: LGTM!
255-255: 🗄️ Data Integrity & IntegrationKeep the registry declaration contract consistent with runtime tool names.
The validator admits function-group keys, but the supplied GSF group registers namespaced callable names. Confirm that the data-source registry intentionally consumes group IDs.
.agents/skills/aiq-configure-workflow/scripts/validate_config.py#L255-L255: Validate the same identifiers that the registry resolves, or add an explicit group-ID resolution path.sources/gsf/README.md#L44-L45: Keeptools: - gsfonly if the registry consumes group IDs; otherwise list the namespaced runtime tools.scripts/setup.sh (1)
117-117: LGTM!sources/gsf/README.md (1)
1-27: LGTM!Also applies to: 29-43, 46-47, 70-80
sources/gsf/tests/__init__.py (1)
1-2: LGTM!pyproject.toml (1)
213-213: 📐 Maintainability & Code QualityNo lockfile change is needed.
uv.lockalready containsaiq-gsfas a workspace member, development dependency, editable package, and resolved package with its dependencies.> Likely an incorrect or invalid review comment.sources/gsf/src/gsf/__init__.py (1)
1-10: LGTM!sources/gsf/src/gsf/errors.py (1)
13-63: LGTM!sources/gsf/src/gsf/models.py (1)
13-41: LGTM!Also applies to: 53-124
sources/gsf/tests/conftest.py (1)
7-132: LGTM!sources/gsf/tests/test_models.py (1)
44-105: LGTM!sources/gsf/src/gsf/client.py (11)
68-82: LGTM!
105-127: LGTM!Also applies to: 155-176
195-224: LGTM!Also applies to: 233-237, 247-260
302-323: LGTM!
325-339: LGTM!
341-348: LGTM!Also applies to: 365-386
388-419: LGTM!
421-436: LGTM!Also applies to: 442-467
500-528: LGTM!
530-650: LGTM!
138-153: 🚀 Performance & ScalabilityConfirm the
question-entity-coverageresult-limit contract before changing this payload.
max_resultsis not sent to GSF._normalize_catalog_searchtruncates the returnedcandidateslist locally, so requests can transfer more candidates than requested and may hitmax_response_bytes. If the endpoint supports a result-limit field, sendrequest.max_resultsand retain client-side truncation as a safety net.sources/gsf/tests/test_client.py (1)
18-34: LGTM!Also applies to: 37-289, 310-355
sources/gsf/src/gsf/register.py (3)
36-55: LGTM!
58-74: LGTM!
87-108: LGTM!Also applies to: 117-134, 155-174
sources/gsf/tests/test_register.py (1)
19-65: LGTM!Also applies to: 79-173
sources/gsf/pyproject.toml (1)
18-25: 📐 Maintainability & Code QualityUse the existing workspace test configuration. The root
pyproject.tomlprovidespytest,pytest-asyncio, andasyncio_mode = "auto".aiq-gsfis asources/*workspace member included in the root development group. No package-level test extra or local asyncio configuration is required.> Likely an incorrect or invalid review comment.
soumilinandi
changed the title
Signed-off-by: Soumili Nandi <soumilin@nvidia.com>
Signed-off-by: Soumili Nandi <soumilin@nvidia.com>
Signed-off-by: Soumili Nandi <soumilin@nvidia.com>
Signed-off-by: Soumili Nandi <soumilin@nvidia.com>
Signed-off-by: Soumili Nandi <soumilin@nvidia.com>
Signed-off-by: Soumili Nandi <soumilin@nvidia.com>
Signed-off-by: Soumili Nandi <soumilin@nvidia.com>
Signed-off-by: Soumili Nandi <soumilin@nvidia.com>
Signed-off-by: Soumili Nandi <soumilin@nvidia.com>
Signed-off-by: Soumili Nandi <soumilin@nvidia.com>
Signed-off-by: Soumili Nandi <soumilin@nvidia.com>
9e23c08 to
f0a1a6b
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/gsf/src/gsf/client.py`:
- Around line 411-425: Update the SSE event parsing loop in the client method
containing event_data and _answer_from_event to catch json.JSONDecodeError for
malformed intermediate events, skip those events, and continue reading until a
valid result is found. Preserve strict handling for decodable error/result
events, and add a test using a malformed step event before a valid result that
verifies the SQL is returned.
In `@sources/gsf/src/gsf/register.py`:
- Around line 43-52: The GSF configuration currently uses the shared
FunctionGroup configuration instead of the required registration contract.
Update catalog_search and text_to_sql to use `@register_function` with
FunctionBaseConfig, replacing or separating GSFFunctionGroupConfig as needed
while preserving their existing configuration fields; retain the shared
FunctionGroup design only if an explicit exception is obtained.
In `@sources/gsf/tests/test_client.py`:
- Around line 357-404: The retry tests cover only jitter fallback and not
server-directed delays. Add an async test alongside
test_client_retries_rate_limit_then_succeeds that returns a 429 with a
Retry-After header, retries successfully, and asserts asyncio.sleep is awaited
with the parsed delay clamped to _MAX_RETRY_DELAY_SECONDS (for example, 30.0);
keep the existing deterministic jitter tests unchanged.
🪄 Autofix
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: 39704dea-13f8-407c-afdd-1b7226cc0628
⛔ Files ignored due to path filters (2)
mcp/uv.lockis excluded by!**/*.lockuv.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
.agents/skills/aiq-configure-workflow/scripts/validate_config.pypyproject.tomlscripts/setup.shsources/gsf/README.mdsources/gsf/pyproject.tomlsources/gsf/src/gsf/__init__.pysources/gsf/src/gsf/client.pysources/gsf/src/gsf/errors.pysources/gsf/src/gsf/models.pysources/gsf/src/gsf/register.pysources/gsf/tests/conftest.pysources/gsf/tests/test_client.pysources/gsf/tests/test_models.pysources/gsf/tests/test_register.py
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: Lint and Hooks
- GitHub Check: Pytest and Coverage
- GitHub Check: Script Validation
🧰 Additional context used
📓 Path-based instructions (7)
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Keep changes inside this repository, avoid editing adjacent repositories, and scope changes to the smallest relevant independent package, especially undersources/.
Run the narrowest relevant validation command first and broaden to the full suite only when a change crosses shared boundaries.
Keep pull requests scoped, exclude unrelated files and generated artifacts, never include secrets, and provide validation commands and results.
**/*: Do not include secrets, credentials, private hostnames, internal-only logs, customer data, or generated local artifacts.
Add or update tests for behavior changes.
Files:
scripts/setup.shsources/gsf/pyproject.tomlsources/gsf/tests/test_models.pypyproject.tomlsources/gsf/src/gsf/__init__.pysources/gsf/README.mdsources/gsf/tests/conftest.pysources/gsf/src/gsf/models.pysources/gsf/src/gsf/register.pysources/gsf/src/gsf/errors.pysources/gsf/tests/test_register.pysources/gsf/tests/test_client.pysources/gsf/src/gsf/client.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/gsf/pyproject.tomlsources/gsf/tests/test_models.pysources/gsf/src/gsf/__init__.pysources/gsf/README.mdsources/gsf/tests/conftest.pysources/gsf/src/gsf/models.pysources/gsf/src/gsf/register.pysources/gsf/src/gsf/errors.pysources/gsf/tests/test_register.pysources/gsf/tests/test_client.pysources/gsf/src/gsf/client.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Format and lint Python with Ruff using a 120-character line length, Python 3.11 target, rules E/F/W/I/PL/UP, and single-line imports; avoid reformatting unrelated code.
Never print or log secret values, including through tool output or error messages.
**/*.py: Runuv run ruff check .anduv run ruff format --check .for root Python changes.
Runuv run pytestfor root project Python changes.
Files:
sources/gsf/tests/test_models.pysources/gsf/src/gsf/__init__.pysources/gsf/tests/conftest.pysources/gsf/src/gsf/models.pysources/gsf/src/gsf/register.pysources/gsf/src/gsf/errors.pysources/gsf/tests/test_register.pysources/gsf/tests/test_client.pysources/gsf/src/gsf/client.py
sources/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Implement new tools and data sources as NeMo Agent Toolkit functions registered with
@register_function; configuration schemas must inherit fromFunctionBaseConfig.
Files:
sources/gsf/tests/test_models.pysources/gsf/src/gsf/__init__.pysources/gsf/tests/conftest.pysources/gsf/src/gsf/models.pysources/gsf/src/gsf/register.pysources/gsf/src/gsf/errors.pysources/gsf/tests/test_register.pysources/gsf/tests/test_client.pysources/gsf/src/gsf/client.py
{.github/**,ci/**,.pre-commit-config.yaml,pyproject.toml,uv.lock,mcp/pyproject.toml,mcp/uv.lock}
⚙️ CodeRabbit configuration file
{.github/**,ci/**,.pre-commit-config.yaml,pyproject.toml,uv.lock,mcp/pyproject.toml,mcp/uv.lock}: Review automation and packaging changes for least-privilege permissions, pinned versions where appropriate,
copy-pr-bot pull-request/ branch behavior, reproducible uv/npm setup, secret handling, and consistency with
the documented validation matrix.
Files:
pyproject.toml
.agents/skills/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Use repository maintainer skills from
.agents/skills/for applicable workflows; keep API-consumer skills inskills/separate because they are not deployed application runtime skills.
Files:
.agents/skills/aiq-configure-workflow/scripts/validate_config.py
{skills/**,.agents/skills/**,.claude/skills/**,.github/skill-eval/**}
⚙️ CodeRabbit configuration file
{skills/**,.agents/skills/**,.claude/skills/**,.github/skill-eval/**}: Review Agent Skill and skill-eval changes for valid skill metadata, deterministic eval specs, safe handling of
credentials, and clear generated-output boundaries. Do not flag SKILL.md files for missing SPDX headers when the
entrypoint intentionally starts with YAML frontmatter.
Files:
.agents/skills/aiq-configure-workflow/scripts/validate_config.py
🪛 ast-grep (0.45.0)
sources/gsf/tests/test_client.py
[info] 28-28: use jsonify instead of json.dumps for JSON output
Context: json.dumps({'type': 'result', 'answer': answer})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 192-192: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"type": "result", "answer": answer}, ensure_ascii=False)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
sources/gsf/src/gsf/client.py
[info] 371-371: use secrets package over random package
Context: random.random()
Note: [CWE-330] Use of Insufficiently Random Values.
(avoid-random-python)
🔇 Additional comments (26)
sources/gsf/src/gsf/__init__.py (1)
1-11: LGTM!sources/gsf/src/gsf/errors.py (1)
13-67: LGTM!sources/gsf/tests/conftest.py (1)
9-134: LGTM!sources/gsf/tests/test_models.py (1)
18-138: LGTM!sources/gsf/src/gsf/client.py (11)
28-74: LGTM!
76-119: LGTM!
213-284: LGTM!
286-341: LGTM!
343-372: LGTM!
374-390: LGTM!
441-476: LGTM!
478-526: LGTM!
528-591: LGTM!
593-721: LGTM!
130-169: 🚀 Performance & ScalabilityCheck whether GSF supports upstream result limits.
catalog_searchandtext_to_sqlapplymax_resultsandmax_rowsonly after_postreads the complete response. A large response can therefore raiseRESPONSE_TOO_LARGEbefore client-side truncation. If the GSF endpoints support result-limit parameters, send the effective limits in the payload and test the API contract.sources/gsf/tests/test_client.py (4)
20-56: LGTM!
59-232: LGTM!
234-337: LGTM!
408-529: LGTM!sources/gsf/pyproject.toml (1)
11-25: 📐 Maintainability & Code QualityThe root
pyproject.tomlprovidespytest-asyncioand setsasyncio_mode = "auto"forsources/**/tests. No package-local test dependency is required for root workspace tests.> Likely an incorrect or invalid review comment..agents/skills/aiq-configure-workflow/scripts/validate_config.py (1)
232-256: Add regression tests forfunction_groupsvalidation.This behavior change still needs tests for a valid mapping, invalid list and scalar values, and a registry tool declared through
function_groups.As per coding guidelines, “Add or update tests for behavior changes.”
Source: Coding guidelines
sources/gsf/src/gsf/models.py (1)
1-124: LGTM!sources/gsf/tests/test_register.py (1)
19-193: LGTM!scripts/setup.sh (1)
117-117: LGTM!sources/gsf/README.md (1)
1-81: LGTM!pyproject.toml (1)
213-213: 🩺 Stability & AvailabilityThe lockfile already includes
aiq-gsfand its dependencies. No lockfile update is required.> Likely an incorrect or invalid review comment.
Signed-off-by: Soumili Nandi <soumilin@nvidia.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/gsf/src/gsf/client.py (1)
364-372: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winParse HTTP-date
Retry-Aftervalues.
_retry_delayhandles only numeric values. HTTP permitsRetry-Afterto contain either delay seconds or an HTTP-date. A valid date currently falls through to jitter, so the client can retry before the server-directed time. (rfc-editor.org)Parse HTTP dates, calculate the UTC delay, and apply the existing zero and maximum bounds. Add a deterministic test with a known past HTTP-date that asserts a
0.0delay. As per path instructions: "Review data-source and knowledge-layer changes for ... retry/rate-limit behavior, deterministic tests."🤖 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/gsf/src/gsf/client.py` around lines 364 - 372, The _retry_delay function currently ignores valid HTTP-date Retry-After values. After numeric parsing fails, parse the header as an HTTP date, calculate the delay from the current UTC time, and apply the existing 0.0-to-_MAX_RETRY_DELAY_SECONDS bounds before returning it; retain jitter fallback for invalid values. Add a deterministic test using a known past HTTP date that asserts a 0.0 delay.Source: Path instructions
🤖 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/gsf/src/gsf/client.py`:
- Around line 364-372: The _retry_delay function currently ignores valid
HTTP-date Retry-After values. After numeric parsing fails, parse the header as
an HTTP date, calculate the delay from the current UTC time, and apply the
existing 0.0-to-_MAX_RETRY_DELAY_SECONDS bounds before returning it; retain
jitter fallback for invalid values. Add a deterministic test using a known past
HTTP date that asserts a 0.0 delay.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: fb656a97-8112-4e49-8d99-5eab21adf0b7
📒 Files selected for processing (2)
sources/gsf/src/gsf/client.pysources/gsf/tests/test_client.py
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: Lint and Hooks
- GitHub Check: Pytest and Coverage
- GitHub Check: Script Validation
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Format and lint Python with Ruff using a 120-character line length, Python 3.11 target, rules E/F/W/I/PL/UP, and single-line imports; avoid reformatting unrelated code.
Never print or log secret values, including through tool output or error messages.
**/*.py: Runuv run ruff check .anduv run ruff format --check .for root Python changes.
Runuv run pytestfor root project Python changes.
Files:
sources/gsf/tests/test_client.pysources/gsf/src/gsf/client.py
sources/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Implement new tools and data sources as NeMo Agent Toolkit functions registered with
@register_function; configuration schemas must inherit fromFunctionBaseConfig.
Files:
sources/gsf/tests/test_client.pysources/gsf/src/gsf/client.py
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Keep changes inside this repository, avoid editing adjacent repositories, and scope changes to the smallest relevant independent package, especially undersources/.
Run the narrowest relevant validation command first and broaden to the full suite only when a change crosses shared boundaries.
Keep pull requests scoped, exclude unrelated files and generated artifacts, never include secrets, and provide validation commands and results.
**/*: Do not include secrets, credentials, private hostnames, internal-only logs, customer data, or generated local artifacts.
Add or update tests for behavior changes.
Files:
sources/gsf/tests/test_client.pysources/gsf/src/gsf/client.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/gsf/tests/test_client.pysources/gsf/src/gsf/client.py
🪛 ast-grep (0.45.0)
sources/gsf/tests/test_client.py
[info] 211-211: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"type": "result", "answer": chat_sql_answer})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🔇 Additional comments (2)
sources/gsf/src/gsf/client.py (1)
40-363: LGTM!Also applies to: 374-414, 415-420, 421-726
sources/gsf/tests/test_client.py (1)
207-223: LGTM!Also applies to: 400-419
|
/ok to test 9872f90 |
cdgamarose-nv
left a comment
cdgamarose-nv
left a comment
There was a problem hiding this comment.
Looks good to me! Great job!
…-Blueprints#427) * Add GSF structured query tools Signed-off-by: Soumili Nandi <soumilin@nvidia.com> * Align GSF adapter with chat completions Signed-off-by: Soumili Nandi <soumilin@nvidia.com> * Extend GSF query tools and local authentication Signed-off-by: Soumili Nandi <soumilin@nvidia.com> * Validate function group registry references Signed-off-by: Soumili Nandi <soumilin@nvidia.com> * Handle GSF prediction PQL response Signed-off-by: Soumili Nandi <soumilin@nvidia.com> * Add GSF catalog search capability Signed-off-by: Soumili Nandi <soumilin@nvidia.com> * Exclude GSF prose from SQL tool output Signed-off-by: Soumili Nandi <soumilin@nvidia.com> * Preserve GSF SQL thought summaries Signed-off-by: Soumili Nandi <soumilin@nvidia.com> * Remove unused GSF provenance helper Signed-off-by: Soumili Nandi <soumilin@nvidia.com> * Disable unvalidated GSF PQL tool Signed-off-by: Soumili Nandi <soumilin@nvidia.com> * Harden GSF integration behavior Signed-off-by: Soumili Nandi <soumilin@nvidia.com> * Harden GSF SSE response handling Signed-off-by: Soumili Nandi <soumilin@nvidia.com> --------- Signed-off-by: Soumili Nandi <soumilin@nvidia.com>
2 participants
Overview
Adds the initial GSF integration as an independent
sources/gsfpackage and NAT function group.gsf__text_to_sqlandgsf__catalog_search.database_nameinputs to GSF’starget_db.thoughts, while excluding compatibility prose from SQL evidence.DCO sign-off for the squash commit
Signed-off-by: Soumili Nandi soumilin@nvidia.com
Validation
.venv/bin/ruff check sources/gsf— passed..venv/bin/ruff format --check sources/gsf— 10 files already formatted..venv/bin/pytest sources/gsf/tests— 30 passed.Manually smoke-tested password authentication, catalog search, text-to-SQL,
target_dbforwarding, and parallel sessions against the GSF development deployment.I 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.I replaced the DCO sign-off placeholder with my GitHub commit identity and kept the required angle brackets around the email address.
Where should reviewers start?
Start with
sources/gsf/src/gsf/register.pyfor the NAT function-group boundary and exposed tools, followed bysources/gsf/src/gsf/client.pyfor authentication, request handling, SSE parsing, response normalization, and bounds.Focused coverage is under
sources/gsf/tests/.Related Issues
Summary by CodeRabbit