Conversation
📝 WalkthroughWalkthroughAdds optional response_context_path to GenericHTTPConnector and extends parse_response to extract a session_id and a response context (serializing dict/list to JSON, stringifying other types), returning custom_outputs only when values are extracted; tests validate these behaviors. ChangesCore Connector Enhancement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/sdg_hub/core/connectors/agent/generic_http.py (2)
44-58: Document the newresponse_context_pathparameter in the class docstring.The class-level Parameters section lists
request_message_path,response_text_path,response_session_id_path, andrequest_session_id_path, but the newly addedresponse_context_pathis not documented here. Adding it keeps the docstring in sync with the field set and improves discoverability.📝 Suggested addition
response_session_id_path : str, optional Dot-notation path to extract session ID from the response. request_session_id_path : str, optional Dot-notation path where session ID is placed in the request body. + response_context_path : str, optional + Dot-notation path to extract retrieved context from the response + (e.g., ``"output.context"``). Useful for RAG evaluation. ``dict`` + and ``list`` values are JSON-serialized; other values are stringified.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sdg_hub/core/connectors/agent/generic_http.py` around lines 44 - 58, The class docstring Parameters section is missing the newly added response_context_path parameter; update the docstring for the class (where Parameters lists request_message_path, response_text_path, response_session_id_path, request_session_id_path) to include a short description of response_context_path (e.g., "Dot-notation path to extract contextual metadata from the response body, optional; Example: 'output.context'") so the docstring matches the class fields and aids discoverability; ensure wording follows the existing style and placement among the other path parameters.
178-184: Update theparse_responsesummary to mention context extraction.The Returns section already references
_extracted_context, but the leading description still only mentions text and session ID. Consider mentioning the new context extraction for consistency.📝 Suggested edit
- Extracts text (and optionally session ID) from the response using - the configured dot-notation paths, storing them under known keys - so the ``extract_text`` and ``extract_session_id`` classmethods - can retrieve them without instance access. + Extracts text (and optionally session ID and retrieved context) + from the response using the configured dot-notation paths, + storing them under known keys so the ``extract_text``, + ``extract_session_id``, and downstream context consumers can + retrieve them without instance access.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sdg_hub/core/connectors/agent/generic_http.py` around lines 178 - 184, Update the docstring for parse_response to mention that it also extracts and returns contextual data: change the leading description (the triple-quoted summary) to state that the method validates the response and extracts text, optional session ID, and context (stored under _extracted_context) so the Returns section and summary are consistent with the extracted context behavior in parse_response.tests/connectors/agent/test_generic_http.py (1)
239-321: Good coverage for the new context-extraction paths.The new cases cover the meaningful branches of
parse_responseforresponse_context_path: list → JSON string, dict → JSON string, scalar string passthrough, missing path in response, unconfigured path, and stripping of an upstream-spoofed_extracted_context. This matches the documented behavior ingeneric_http.pyand aligns with the guideline to test both success and absence cases.One small optional addition: a test asserting JSON round-tripping with non-ASCII content (since the implementation uses
ensure_ascii=False) would lock in that behavior — e.g., a context value containing characters like"café"and verifyingjson.loads(parsed["_extracted_context"])returns the original string. Not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/connectors/agent/test_generic_http.py` around lines 239 - 321, Add a small test to assert non-ASCII JSON round-tripping for response_context_path: create a connector via _make_connector(response_context_path="output.context"), pass a response where output.context contains a non-ASCII value (e.g., "café") in one of the items (or as the string/dict), call connector.parse_response(response), then json.loads(parsed["_extracted_context"]) and assert it equals the original non-ASCII content; place it alongside the other test_parse_response_* functions in tests/connectors/agent/test_generic_http.py and name it something like test_parse_response_context_non_ascii to make the behavior explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/sdg_hub/core/connectors/agent/generic_http.py`:
- Around line 44-58: The class docstring Parameters section is missing the newly
added response_context_path parameter; update the docstring for the class (where
Parameters lists request_message_path, response_text_path,
response_session_id_path, request_session_id_path) to include a short
description of response_context_path (e.g., "Dot-notation path to extract
contextual metadata from the response body, optional; Example:
'output.context'") so the docstring matches the class fields and aids
discoverability; ensure wording follows the existing style and placement among
the other path parameters.
- Around line 178-184: Update the docstring for parse_response to mention that
it also extracts and returns contextual data: change the leading description
(the triple-quoted summary) to state that the method validates the response and
extracts text, optional session ID, and context (stored under
_extracted_context) so the Returns section and summary are consistent with the
extracted context behavior in parse_response.
In `@tests/connectors/agent/test_generic_http.py`:
- Around line 239-321: Add a small test to assert non-ASCII JSON round-tripping
for response_context_path: create a connector via
_make_connector(response_context_path="output.context"), pass a response where
output.context contains a non-ASCII value (e.g., "café") in one of the items (or
as the string/dict), call connector.parse_response(response), then
json.loads(parsed["_extracted_context"]) and assert it equals the original
non-ASCII content; place it alongside the other test_parse_response_* functions
in tests/connectors/agent/test_generic_http.py and name it something like
test_parse_response_context_non_ascii to make the behavior explicit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 78e33ddb-0dcf-47a4-a6b7-dec8887f9066
📒 Files selected for processing (2)
src/sdg_hub/core/connectors/agent/generic_http.pytests/connectors/agent/test_generic_http.py
|
@claude-code review |
|
Claude Code is working… I'll analyze this and get back to you. |
| context, ensure_ascii=False | ||
| ) | ||
| else: | ||
| parsed["_extracted_context"] = str(context) |
There was a problem hiding this comment.
Suggestion: The existing pattern defines extract_text, extract_session_id, and extract_tool_trace classmethods on BaseAgentConnector (returning None) which GenericHTTPConnector overrides. AgentResponseExtractorBlock uses these classmethods to access extracted fields (see agent_response_extractor_block.py:188-207).
Without a corresponding extract_context classmethod, the _extracted_context value stored here is not accessible through the standard extraction interface. Downstream code would need to read response.get("_extracted_context") directly, bypassing the abstraction.
Consider adding in a follow-up PR:
extract_context()classmethod onBaseAgentConnector(returningNone)- Override in
GenericHTTPConnectorreading_extracted_context extract_contextfield onAgentResponseExtractorBlock
Not blocking since the connector-layer work here is complete and correct.
shivchander
left a comment
shivchander
left a comment
There was a problem hiding this comment.
Code Review — Approved
Summary
Adds an optional response_context_path field to GenericHTTPConnector for extracting retrieved context from HTTP responses using dot-notation. Context values that are dicts or lists are JSON-serialized; other types are stringified. This is useful for RAG evaluation pipelines that need to capture retrieved documents alongside agent answers.
Quality Check
- Correctness: PASS — Implementation correctly reuses the existing
_get_nestedhelper and follows the same extraction pattern asresponse_text_pathandresponse_session_id_path. JSON serialization withensure_ascii=Falseis correct. Reserved key_extracted_contextis properly sanitized to prevent upstream spoofing. - Tests: PASS — 45/45 tests pass. 5 new tests cover list context (JSON), dict context (JSON), string context (str), missing path (no key), and unconfigured path (no key). Reserved key stripping test updated. All assertions check specific values, not just existence.
- Architecture: PASS — Follows connector invariants.
GenericHTTPConnectorinherits fromBaseAgentConnector, is registered via@ConnectorRegistry.register("generic_http"), test file at correct path. New field is optional withNonedefault, so backward-compatible. - Documentation: PASS — Field has a descriptive docstring.
parse_responsedocstring updated to mention_extracted_context. Validator updated to include the new path. - Linting: PASS — ruff check clean, mypy clean (0 errors).
What's Good
- Clean, minimal diff that follows existing patterns exactly
- Thorough test coverage with specific assertions for each type case (list, dict, string, missing, unconfigured)
- Proper reserved key sanitization prevents upstream spoofing
- Backward-compatible — no changes to existing behavior when
response_context_pathisNone
Suggestion (non-blocking)
See inline comment: the existing extraction pattern uses classmethods (extract_text, extract_session_id, extract_tool_trace) on BaseAgentConnector consumed by AgentResponseExtractorBlock. A follow-up PR to add extract_context classmethod + block integration would complete the end-to-end context extraction pipeline.
All criteria met. Approved.
|
This pull request has merge conflicts that must be resolved before it can be |
shivchander
left a comment
shivchander
left a comment
There was a problem hiding this comment.
Final Review — SDG-45
Code looks good. Clean, minimal diff that follows existing extraction patterns exactly. Backward-compatible with proper reserved-key sanitization and thorough test coverage.
Checklist:
- Aligned with task — adds
response_context_pathfor RAG context extraction - No scope creep — only the new field and its extraction logic
- Evaluator review was thorough — all quality checks passed
- Tests comprehensive — 5 new tests cover list/dict/string/missing/unconfigured cases
Blocking before merge:
- PR has merge conflicts (
needs-rebaselabel already applied) — contributor needs to rebase onto main code-reviewanddocsCI checks are failing
Per team policy: This is an external contributor PR from @csoceanu. Adding needs-human-review for human approval before merge.
shivchander
added
the
needs-human-review
|
@shivchander Thanks for the review! |
|
@shivchander Hey, just checking — is this feature still something you'd want merged? If so, I'll rebase against the MLflow refactor and put the extracted context into |
Add optional response_context_path field for extracting retrieved context from responses into custom_outputs. Lists and dicts are JSON-serialized; other values are stringified. Useful for RAG evaluation pipelines. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2a18bc2 to
6668bbe
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/sdg_hub/core/connectors/agent/generic_http.py (1)
235-241: 💤 Low valueConsider adding a brief inline comment explaining the context serialization strategy.
The serialization logic is correct but a brief comment would make the intent more explicit for future maintainers.
📝 Optional inline comment
if self.response_context_path: context = _get_nested(response, self.response_context_path) if context is not None: + # Serialize dict/list as JSON; stringify other types if isinstance(context, (dict, list)): custom_outputs["context"] = json.dumps(context, ensure_ascii=False) else: custom_outputs["context"] = str(context)🤖 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 `@src/sdg_hub/core/connectors/agent/generic_http.py` around lines 235 - 241, Add a short inline comment above the block that handles self.response_context_path explaining the serialization strategy: use _get_nested to extract the context, JSON-serialize dicts/lists with ensure_ascii=False to preserve Unicode for storage in custom_outputs["context"], and fall back to str(context) for scalar values—referencing the response_context_path, _get_nested, custom_outputs, and the local variable context so maintainers understand why both json.dumps and str() are used.tests/connectors/agent/test_generic_http.py (2)
281-330: 💤 Low valueConsider adding test coverage for numeric/boolean context values and non-ASCII characters.
While the current tests cover the main scenarios, additional test cases would provide more complete coverage:
- Numeric/boolean context: Test that numeric and boolean values are stringified correctly (e.g.,
context: 42becomes"42").- Non-ASCII characters: Test that
ensure_ascii=Falsepreserves non-ASCII characters in JSON serialization (e.g., context with emoji or non-Latin scripts).These are edge cases with straightforward behavior, so they're lower priority.
🤖 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 `@tests/connectors/agent/test_generic_http.py` around lines 281 - 330, Add two new unit tests in tests/connectors/agent/test_generic_http.py using the existing _make_connector helper and connector.parse_response: one that sets response_context_path="output.context" and passes numeric/boolean values (e.g., 42 and True) to "context" and asserts result.custom_outputs["context"] is the stringified values ("42", "true" or "True" depending on current stringify behavior), and another that passes a non-ASCII string (emoji or non‑Latin chars) and asserts the returned JSON preserves those characters (ensure the connector's JSON serialization uses ensure_ascii=False or the test expects the preserved characters). Reference the existing test naming pattern (e.g., test_parse_response_handles_numeric_boolean_context and test_parse_response_preserves_non_ascii_context) and keep assertions consistent with how other tests check result.custom_outputs.
281-330: ⚡ Quick winAdd test coverage for simultaneous session_id and context extraction.
The current tests verify session_id and context extraction separately, but there's no test confirming both can be extracted together into
custom_outputs. While the implementation logic appears correct (both are added to the same dict), a test would verify this integration.🧪 Suggested test case
def test_parse_response_extracts_both_session_id_and_context(self): connector = self._make_connector( response_session_id_path="meta.sid", response_context_path="output.context", ) response = { "output": {"answer": "hi", "context": ["doc1", "doc2"]}, "meta": {"sid": "s-123"}, } result = connector.parse_response(response) assert result.custom_outputs is not None assert result.custom_outputs["session_id"] == "s-123" assert result.custom_outputs["context"] == '["doc1", "doc2"]'🤖 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 `@tests/connectors/agent/test_generic_http.py` around lines 281 - 330, Add an integration test to confirm parse_response can extract both session_id and context into the same custom_outputs dict: create a test named test_parse_response_extracts_both_session_id_and_context that uses _make_connector with response_session_id_path="meta.sid" and response_context_path="output.context", constructs a response containing both meta.sid and output.context (e.g., context as a list), calls connector.parse_response(response), and asserts result.custom_outputs is not None and that result.custom_outputs["session_id"] equals the sid string and result.custom_outputs["context"] equals the JSON stringified context; this verifies parse_response correctly merges both values into custom_outputs.
🤖 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.
Nitpick comments:
In `@src/sdg_hub/core/connectors/agent/generic_http.py`:
- Around line 235-241: Add a short inline comment above the block that handles
self.response_context_path explaining the serialization strategy: use
_get_nested to extract the context, JSON-serialize dicts/lists with
ensure_ascii=False to preserve Unicode for storage in custom_outputs["context"],
and fall back to str(context) for scalar values—referencing the
response_context_path, _get_nested, custom_outputs, and the local variable
context so maintainers understand why both json.dumps and str() are used.
In `@tests/connectors/agent/test_generic_http.py`:
- Around line 281-330: Add two new unit tests in
tests/connectors/agent/test_generic_http.py using the existing _make_connector
helper and connector.parse_response: one that sets
response_context_path="output.context" and passes numeric/boolean values (e.g.,
42 and True) to "context" and asserts result.custom_outputs["context"] is the
stringified values ("42", "true" or "True" depending on current stringify
behavior), and another that passes a non-ASCII string (emoji or non‑Latin chars)
and asserts the returned JSON preserves those characters (ensure the connector's
JSON serialization uses ensure_ascii=False or the test expects the preserved
characters). Reference the existing test naming pattern (e.g.,
test_parse_response_handles_numeric_boolean_context and
test_parse_response_preserves_non_ascii_context) and keep assertions consistent
with how other tests check result.custom_outputs.
- Around line 281-330: Add an integration test to confirm parse_response can
extract both session_id and context into the same custom_outputs dict: create a
test named test_parse_response_extracts_both_session_id_and_context that uses
_make_connector with response_session_id_path="meta.sid" and
response_context_path="output.context", constructs a response containing both
meta.sid and output.context (e.g., context as a list), calls
connector.parse_response(response), and asserts result.custom_outputs is not
None and that result.custom_outputs["session_id"] equals the sid string and
result.custom_outputs["context"] equals the JSON stringified context; this
verifies parse_response correctly merges both values into custom_outputs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 48f50661-3548-467e-bce0-d18016c5ad27
📒 Files selected for processing (2)
src/sdg_hub/core/connectors/agent/generic_http.pytests/connectors/agent/test_generic_http.py
2 participants
Add optional response_context_path field for extracting retrieved context from responses using the same declarative dot-notation pattern as existing path fields. Context is JSON-serialized for lists and dicts.
Summary by CodeRabbit
New Features
Tests