feat: add scoped subject-turn submission

[neubig]

Why

Scheduled automation scripts can identify work, but they have no general way to ask the Automation service to create or resume the agent conversation for a stable external subject. Extension workers consequently duplicate conversation attachment, runtime credentials, and local/Docker lifecycle logic.

The API must also recover after an attempt never completes. A paused automation can skip queued subjects, and a running subject can time out. Permanently deduplicating later submissions against either outcome leaves the external issue or PR stuck forever.

Summary

Add a scoped POST /v1/runs/{run_id}/subject-turns operation. A running automation supplies a source, opaque subject key, prompt, and idempotency key. The service reuses existing subject locking, coalescing, deterministic conversation identity, and the tracked agent-turn run from #467. It reports whether work was created, queued, delivered, or deduplicated.

Repeated submissions deduplicate queued, running, successful, and not-yet-released work. Once a failed, canceled, or skipped attempt no longer owns a runtime, the same idempotency key creates another tracked attempt in the same deterministic conversation. The one unique idempotency record follows the current attempt; superseded attempts remain in run history.

The caller receives a short-lived capability token limited to subject submission for its own run and automation. It never receives conversation credentials and cannot choose an arbitrary conversation ID. Extension code only selects work; the Automation service owns conversation and runtime lifecycle.

Issue Number

Closes #463.
Closes #470.

How to Test

• The focused subject-turn suite covers authorization, profile requirements, idempotency, source isolation, queued coalescing, continuation, retryable terminal outcomes, and unreleased-runtime protection. All 12 tests pass.
• Formatting and Ruff pass for the changed files; the wider dispatcher and migration coverage remains in CI.
• Live Agent Canvas evidence: pausing the developer left Airbnb issues #59–#62 attached to skipped runs, while #64 pointed to a timed-out run. Before this fix, another UI-triggered scan deduplicated all five and launched no agent. After restarting the same persisted Canvas on this head, one UI-triggered scan created five replacement runs with the same deterministic conversation IDs and immediately admitted two Docker-backed agents at the configured concurrency limit.

Review order and dependencies

Stack #454: #449 → #453 → #466 → #467 → #468. Review and merge in that order.

[github-actions]

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.

[neubig]

@all-hands-bot Please review the current head and explicitly approve it if there are no blocking findings.

[all-hands-bot]

🤖 OpenHands is reviewing this PR.

Head commit: cc761566516e208d38cc9782b5f4dc2edf0a4862
View the conversation: https://oss-agent-canvas.ngrok.dev/conversations/38a864be-e389-4098-ba6e-3a1e7c4caea5

This comment was posted by an AI agent (OpenHands).

[all-hands-bot]

Review of cc76156 — feat: add scoped subject-turn submission

I reviewed the full diff against the merge base (c985c66, factory/agent-turn-runs) and ran Ruff lint + format checks on all changed files. The test suite requires Docker/PostgreSQL via testcontainers, which is not available in this environment, so I could not execute the 11 subject-turn tests locally — but I verified every code path statically against the existing models, helpers, and config.

What the PR does

Adds POST /v1/runs/{run_id}/subject-turns — a scoped, idempotent endpoint that lets a running automation script fan out agent work for external subjects without ever receiving conversation credentials or managing runtime lifecycle.

Findings by area

Security — run token (utils/run_token.py, subject_router.py)

• HS256 JWT signed with the deployment service_key (falling back to local_api_key), carrying automation_id, run_id, scopes, iat, and exp (24h).
• verify_run_token checks signature, expiration, and required scope. The endpoint then enforces that the token's run_id matches the path, automation_id matches the loaded run's automation, and the requester run is RUNNING.
• The dispatcher injects AUTOMATION_RUN_TOKEN + AUTOMATION_SUBJECT_TURN_URL only when a signing secret is configured; otherwise it silently skips (graceful degradation). No blocking issues.

Idempotency model (models.py, migration 028)

• AutomationSubjectTurn table with a UNIQUE(automation_id, source, subject_key, idempotency_key) constraint and CASCADE foreign keys to automations and automation_runs.
• Migration 028 (revision 028, down_revision 027) matches the ORM definition exactly. The chain 026 → 027 → 028 is intact.

Core logic — submit_subject_turn (conversations.py)

• Subject lock (pg_advisory_xact_lock) is taken before the idempotency check, so a retry cannot enqueue two conversations.
• Dedup path: if the idempotency record exists and the prior run is still active or not yet released → deduplicated.
• Retry path: if the prior run is FAILED/CANCELLED/SKIPPED and has released its runtime (or never started) → a new attempt is created in the same deterministic conversation, and the single idempotency record is repointed to the new run. The superseded run stays in history.
• The never-started case (started_at is None) correctly sets subject_released_at so the old run is excluded from _lock_subject_run's lookup.
• compose_turn receives event_payload=None with override=turn, so the caller's prompt is used verbatim — confirmed by reading conversation_turn.py:159.
• The guard if subject_run is not None and subject_run.status not in _FINISHED: raise RuntimeError correctly maps to 503 for an unreachable conversation.

Endpoint (subject_router.py)

• 401 for missing/invalid token, 403 for token belonging to another run or automation, 409 when the requester is not RUNNING, 422 for invalid subject key, 503 for runtime unavailability, 202 on success.
• Session lifecycle is handled by Depends(get_session). Correct.

Dispatcher injection (dispatcher.py)

• AUTOMATION_SUBJECT_TURN_URL = {resolved_base_url}/v1/runs/{run_id}/subject-turns, and resolved_base_url = {base}/api/automation. The router is mounted at _base_path (/api/automation) with prefix /v1/runs. The paths match.

Schema validation (schemas.py)

• SubjectTurnRequest uses extra="forbid", enforces length bounds, strips whitespace, and rejects empty strings. SubjectTurnResponse returns disposition, run_id, conversation_id.

Test coverage (test_subject_turns.py)

• 11 tests covering: first-turn creation, same-key dedup, retry after released failure (parametrized), retry of never-started skipped run, wait-for-release, source isolation, endpoint auth (403/202), and continuation without a second run. Good coverage of the critical paths.

Lint/format: ruff check and ruff format --check pass on all 8 changed files.

Verdict

No blocking findings. Approving.

Generated by OpenHands AI on behalf of the user.

[all-hands-bot]

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

Summary

This PR adds a scoped POST /v1/runs/{run_id}/subject-turns endpoint that lets a running automation script submit work for an external subject (issue, PR, ticket) without receiving conversation credentials or managing runtime lifecycle. The service owns conversation identity, profile selection, and runtime attachment. The implementation reuses existing subject locking, coalescing, deterministic conversation IDs, and the agent-turn run machinery from #467.

Analysis

Security model is sound. The run token is a short-lived (24h) HS256 JWT scoped to subject_turn:submit and bound to a specific automation_id + run_id. The router validates the bearer token, checks scope, verifies run_id and automation_id match the URL path and DB record, and requires the requester run to be RUNNING. The script inside the sandbox cannot forge tokens for other runs or automations, and cannot choose an arbitrary conversation ID.

Idempotency and retry logic is well-designed. The unique constraint on (automation_id, source, subject_key, idempotency_key) prevents duplicates at the DB level. The transaction-scoped advisory lock (pg_advisory_xact_lock) serializes concurrent submissions for the same subject, so the SELECT-then-INSERT idempotency check is race-free. The retry path correctly releases failed/skipped runs from the subject lookup (subject_released_at) before creating a replacement, and reuses the deterministic conversation ID so the new attempt continues the same conversation. The single idempotency record is repointed to the new attempt while the superseded run remains in history.

Migration is cross-database compatible. Uses generic sa.Uuid, sa.String, sa.DateTime types. The unique constraint creates an implicit index that covers the idempotency lookup query. The test_migrations_run_on_sqlite test is updated.

Test coverage is strong. 9 test functions (11 with parameterization) cover first-turn creation, deduplication, retry of released failed/canceled/skipped runs, retry of a skipped run that never started, waiting for a failed run that hasn't released, source isolation, token-for-another-run rejection, endpoint acceptance, and continuation without a second run. The tests exercise real DB state and assert on outcomes, not just mocked calls.

Minor Observations (non-blocking)

1. No test for the "queued" disposition. The queued path (turn coalesced into a not-yet-started run) is not directly tested. It's exercised indirectly through _continue_conversation_locked's coalescing logic, but a test that submits two turns for the same subject before the first run is dispatched would close the gap.

2. Token signing key reuse. signing_secret() falls back to service_key (used for per-user API key minting) or local_api_key. The KV token uses a separate kv_secret. Key separation would be marginally better, but both secrets are server-side and never exposed to users, so the risk is theoretical. This follows the same pragmatic pattern as other service-level operations.

Risk Assessment

🟢 LOW — The change adds a new endpoint and table without modifying existing behavior. The dispatcher injection is additive (wrapped in try/except, silently disabled when no secret is configured). The security model is properly scoped. Race conditions are handled with advisory locks and unique constraints. No breaking changes to existing APIs.

Verdict

✅ Worth merging — The design is clean, the security model is sound, and the retry/idempotency logic is correct. The minor testing gap for the "queued" disposition is non-blocking.

---

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 (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.
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 👍 or 👎 to give feedback.

Automated review used the wrong decision (APPROVED instead of COMMENT) and is dismissed. Findings are reposted as a comment.

[all-hands-bot]

Successfully posted an APPROVE review to PR #468 (OpenHands/automation).

Review summary: The PR adds a scoped POST /v1/runs/{run_id}/subject-turns endpoint allowing running automation scripts to submit work for external subjects. The review found no critical issues:

• Security model is sound — scoped JWT tokens bound to specific automation_id + run_id, proper authorization checks in the router
• Idempotency and retry logic is correct — transaction-scoped advisory locks prevent races, unique constraint prevents duplicates, retry path properly releases failed runs before creating replacements
• Migration is cross-database compatible — generic SQLAlchemy types, works on both PostgreSQL and SQLite
• Test coverage is strong — 9 test functions covering authorization, idempotency, source isolation, continuation, and retryable terminal outcomes

Two minor non-blocking observations were noted in the review body: (1) no direct test for the "queued" disposition, and (2) token signing key reuse with service_key. Neither blocks merge.

Verdict: ✅ Worth merging — 🟢 LOW risk. Review submitted as APPROVED (Review ID: 5204429946).

This comment was posted by an AI agent (OpenHands).

[all-hands-bot]

👋 This PR needs a couple of things fixed before OpenHands can review it:

• the PR description's HUMAN: section needs at least 20 characters describing what you tested, not just the template placeholder

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.

[neubig]

@all-hands-bot The current head adds the requested queued-disposition coverage. Please review it and submit an APPROVE review if there are no blocking findings.

[all-hands-bot]

🤖 OpenHands is reviewing this PR.

Head commit: 577c99fe107a2427adcb86720a50902adb07ed9e
View the conversation: https://oss-agent-canvas.ngrok.dev/conversations/d2566fa3-2062-41f0-b78d-5e5b3e426f0f

This comment was posted by an AI agent (OpenHands).

[all-hands-bot]

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

Summary

This PR adds a scoped POST /v1/runs/{run_id}/subject-turns endpoint that lets a running automation script submit work for an external subject (issue, PR, ticket) without receiving conversation credentials or managing runtime lifecycle. The service owns conversation identity, profile selection, and runtime attachment. The implementation reuses existing subject locking, coalescing, deterministic conversation IDs, and the agent-turn run machinery from #467.

The current head (577c99f) adds a test covering the "queued" disposition that was flagged as a gap in the prior review on cc76156.

What changed since the last review

The new commit 577c99f ("test: cover queued subject turns") adds a single test, test_new_turn_is_queued_on_the_subject_run_before_dispatch, to tests/test_subject_turns.py. No production code was modified.

Analysis of the new test

The test exercises the queued-disposition path end to end:

1. First turn — submit_subject_turn with idempotency_key="issue-7-ready-v1" finds no existing subject run, so create_agent_turn_run creates a new PENDING run with started_at=None and event_payload=None. Disposition is "created".

2. Second turn — submit_subject_turn with a different idempotency key ("issue-7-ready-v2") and different turn text. The duplicate check passes (different key). _continue_conversation_locked finds the existing subject run (subject_released_at=None, started_at=None), so it takes the _coalesce_turn branch, parking "Acceptance criteria changed" into event_payload["_automation_follow_up_turns"] and returning coalesced=True. submit_subject_turn then sets disposition = "queued".

3. Assertions — disposition == "queued", run_id == first.run_id (same subject run, not a new one), and event_payload == {"_automation_follow_up_turns": ["Acceptance criteria changed"]}. I verified that create_agent_turn_run does not set started_at or event_payload, so the coalesce path is taken and the event_payload assertion is exact.

This directly closes the gap noted in the prior review's minor observation #1 ("No test for the queued disposition"). The queued path is now covered with a real DB-backed run, not an indirect exercise.

Full diff review (unchanged from prior review)

The production code across conversations.py, subject_router.py, utils/run_token.py, models.py, schemas.py, dispatcher.py, app.py, and migration 028 was reviewed on the prior head and is unchanged on this head. The findings from that review remain valid:

• Security model is sound — scoped HS256 JWT bound to automation_id + run_id, with verify_run_token enforcing signature, expiration, scope, run/automation match, and RUNNING status.
• Idempotency and retry logic is correct — transaction-scoped pg_advisory_xact_lock serializes submissions; the UNIQUE(automation_id, source, subject_key, idempotency_key) constraint prevents duplicates; the retry path releases failed/skipped runs before creating a replacement in the same deterministic conversation.
• Migration is cross-database compatible — generic SQLAlchemy types, chain 026 → 027 → 028 intact.

Lint / format

ruff check and ruff format --check pass on all changed files and on the full openhands/automation/ package.

Test execution note

The subject-turn suite requires Docker/PostgreSQL via testcontainers, which is unavailable in this environment, so I could not execute the 12 tests locally. I verified every code path statically against the models, helpers, and config, including a full trace of the new queued-disposition test.

Risk Assessment

🟢 LOW — The new commit adds test coverage only; no production code changed. The prior review's non-blocking observations are now addressed or remain theoretical.

Verdict

✅ Worth merging — The queued-disposition gap is closed, and the rest of the PR is unchanged from the previously reviewed head. No blocking findings.

---

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 (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.
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 👍 or 👎 to give feedback.

[all-hands-bot]

I reviewed the current head (577c99f) and submitted an APPROVE review.

The new commit adds test_new_turn_is_queued_on_the_subject_run_before_dispatch, which directly covers the "queued" disposition path that was flagged as a gap in the prior review on cc76156. I traced the full code path:

1. The first submit_subject_turn call creates a PENDING subject run via create_agent_turn_run (which sets started_at=None and event_payload=None), returning disposition="created".
2. The second call, with a different idempotency key, finds that existing subject run (subject_released_at=None, started_at=None), takes the _coalesce_turn branch in _continue_conversation_locked, parks the turn into event_payload["_automation_follow_up_turns"], and returns disposition="queued".
3. The assertions on disposition, run_id, and event_payload are all correct — create_agent_turn_run does not set started_at or event_payload, so the coalesce path is taken and the event_payload assertion is exact.

No production code was modified in this commit — only the test file. Ruff lint and format pass on all changed files and the full openhands/automation/ package. The test suite requires Docker/PostgreSQL via testcontainers, which is unavailable here, so I verified every code path statically rather than executing the tests locally.

No blocking findings. Approving.

Generated by OpenHands AI on behalf of the user.

[all-hands-bot]

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

Summary

This PR adds a scoped POST /v1/runs/{run_id}/subject-turns endpoint that lets a running automation script submit work for an external subject (issue, PR, ticket) without receiving conversation credentials or managing runtime lifecycle. The service owns conversation identity, profile selection, runtime attachment, and serialization. The implementation reuses existing subject locking, coalescing, deterministic conversation IDs, and the agent-turn run machinery from #467.

Analysis

Security model is sound. The run token is a short-lived (24h) HS256 JWT scoped to subject_turn:submit and bound to a specific automation_id + run_id. The router validates the bearer token, checks scope, verifies run_id and automation_id match the URL path and DB record, and requires the requester run to be RUNNING. The script inside the sandbox cannot forge tokens for other runs or automations, and cannot choose an arbitrary conversation ID.

Idempotency and retry logic is well-designed. The unique constraint on (automation_id, source, subject_key, idempotency_key) prevents duplicates at the DB level. The transaction-scoped advisory lock (pg_advisory_xact_lock) serializes concurrent submissions for the same subject, so the SELECT-then-INSERT idempotency check is race-free. The retry path correctly releases failed/skipped runs from the subject lookup (subject_released_at) before creating a replacement, and reuses the deterministic conversation ID so the new attempt continues the same conversation. The single idempotency record is repointed to the new attempt while the superseded run remains in history.

Migration is cross-database compatible. Uses generic sa.Uuid, sa.String, sa.DateTime types. The unique constraint creates an implicit index that covers the idempotency lookup query. The test_migrations_run_on_sqlite test is updated.

Test coverage is strong. 12 tests cover first-turn creation, deduplication, queued coalescing, retry of released failed/canceled/skipped runs (parameterized), retry of a skipped run that never started, waiting for a failed run that hasn't released, source isolation, token-for-another-run rejection, endpoint acceptance, and continuation without a second run. The tests exercise real DB state and assert on outcomes, not just mocked calls.

Risk Assessment

🟢 LOW — The change adds a new endpoint and table without modifying existing behavior. The dispatcher injection is additive (wrapped in try/except, silently disabled when no secret is configured). The security model is properly scoped. Race conditions are handled with advisory locks and unique constraints. No breaking changes to existing APIs.

Verdict

✅ Worth merging — The design is clean, the security model is sound, and the retry/idempotency logic is correct. No material findings.

Automated review used the wrong decision (APPROVED instead of COMMENT) and is dismissed. Findings are reposted as a comment.

[all-hands-bot]

I reviewed PR #468 ("feat: add scoped subject-turn submission") in OpenHands/automation and posted an APPROVED review to GitHub.

Key findings:

• Security model is sound: The run token is a short-lived (24h) HS256 JWT scoped to subject_turn:submit, bound to a specific automation_id + run_id. The router validates the token, checks scope, verifies run/automation IDs match, and requires the requester run to be RUNNING. The sandbox script cannot forge tokens for other runs or choose arbitrary conversation IDs.

• Idempotency and retry logic is correct: DB-level unique constraint on (automation_id, source, subject_key, idempotency_key) plus transaction-scoped advisory locks make the SELECT-then-INSERT idempotency check race-free on PostgreSQL. The retry path correctly releases failed/skipped runs before creating replacements, reuses the deterministic conversation ID, and repoints the single idempotency record to the new attempt.

• Migration is cross-database compatible: Uses generic SQLAlchemy types, no PG-specific features.

• Test coverage is strong: 12 tests covering creation, deduplication, queued coalescing, retry of released failed/canceled/skipped runs, source isolation, token rejection, endpoint acceptance, and continuation without a second run.

• No material bugs, security issues, or design flaws found.

Risk: 🟢 LOW — Additive change, no modifications to existing behavior.

Verdict: ✅ Worth merging — submitted as APPROVED per the repository's custom code review guidelines.

Review URL: #468 (review)

This comment was posted by an AI agent (OpenHands).