issue #4038 - fix(mcp): handle dict/object result shapes in stream writer error parsing#4039
Open
Shadow2121 wants to merge 1 commit intotraceloop:mainfrom
Open
issue #4038 - fix(mcp): handle dict/object result shapes in stream writer error parsing#4039Shadow2121 wants to merge 1 commit intotraceloop:mainfrom
Shadow2121 wants to merge 1 commit intotraceloop:mainfrom
Conversation
…stream writer error parsing
|
|
📝 WalkthroughWalkthroughAdded helper methods to Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/opentelemetry-instrumentation-mcp/tests/test_fastmcp.py (1)
156-183: Consider adding a negative-path test forisError=False.The three new tests cover only
isError=Truebranches. Adding a case whereisErroris absent/False(expecting(False, None)) would lock in the early-return behavior at line 559–560 and prevent regressions if the helper is later refactored.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opentelemetry-instrumentation-mcp/tests/test_fastmcp.py` around lines 156 - 183, Add a negative-path unit test for InstrumentedStreamWriter._extract_result_error that verifies when isError is False or missing the function returns (False, None); create a new test similar to the others (e.g., test_extract_result_error_when_not_error) that passes a dict (and/or SimpleNamespace) with isError=False or without isError and asserts is_error is False and message is None to lock in the early-return behavior of _extract_result_error.packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py (2)
342-349: Duplicate error-extraction logic in_execute_and_handle_result.This block uses the same direct-indexing pattern (
result.content[0].text) that the new_extract_result_errorhelper was introduced to replace. Ifresultarrives in dict shape here (same scenario motivating this PR), this path still breaks. Consider reusing the new helper for consistency and to fully close out issue#4038.Proposed refactor
- # Handle errors - if hasattr(result, "isError") and result.isError: - if len(result.content) > 0: - span.set_status( - Status(StatusCode.ERROR, f"{result.content[0].text}") - ) - else: - span.set_status(Status(StatusCode.OK)) + # Handle errors + is_error, error_message = InstrumentedStreamWriter._extract_result_error(result) + if is_error: + span.set_status(Status(StatusCode.ERROR, error_message)) + else: + span.set_status(Status(StatusCode.OK))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py` around lines 342 - 349, Replace the direct-indexing error extraction in _execute_and_handle_result with the new _extract_result_error helper: when result has isError, call _extract_result_error(result) to obtain the error text (falling back appropriately) and pass that string into span.set_status(Status(StatusCode.ERROR, ...)); remove any use of result.content[0].text so dict-shaped results are handled consistently with _extract_result_error.
556-571: Strictis Truecheck may miss truthy error flags;content[0]still assumes subscriptable.Two minor robustness considerations:
- Line 558 uses
is True, so truthy-but-non-bool values (e.g.,1,"true") won't trigger the error path. The MCP spec definesisErroras a bool, so this is likely intentional, but a simplebool(...)coercion would be more forgiving of non-conforming servers — which is precisely the class of shape-variance this PR is addressing.- Line 566 (
content[0]) assumescontentis indexable. If a server returnscontentas a non-list iterable or dict, this raisesTypeError/KeyError. It's currently shielded by@dont_throwonsend(), but the helper itself isn't, and the new unit tests call it directly — a malformed shape there would surface as a test/production exception rather than the graceful(True, None)fallback the rest of the function aims for.Suggested hardening
`@classmethod` def _extract_result_error(cls, result: Any) -> Tuple[bool, Union[str, None]]: - is_error = cls._get_value(result, "isError", False) is True + is_error = bool(cls._get_value(result, "isError", False)) if not is_error: return False, None content = cls._get_value(result, "content", []) - if not content: + if not content or not isinstance(content, (list, tuple)): return True, None first_item = content[0] message = cls._get_value(first_item, "text") if message is None: return True, None return True, str(message)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py` around lines 556 - 571, _extract_result_error is fragile: replace the strict "is True" check on isError with a boolean coercion (e.g., is_error = bool(cls._get_value(result, "isError", False))) so truthy non-bool values are handled, and make the content-first-item access robust by not assuming subscriptability — use a safe extraction strategy (e.g., if isinstance(content, (list, tuple)) take content[0], else attempt first = next(iter(content)) in a try/except or handle dict by taking next(iter(content.values()))), and fall back to returning (True, None) on any extraction failure; keep references to _extract_result_error, _get_value, is_error, content, and first_item so you update the correct code paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py`:
- Around line 342-349: Replace the direct-indexing error extraction in
_execute_and_handle_result with the new _extract_result_error helper: when
result has isError, call _extract_result_error(result) to obtain the error text
(falling back appropriately) and pass that string into
span.set_status(Status(StatusCode.ERROR, ...)); remove any use of
result.content[0].text so dict-shaped results are handled consistently with
_extract_result_error.
- Around line 556-571: _extract_result_error is fragile: replace the strict "is
True" check on isError with a boolean coercion (e.g., is_error =
bool(cls._get_value(result, "isError", False))) so truthy non-bool values are
handled, and make the content-first-item access robust by not assuming
subscriptability — use a safe extraction strategy (e.g., if isinstance(content,
(list, tuple)) take content[0], else attempt first = next(iter(content)) in a
try/except or handle dict by taking next(iter(content.values()))), and fall back
to returning (True, None) on any extraction failure; keep references to
_extract_result_error, _get_value, is_error, content, and first_item so you
update the correct code paths.
In `@packages/opentelemetry-instrumentation-mcp/tests/test_fastmcp.py`:
- Around line 156-183: Add a negative-path unit test for
InstrumentedStreamWriter._extract_result_error that verifies when isError is
False or missing the function returns (False, None); create a new test similar
to the others (e.g., test_extract_result_error_when_not_error) that passes a
dict (and/or SimpleNamespace) with isError=False or without isError and asserts
is_error is False and message is None to lock in the early-return behavior of
_extract_result_error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 863d7ad6-ec74-4b89-b955-175a7a366a25
📒 Files selected for processing (2)
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.pypackages/opentelemetry-instrumentation-mcp/tests/test_fastmcp.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
feat(instrumentation): ...orfix(instrumentation): ....Summary by CodeRabbit
Release Notes