Conversation
|
Warning Your comment is too long (maximum is 65536 characters), so the coverage report was not added. See the job log for how to reduce it. |
8e0d917 to
fd1a372
Compare
f40f85d to
c0f6da8
Compare
Co-authored-by: openhands <openhands@all-hands.dev>
c0f6da8 to
c985c66
Compare
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Verdict: Worth merging
Good taste — The PR cleanly extends the existing run/dispatch infrastructure to support service-owned agent turns without introducing special-case spaghetti. The data structure additions (subject_source, conversation_turn, conversation_wake_agent) are minimal and well-scoped. Subject routing is now namespaced by source via the advisory lock digest and the partial index, preventing cross-integration key collisions. The dispatcher's concurrency gate correctly separates scanner runs (blocked from overlapping) from independent agent-turn runs (allowed to fan out).
Key observations
Migration (027): Cross-database compatible — uses generic SQLAlchemy types, conditional SQL for the backfill (json_extract for SQLite, ->> for PostgreSQL), and conditionally applies the partial index predicate for both dialects. The drop-and-recreate of ix_automation_runs_subject to include subject_source is correct.
Conversation ID persistence: Moving the conversation_id and sandbox_id persistence to before the turn execution (dispatcher.py:369-380) is the right call — follow-up turns from continue_conversation can route to the correct conversation as soon as it starts, rather than waiting for bundle completion.
Subject release flow: The success path does not set subject_released_at, which is consistent with existing conversation-run behavior. Follow-up events that find a COMPLETED run attempt delivery; if the runtime is gone (Docker released), delivery fails and the subject is released, falling back to a fresh run. For local runtime, the conversation persists and turns are delivered successfully. Both paths are correct.
Error path: On turn failure, the subject is released, the runtime context is released, and the run is marked FAILED with structured status detail. On success, release_context failures are logged but don't fail the run — conversation history is persisted outside the runtime, with TTL cleanup as fallback. This is pragmatic.
Runtime scope: _run_turn correctly passes runtime_conversation_id to RemoteWorkspace, ensuring agent tools execute in the provisioned Docker/local runtime rather than the host. The test test_service_owned_turn_keeps_the_runtime_scope verifies this.
Dependency: openhands-tools is added from the same first-party git ref as openhands-sdk, satisfying the PR's stated SDK dependency. First-party, same-org — no 7-day waiting period applies.
[RISK ASSESSMENT]
- Overall PR: LOW
- New code path (agent-turn dispatch) is cleanly separated from the existing bundle-execution path via the conversation_turn is not None guard.
- Migration is additive (nullable columns, index recreation) with a correct downgrade.
- No new external attack surface — the turn text originates from the scanner script running inside the service's own sandbox, not from untrusted external input.
- Test coverage spans poller fan-out, dispatch skip-bundle-and-release, runtime scope, subject isolation, and SQLite migration.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger and the context the reviewer is missing.
- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run /iterate to automatically drive this PR through CI, review, and QA until it's merge-ready.
Was this review helpful? React with thumbs up or thumbs down to give feedback.
Automated review used the wrong decision (APPROVED instead of COMMENT) and is dismissed. Findings are reposted as a comment.
|
Review posted to GitHub PR #467 as APPROVED (review ID: 5203663668). Summary: The PR adds service-owned agent-turn runs that let a scanner select independent agent jobs without turning its own run into a conversation. The review found no material issues:
Risk assessment: 🟢 LOW. Verdict: ✅ Worth merging. This comment was posted by an AI agent (OpenHands). |
|
@all-hands-bot The current head is CI-clean and has now been validated in the isolated live Canvas: a host scanner provisioned a Docker subject, attached through the SDK, completed triage, and released the runtime. Please explicitly approve this pull request if there are no blocking findings. |
|
@neubig I've reviewed the head commit ( Review summaryI re-read the full diff against the base branch (
Risk assessment: 🟢 LOW. No blocking findings — approved. Generated by OpenHands AI on behalf of the user. |
Why
A deterministic scanner must be able to select several independent agent jobs without turning its own automation run into a conversation. The selected work also needs normal run tracking and bounded runtime cleanup.
Summary
Add a tracked, service-owned agent-turn run built by the existing run utilities. The dispatcher provisions it through the existing conversation backend, executes the stored turn through software-agent-sdk, records the normal terminal state and telemetry, and releases its Docker runtime after completion while retaining conversation history.
Local admission still prevents overlapping scanner executions for one definition, while allowing independent agent-turn runs to fan out up to the existing conversation concurrency limit. Subject routing is now namespaced by source, so equal opaque keys from different integrations cannot collide.
Issue Number
Closes #465.
How to Test
105 focused dispatcher, conversation backend, follow-up routing, source isolation, runtime scope, and SQLite migration tests pass. Formatting, Ruff, pycodestyle, and Pyright pass.
Review order and dependencies
Stack #454: #449 → #453 → #466 → #467 → #468. Review and merge in that order. This PR uses the explicit script/agent boundary introduced by #466. It also requires software-agent-sdk #5069 so the lightweight Automation process can attach to a tool-enabled runtime conversation without importing role-specific schemas; the runtime tool package is installed alongside the SDK.