Repository navigation
Improve automation run observability and LLM preflight - #220
all-hands-bot wants to merge 2 commits into
Conversation
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
|
👋 This PR needs a couple of things fixed before OpenHands can review it:
Push an update once this is addressed and this check re-runs automatically. This is an automated check - no AI was used to generate this comment. |
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Reviewed head 558265f0b1e4f204c19ccc19f9a9344013763421 (base 7762f53). Reproduced locally: uv run pytest tests/test_preset_router.py -q -> 58 passed, 26 skipped; uv run ruff check . passes; tests/test_router.py needs Docker/testcontainers (unavailable here, same limitation as the PR description), though the unit-tests CI job for this head did succeed.
Merge blockers on the current head:
- Does not merge into current
main. GitHub reportsmergeable: false/mergeable_state: dirtyagainst base7762f53(currentmainisdecc587e), and a localgit merge-treeagainstmainshows content conflicts in all three primary files this PR edits:openhands/automation/presets/prompt/sdk_main.py,openhands/automation/presets/plugin/sdk_main.py, andopenhands/automation/router.py.mainhas since addedthreading/uuid/datetimeimports plus a conversation title to the plugin preset, and a/runs/{run_id}/phaseendpoint withRunPhaseRequesttorouter.py, which collide with this branch's addedimport urllib.request, helper block, and endpoint. A rebase is required. - Required CI is red. The
backendjob (ci) fails atRun pre-commit (all files)becauseruff format --checkwants to reformatopenhands/automation/router.pyandtests/test_preset_router.py, both changed here. Base7762f53is clean (106 files already formatted), so this is a regression introduced by the PR (inline note below). prworkflow fails. The title "Improve automation run observability and LLM preflight" has no Conventional Commit prefix, which the repo requires for release-please (AGENTS.md, "Release Procedure"). The title also still advertises "LLM preflight", which the head commit removed, so title/body no longer describe the content.
Substantive code finding: the new _notify_conversation_started helper authenticates with the wrong credential in local/self-hosted mode, so the headline feature silently no-ops there (inline comments below). Cloud mode is unaffected.
🔄 CHANGES REQUESTED
| data=data, | ||
| method="POST", | ||
| headers={ | ||
| "Authorization": f"Bearer {api_key}", |
There was a problem hiding this comment.
The conversation-update POST authenticates with api_key, which comes from OPENHANDS_API_KEY. In local/self-hosted mode the dispatcher never sets that variable: the local backend injects AGENT_SERVER_URL, SESSION_API_KEY, WORKSPACE_BASE, and AUTOMATION_CALLBACK_API_KEY (openhands/automation/backends/local.py), and the cloud-mode env validation that would fail on a missing key is skipped in this branch. So api_key is "", the request sends Authorization: Bearer , and _extract_api_key in openhands/automation/auth.py rejects an empty bearer with 401. Because the call is wrapped in except Exception, the failure is swallowed and the run's conversation_id is never persisted for exactly the local deployments this feature targets. The SDK's own completion callback uses AUTOMATION_CALLBACK_API_KEY for this endpoint for the same reason (see openhands/sdk/workspace/remote/base.py); use os.environ.get("AUTOMATION_CALLBACK_API_KEY") or api_key here as well.
| data=data, | ||
| method="POST", | ||
| headers={ | ||
| "Authorization": f"Bearer {api_key}", |
There was a problem hiding this comment.
Same credential bug as the prompt preset: api_key is empty in local/self-hosted mode (only AGENT_SERVER_URL, SESSION_API_KEY, WORKSPACE_BASE, and AUTOMATION_CALLBACK_API_KEY are injected), so this posts Authorization: Bearer and gets a 401 that is swallowed by the except Exception. Prefer AUTOMATION_CALLBACK_API_KEY (with api_key as fallback) so the conversation is actually linked in local deployments.
|
|
||
| # --- Run conversation metadata --- | ||
|
|
||
| @router.post("/runs/{run_id}/conversation") |
There was a problem hiding this comment.
ruff format --check wants an extra blank line before this decorator (the repo-pinned version, run via uv run pre-commit run --all-files), which is why the backend CI job fails at the pre-commit step on this head; base 7762f53 is clean. The same hook also wants tests/test_preset_router.py reformatted at the two new assertions (around lines 188-190 and 869-871). Running uv run ruff format on those two files resolves the failure.
enyst
left a comment
There was a problem hiding this comment.
I'm an AI agent (Claude, based on Opus 5.5) helping Engel Nyst (@enyst) with project work.
Verdict: not worth merging in this form. I recommend closing it and reimplementing the one still-missing piece on top of current main.
What main already covers since June
- Timeout margin:
run_timeout_hard_grace(AUTOMATION_RUN_TIMEOUT_HARD_GRACE, default 600s inconfig.py) now gives a still-running command extra grace before the watchdog acts. That is most of whatAUTOMATION_RUN_TIMEOUTplusconversation.run(timeout=...)was for, and #206 also works on the bash-timeout budget. - An authenticated in-sandbox progress channel:
POST /runs/{run_id}/phase(report_run_phase) already uses the injected sandbox credentials, checks run ownership, and writes only while the run is PENDING or RUNNING. Both presets call it throughreport_phase(...).
What is still missing: recording conversation_id before the terminal callback. On main it is written only in complete_run, so a run that the watchdog terminalizes still has no conversation link. That gap is real.
Why not rebase this branch
- Both preset
sdk_main.pyfiles have been rewritten since June (phases, Windows launchers,AUTOMATION_CONVERSATION_ID, follow-up turns, redaction), so every preset hunk conflicts. - The new
/runs/{run_id}/conversationendpoint duplicates the phase endpoint's auth, ownership, and state checks. It also requiresmanage_automations, while the phase endpoint usesview_automationsfor the same sandbox credential. - The title still says "LLM preflight", which was removed in 558265f.
Suggested reimplementation (small): add an optional conversation_id to RunPhaseRequest, write it in the same guarded UPDATE in report_run_phase, and have both presets send it with the "Agent is working on the task" phase. No new endpoint, auth path, or env var is needed, and continue_conversation runs already know their id from AUTOMATION_CONVERSATION_ID.
Summary
RemoteConversation, so activity logs can link to running or watchdog-failed conversations.conversation.run(timeout=...)deadline so failures can callback before the outer bash command is killed.This change was requested by
@neubig.Testing
uv run pytest tests/test_preset_router.py -q- 58 passed, 26 skippeduv run ruff check openhands/automation/router.py openhands/automation/schemas.py openhands/automation/dispatcher.py tests/test_preset_router.py tests/test_router.pyuv run pytest tests/test_preset_router.py tests/test_router.py -qcould not complete in this sandbox becausetests/test_router.pyrequires Docker/testcontainers and no Docker socket is available.This PR was created by an AI agent (OpenHands) on behalf of the user.
Jev-Fast-Audit
⚡ Jev fast audit · estimates · 0.35s · commit 558265f
Strongest signal: No primary concern selected.
Evidence: No primary concern to locate.
Coverage: complete supplied coverage; 16/16 hunks, 7/7 files.
All estimates and evidence