Skip to content

fix(agent-server): preserve custom stuck detection thresholds - #6242

Open
MuonChaser wants to merge 3 commits into
OpenHands:mainfrom
MuonChaser:codex/6240-remote-stuck-thresholds
Open

MuonChaser wants to merge 3 commits into
OpenHands:mainfrom
MuonChaser:codex/6240-remote-stuck-thresholds

Conversation

@MuonChaser

@MuonChaser MuonChaser commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

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 main ce5716a3c, requested observation thresholds 2 and 6 both stopped after 4 ThinkTool calls; an invalid threshold of 0 was accepted with HTTP 201.

Summary

  • Reuse StuckDetectionThresholds in shared conversation config, persisted metadata, and conversation info; forward it when EventService builds LocalConversation.
  • Preserve configuration through disk reload, fresh-service restart, and server forks. Accept older metadata without the field and retain default/disabled behavior.
  • Map TypeScript camelCase options to canonical snake_case payloads in RemoteConversation and ConversationManager, including the shared ACP request path.

Companion documentation: OpenHands/docs#1012

Issue Number

Fixes #6240

How to Test

make build
LITELLM_LOCAL_MODEL_COST_MAP=True uv run --frozen pytest \
  tests/cross/test_remote_conversation_live_server.py \
  tests/cross/test_stuck_detector.py \
  tests/cross/test_stuck_detector_config.py \
  tests/agent_server/test_openapi_contract.py -q
LITELLM_LOCAL_MODEL_COST_MAP=True uv run --frozen pytest \
  tests/agent_server/test_conversation_service.py -q
make test-server-schema
(cd clients/typescript && npm ci && npm run lint && npm run build && npm test && npm run check:agent-server-api)

Local results on WSL Linux/Python 3.13.17 with the committed uv lock:

  • Revalidated after merging main 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.
  • Native stopping counts for thresholds 2/6/default/disabled: 2/6/4/7, repeated after disk reload and server fork. Invalid input returns 422 before creating a conversation. The old metadata format also reloads.
  • Original separate-process validation retained thresholds across process restart and server fork. The built TypeScript client sent all four values to that server and read them back correctly. The process was stopped and its port closed.
  • TypeScript: 396 passed, build/lint pass (5 existing lint warnings); public weak-type budget unchanged. Canonical deterministic OpenAPI/type-quality/Swagger checks and pinned TypeScript schema regeneration pass. Candidate OpenAPI threshold types also pass mutual TypeScript assignability with the transport payload.
  • All applicable changed-file pre-commit hooks pass, including Pyright and import rules.

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

  • Bug fix
  • Feature
  • Refactor
  • Breaking change
  • Docs / chore

Notes

AI-assisted contribution. The contributor has filled the HUMAN section; that text is preserved unchanged. Main 6edd54d48 was merged in commit 10b768720, 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. Commit d3459f01a makes 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.

LITELLM_LOCAL_MODEL_COST_MAP=True uv run --frozen pytest   tests/agent_server/test_agent_launch_additions.py   tests/agent_server/test_agent_profile_conv_start.py   tests/agent_server/test_conversation_service.py -q

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.

Co-authored-by: openhands <openhands@all-hands.dev>
@github-actions

Copy link
Copy Markdown
Contributor

📁 PR Artifacts Notice

This PR contains a .pr/ directory with temporary PR-specific documents. The directory will be automatically removed after this PR merges to main through a cleanup PR.

Co-authored-by: openhands <openhands@all-hands.dev>
@MuonChaser
MuonChaser marked this pull request as ready for review October 27, 2026 16:33
Copilot AI balanced review requested due to automatic review settings October 27, 2026 16:33

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 all-hands-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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; invalid action_observation: 0 returns 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's OH_RUNTIME_LAUNCHED_PROFILE environment variable injected, one pre-existing launch-additions assertion fails; I reproduced the identical failure on origin/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-schema passes.
  • TypeScript: 396 tests passed, build passes, check:agent-server-api confirms 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

@all-hands-bot
all-hands-bot requested a review from enyst October 29, 2026 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Agent server silently drops custom stuck detection thresholds

3 participants