feat: run scanner-selected conversation turns - #467

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:

1. 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.
2. Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
3. 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.

Approval review

This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.

Summary

This PR introduces conversation-turn runs: scanner-selected tasks that are dispatched as conversation-scoped child runs through the existing ConversationBackend, with completion detected via the SDK's ConversationExecutionStatus and cleanup handled by the existing watchdog.

The design is sound and well-structured:

• Conversation linking after first turn acceptance: The run-to-conversation link is persisted only after submit_conversation_turn succeeds (_save_conversation_id), preventing the watchdog from mistaking container startup for a missing conversation. This directly addresses the startup race described in the PR.
• Verification via execution status: verify_run reads ConversationExecutionStatus from the agent server. I confirmed in the SDK source that FINISHED is set when the agent produces a message response (response_dispatch._handle_message_response), so the watchdog correctly detects task completion. IDLE, RUNNING, PAUSED, WAITING_FOR_CONFIRMATION, and DELETING all fall through to STILL_RUNNING, which is correct — they represent in-flight or paused states.
• Subject source namespacing: Including source in the advisory lock hash and the subject index prevents cross-integration key collisions. The migration backfills subject_source from the automation trigger for existing rows.
• Run-scoped admission control: The dispatcher prevents overlapping run-scoped commands per definition while allowing conversation-scoped runs to fan out independently up to the configured capacity. The capacity logic correctly leaves batch_size unmodified when conversation capacity is exhausted, so run-scoped PENDING runs are still fetched.
• Error handling: Turn submission failure releases the subject (subject_released_at), releases the runtime context, and marks the run FAILED with structured status detail — all proper cleanup paths.
• Migration: Cross-database compatible (SQLite json_extract vs PostgreSQL ->>), recreates the partial index with the new subject_source column, and the downgrade restores the original index.

Risk Assessment

🟡 MEDIUM — The PR introduces a new execution path (conversation-turn dispatch), a database migration with index changes, and new watchdog verification logic. However, the design is well-reasoned, the changes are additive (existing bundle runs are unaffected), and test coverage is comprehensive (dispatcher fan-out, turn verification, watchdog timing, migration, source isolation). The live evidence in the PR description demonstrates end-to-end functionality.

Verdict

✅ Worth merging — No material issues found. The architecture is clean, the race condition handling is correct, and the migration is safe.