Repository navigation
fix(agent-server): preserve custom stuck detection thresholds - #6242
MuonChaser wants to merge 3 commits into
Conversation
Co-authored-by: openhands <openhands@all-hands.dev>
|
📁 PR Artifacts Notice This PR contains a |
Co-authored-by: openhands <openhands@all-hands.dev>
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
Co-authored-by: openhands <openhands@all-hands.dev>
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Scope: In-scope for this repository per the ownership boundaries in AGENTS.md — Python SDK conversation config, agent-server models/services, and the TypeScript client mirror. Issue #6240 is open with ready-for-dev and priority:medium; the fix matches the reported bug (server request schema silently dropping stuck_detection_thresholds).
Cross-layer trace (per the repository review checklist): The field flows through every supported path — SDK ConversationConfig/StartConversationRequest → server persistence (StoredConversation, inherits the field, so it lands in meta.json) → EventService.get_conversation() reconstruction into LocalConversation/StuckDetector → ConversationInfo response → fork path (fork copies the source's stored config via model_copy, so thresholds survive automatically) → TypeScript client (RemoteConversation.start(), ConversationManager.createConversation, and the ACP namespace all map camelCase to canonical snake_case and omit the key entirely when unset, preserving server defaults). Old metadata without the field loads as None (defaults retained), and ge=1 validation now rejects invalid thresholds with 422 before a conversation is created. No threshold-update endpoint is added, consistent with stuck_detection itself also not being updatable via UpdateConversationRequest.
Compatibility: Additive, optional, default None; no breaking change to existing payloads or persisted state. The pinned generated TypeScript contract staying at its 1.58.2 release pin matches the documented release-workflow ownership, and older servers discarding the field is documented in the client README.
Verification on this head (d3459f01a):
tests/cross/test_remote_conversation_live_server.py -k stuck_threshold: 5 passed — real Uvicorn server, real HTTP/agent/run loop; thresholds 2/6/default/disabled stop at 2/6/4/7 ThinkTool calls across create, disk-reload (including old metadata without the field), and server-fork stages; invalidaction_observation: 0returns 422 with no conversation created.tests/agent_server/test_conversation_service.py+test_agent_launch_additions.py+test_agent_profile_conv_start.py: 209 passed. (Note: with the local review harness'sOH_RUNTIME_LAUNCHED_PROFILEenvironment variable injected, one pre-existing launch-additions assertion fails; I reproduced the identical failure onorigin/main, confirming it is an environment artifact, not a PR regression.)tests/cross/test_stuck_detector.py,test_stuck_detector_config.py,test_openapi_contract.py: 20 passed;make test-server-schemapasses.- TypeScript: 396 tests passed, build passes,
check:agent-server-apiconfirms the pinned checked-in contract is current. - All GitHub Actions checks on this exact head are green (agent-server-tests, sdk-tests, cross-tests, pre-commit, OpenAPI schema, TypeScript suites).
No material correctness, security, or compatibility findings. The live before/after evidence satisfies the runtime-evidence requirement for this production-facing fix.
✅ APPROVED
HUMAN:
The fix preserves custom stuck detection thresholds across remote conversation creation, reloads, and server forks, with matching TypeScript support. Existing defaults and disabled behavior remain unchanged; detector heuristics and unrelated local-fork fixes are outside the scope.
AGENT:
Why
The remote SDK sends
stuck_detection_thresholds, but the server request model drops it. In an independent live HTTP reproduction on maince5716a3c, requested observation thresholds 2 and 6 both stopped after 4 ThinkTool calls; an invalid threshold of 0 was accepted with HTTP 201.Summary
StuckDetectionThresholdsin shared conversation config, persisted metadata, and conversation info; forward it when EventService builds LocalConversation.Companion documentation: OpenHands/docs#1012
Issue Number
Fixes #6240
How to Test
Local results on WSL Linux/Python 3.13.17 with the committed uv lock:
6edd54d48: all five Python suites listed above pass together: 205 passed, 1 skipped. This includes both upstream live-server additions and the threshold create/reload/fork controls.Only completion responses are scripted with TestLLM; HTTP, Agent/ThinkTool, run-loop, events and persistence are real. No paid inference, real-model benchmark, GPU, Docker or packaged-binary test was run.
Video/Screenshots
Non-visual change. Before/after evidence and copyable commands.
Design Doc
Small config propagation repair; the request/stored config and reconstruction path remain the owners. No detector redesign or separate design document.
Type
Notes
AI-assisted contribution. The contributor has filled the HUMAN section; that text is preserved unchanged. Main
6edd54d48was merged in commit10b768720, resolving the live-server import/append conflict while retaining both regression suites.No dependency on #6239: the server fork copies stored config and reconstructs EventService, and the regression passes on main plus this patch alone. Local in-process fork configuration loss and scan-capacity/heuristic changes remain outside scope (#6238/#6239). No threshold-update endpoint is added.
The checked-in generated TypeScript contract remains at its required release pin (1.58.2); the release workflow will regenerate it after a release containing this additive field. Older servers continue to discard this option, as the documentation explains. Fresh ownership/duplicate checks found only our issue claim and no same-fix PR before submission.
CI follow-up: the previous head (
10b768720) had 34 launch/profile failures because two test doubles returned an unspecified MagicMock for the new threshold field. Commitd3459f01amakes both fixtures retain the actual StoredConversation supplied by the service. Local reproduction went from 34 failed / 34 passed to 68 passed; the two files plus conversation-service tests pass 209/209 with the committed uv lock, and changed-file pre-commit passes. Production code and dependencies are unchanged by this follow-up.Current-head CI confirms the repair: agent-server-tests passes 2,884 tests (one skipped), sdk-tests passes 7,704 tests (seven skipped, 12 xfailed), and cross-tests passes 604 tests (one skipped). Raw logs were checked. The previous head's finite-clock-mock SDK failure also passes on this head.