Fix _GatedLLM.completion override after _return_metrics removal#3794
Fix _GatedLLM.completion override after _return_metrics removal#3794VascoSch92 wants to merge 1 commit into
Conversation
The `_return_metrics` parameter was removed from LLM/TestLLM.completion in 2bedbd8 (removed_in 1.29.0), but tests/agent_server/test_goal_loop.py's _GatedLLM override (added later in #3770) still declared it as a positional param and forwarded it to super().completion(), which pyright flagged as an incompatible override and a 5-positional call against a 4-positional signature. Drop _return_metrics to match the current signature.
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Coverage Report •
|
|
Closing — this targeted |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
❌ QA Report: FAIL
The PR removes _return_metrics from _GatedLLM.completion, but the actual PR base still expects that parameter and the changed helper regresses the current completion call shape.
Does this PR achieve its stated goal?
No. The stated goal is to align _GatedLLM.completion with TestLLM.completion after _return_metrics removal, but in this PR's checked-out base/head environment TestLLM.completion still includes _return_metrics. Exercising the current positional completion signature succeeds on the base commit and raises a TypeError on the PR head; the existing CI pre-commit check also reports the same incompatible override.
| Phase | Result |
|---|---|
| Environment Setup | ✅ make build completed successfully and installed the uv-managed dev environment. |
| CI Status | ❌ PR is closed unmerged; existing checks show pre-commit failed with pyright override errors and PR description validation failed. |
| Functional Verification | ❌ The PR head fails a completion call shape that works on the base commit. |
Functional Verification
Test 1: Current TestLLM.completion positional call shape
Step 1 — Reproduce / establish baseline without the fix:
Checked out base commit 9e290793addcf7397bce315b8c85803a8b70f804 and ran a scripted _GatedLLM.completion(...) call using the current positional signature:
uv run python - <<'PY'
from openhands.sdk.llm import Message
from tests.agent_server.test_goal_loop import _GatedLLM
def on_token(token: str) -> None:
pass
llm = _GatedLLM(model='test-model')
llm.__pydantic_private__['_scripted_responses'].append(
Message(role='assistant', content='Hello from scripted LLM')
)
response = llm.completion(
[Message(role='user', content='Say hello')],
None,
False,
True,
on_token,
)
print('response_type', type(response).__name__)
print('message_text', response.message.content[0].text)
PYObserved:
response_type LLMResponse
message_text Hello from scripted LLM
This shows the base helper accepts the current TestLLM.completion positional call shape and returns the scripted LLM response.
Step 2 — Apply the PR's changes:
Checked out fix/goal-loop-test-return-metrics-signature at 5239de8a6dd65a9e6082d100d48a11b6bbb27db3.
Step 3 — Re-run with the fix in place:
Ran the same scripted completion call on the PR head:
Traceback (most recent call last):
File "<stdin>", line 11, in <module>
TypeError: _GatedLLM.completion() takes from 2 to 5 positional arguments but 6 were given
This shows the PR head does not align with the actual base signature in this checkout; it rejects a call shape that worked before.
Test 2: Runtime signature alignment
On PR head, I inspected the runtime signatures without running pyright:
TestLLM.completion (self, messages: 'list[Message]', tools: 'Sequence[ToolDefinition] | None' = None, _return_metrics: 'bool' = False, add_security_risk_prediction: 'bool' = False, on_token: 'TokenCallbackType | None' = None, **kwargs: 'Any') -> 'LLMResponse'
_GatedLLM.completion (self, messages, tools=None, add_security_risk_prediction=False, on_token=None, **kwargs)
This confirms the PR head's override is missing _return_metrics while the base class still has it in this PR context.
Existing CI observation
I did not rerun tests, pre-commit, linters, or pyright locally. I only inspected the existing GitHub check log, which reports:
Type check with pyright..................................................Failed
/home/runner/work/software-agent-sdk/software-agent-sdk/tests/agent_server/test_goal_loop.py:42:9 - error: Method "completion" overrides class "TestLLM" in an incompatible manner
Positional parameter count mismatch; base method has 7, but override has 6
Issues Found
- 🔴 Blocker: The PR does not achieve its goal in the actual PR base/head context:
_GatedLLM.completionno longer matchesTestLLM.completion, and a real scripted completion call using the current positional signature now raisesTypeError.
This QA report was created by an AI agent (OpenHands) on behalf of the user.
| messages, | ||
| tools=None, | ||
| _return_metrics=False, | ||
| add_security_risk_prediction=False, |
There was a problem hiding this comment.
🔴 Critical: Functional QA shows this override no longer accepts the current TestLLM.completion positional signature in this PR's base. Calling _GatedLLM.completion(messages, None, False, True, on_token) succeeds on the base commit but raises TypeError: _GatedLLM.completion() takes from 2 to 5 positional arguments but 6 were given on this head. The existing CI pyright log reports the same mismatch: the base method still has _return_metrics in this PR context, so this change does not achieve the stated goal.
This inline finding was created by an AI agent (OpenHands) on behalf of the user.
4 participants
Problem
Pre-commit (
pyright) fails onmain/rel-1.29.0with two errors intests/agent_server/test_goal_loop.py:This is a logical merge conflict between two independently-green changes that landed on 2026-06-18:
2bedbd8f"Remove acp_env and_return_metricsdeprecations (removed_in 1.29.0)" — deleted the_return_metricsparameter fromLLM.completion/TestLLM.completion(now 4 positional params).test_goal_loop.py, whose_GatedLLM.completionwas written against the old signature (with_return_metrics) and was CI'd before the removal merged.Neither PR was red on its own; the combined tree is the first place pyright type-checks both together.
Fix
Drop
_return_metricsfrom the_GatedLLM.completionoverride — both the signature and thesuper().completion(...)call — to match the current 4-positional signature. No behavior change.Agent Server images for this PR
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.13-nodejs22-slimgolang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:5239de8-pythonRun
All tags pushed for this build
About Multi-Architecture Support
5239de8-python) is a multi-arch manifest supporting both amd64 and arm645239de8-python-amd64) are also available if needed