Skip to content

fix: add missing stream parameter to autogen_mcp run_eval fixture#111

Merged
andrewdonheiser merged 1 commit into
mainfrom
RHAIENG-5127-add-missing-stream-parameter-to-autogen-mcp-run-eval-fixture
May 20, 2026
Merged

fix: add missing stream parameter to autogen_mcp run_eval fixture#111
andrewdonheiser merged 1 commit into
mainfrom
RHAIENG-5127-add-missing-stream-parameter-to-autogen-mcp-run-eval-fixture

Conversation

@andrewdonheiser
Copy link
Copy Markdown
Contributor

@andrewdonheiser andrewdonheiser commented May 20, 2026

Summary

  • Adds stream: bool = False to the _run() inner function in the autogen_mcp run_eval fixture, aligning its signature with all other agents (react_agent, agentic_rag, etc.)
  • The parameter is accepted for interface consistency but not wired to TaskConfig — autogen always uses non-streaming for evals since tool_invocations are only exposed in non-streaming JSON responses

Test plan

  • Verify existing autogen_mcp behavioral tests still pass
  • Confirm _run() signature matches react_agent and agentic_rag patterns

Closes RHAIENG-5127

🤖 Generated with Claude Code

Align _run() signature with other agents (react_agent, agentic_rag)
for interface consistency. The parameter defaults to False and is not
wired to TaskConfig since autogen always uses non-streaming for evals.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 20, 2026

Review Change Stack

📝 Walkthrough

Walkthrough

The pull request adds an optional stream: bool = False parameter to the _run callable returned by the run_eval fixture in the test configuration module. The parameter is declared but not yet wired through to the TaskConfig creation, which continues to hardcode stream=False.

Changes

Test Fixture Parameter Expansion

Layer / File(s) Summary
Stream parameter addition to eval fixture
agents/autogen/mcp_agent/tests/behavioral/conftest.py
The _run callable returned by the run_eval fixture now accepts an optional stream: bool = False parameter to support future test-level control over streaming behavior, though the underlying TaskConfig creation still hardcodes stream=False.

🎯 1 (Trivial) | ⏱️ ~3 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: adding a missing stream parameter to the autogen_mcp run_eval fixture.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The pull request description clearly explains the change: adding a stream parameter to align the autogen_mcp fixture signature with other agents, including the rationale for not wiring it to TaskConfig.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch RHAIENG-5127-add-missing-stream-parameter-to-autogen-mcp-run-eval-fixture

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
agents/autogen/mcp_agent/tests/behavioral/conftest.py (1)

104-114: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

stream is accepted but silently ignored

_run() now advertises stream, but TaskConfig is still hardcoded to stream=False. This creates a misleading API: callers can pass stream=True and get no behavior change. Please either wire stream=stream or explicitly reject it (e.g., if stream: raise ValueError(...)) so test intent can’t silently drift.

As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."

🤖 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 `@agents/autogen/mcp_agent/tests/behavioral/conftest.py` around lines 104 -
114, The _run() function accepts a stream parameter but the TaskConfig is
constructed with stream=False, causing the parameter to be ignored; fix by
wiring the passed-in stream through to TaskConfig (set stream=stream) or
explicitly reject usage (e.g., in _run() check if stream: raise ValueError(...))
so callers cannot silently expect streaming behavior; update the TaskConfig
construction site (TaskConfig(..., stream=...)) and/or add the validation near
the start of _run().
🤖 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 `@agents/autogen/mcp_agent/tests/behavioral/conftest.py`:
- Around line 104-114: The _run() function accepts a stream parameter but the
TaskConfig is constructed with stream=False, causing the parameter to be
ignored; fix by wiring the passed-in stream through to TaskConfig (set
stream=stream) or explicitly reject usage (e.g., in _run() check if stream:
raise ValueError(...)) so callers cannot silently expect streaming behavior;
update the TaskConfig construction site (TaskConfig(..., stream=...)) and/or add
the validation near the start of _run().

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Enterprise

Run ID: 9c9818ab-53cb-48a4-bdd1-1aa33013bde5

📥 Commits

Reviewing files that changed from the base of the PR and between 2b8e6ed and 4f11e0e.

📒 Files selected for processing (1)
  • agents/autogen/mcp_agent/tests/behavioral/conftest.py

Copy link
Copy Markdown
Contributor

@mpk-droid mpk-droid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@andrewdonheiser andrewdonheiser merged commit f22644f into main May 20, 2026
8 checks passed
@andrewdonheiser andrewdonheiser deleted the RHAIENG-5127-add-missing-stream-parameter-to-autogen-mcp-run-eval-fixture branch May 20, 2026 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants