feat: add scoped subject-turn submission - #468

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.

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 scanner fan out conversation-scoped work for external subjects. The design is sound: the service owns conversation identity and runtime lifecycle; the scanner receives only a short-lived, single-scope capability token. Idempotency, coalescing, deterministic conversation identity, and retry of terminal outcomes are all correctly implemented.

Material Finding

Subject stuck permanently when release_context fails after a conversation turn failure (dispatcher.py, line 434)

The refactored error handling gates subject_released_at on backend.release_context succeeding. If the conversation turn fails and release_context also fails -- which is the likely scenario when the sandbox/agent server is already dead or unreachable -- the subject is never released. The run is marked FAILED, but subject_released_at stays NULL.

This means submit_subject_turn's retry check (can_retry = status in _RETRYABLE and (started_at is None or subject_released_at is not None)) returns False, and the subject is permanently deduplicated -- the exact problem this PR set out to solve ("Permanently deduplicating failed, canceled, skipped, or timed-out work can leave an external issue or PR stuck forever").

The watchdog cannot recover this either: it only processes RUNNING runs, and this run is already FAILED. No code path sets subject_released_at for a FAILED run whose release_context failed.

The previous code unconditionally set subject_released_at before attempting release_context. The new ordering is a regression for this failure mode. Consider releasing the subject unconditionally when the run is being marked FAILED (as the pre-runtime failure paths at lines 365 and 388 already do), or at minimum releasing it when release_context fails for a run that will be terminal anyway -- the runtime is effectively gone in that case.

[RISK ASSESSMENT]

• Overall PR: 🟡 MEDIUM. The idempotency, locking, and auth design are solid. The one material concern is the subject-stuck-forever path when release_context fails, which contradicts the PR's retry goal and requires operator intervention to recover.

VERDICT: Worth merging with a follow-up to address the release_context failure path -- or a quick fix to release the subject unconditionally on terminal failure, matching the pre-runtime failure paths.

KEY INSIGHT: The subject lock + idempotency record combination ensures deterministic retries, but only if subject_released_at is set on every terminal failure path -- the release_context-failed path is the gap.

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

What changed since the last review

The latest commit (533d9274, "fix: unblock failed subject retries") directly addresses the material finding from the prior review on d91fce1c: the subject was permanently stuck when both the conversation turn failed and backend.release_context also failed, because subject_released_at was gated on release_context succeeding.

Analysis of the fix

The conversation-turn failure path in dispatcher.py (lines 424-442) now wraps backend.release_context in its own try/except and calls _release_conversation_subject() unconditionally afterward - regardless of whether the runtime release succeeded or failed. This is the correct behavior: a run being marked FAILED has already lost its runtime, so the subject must be released to allow idempotent retries.

This matches the pre-runtime failure paths (lines 365-366, 387-388), which already release the subject unconditionally. All terminal failure paths for conversation-scoped runs now set subject_released_at.

The new test test_conversation_turn_failure_releases_subject_when_cleanup_fails (test_dispatcher.py, lines 307-370) verifies the exact scenario: submit_conversation_turn raises, release_context raises, and the test asserts finished.subject_released_at is not None. The existing test_conversation_turn_context_failure_releases_subject covers the pre-runtime path.

Full PR review

Security model - Sound. The run token is a short-lived (24h) HS256 JWT scoped to subject_turn:submit, bound to automation_id + run_id. The router verifies scope, run identity, automation identity, and RUNNING status before proceeding. The scanner receives neither conversation credentials nor control over conversation IDs.

Idempotency and retry - Correct. The transaction-scoped subject lock serializes the SELECT-then-INSERT idempotency check. The unique constraint on (automation_id, source, subject_key, idempotency_key) prevents duplicates at the DB level. The retry condition properly distinguishes never-started runs (releases immediately) from started-but-terminal runs (requires subject_released_at). The idempotency record is repointed to the new attempt rather than inserting a new row.

Migration - Cross-database compatible: generic sa.Uuid(), sa.String(), sa.DateTime(timezone=True). Chain 027 -> 028 intact.

Test coverage - Comprehensive: 12 tests covering first-turn creation, idempotency dedup, queued coalescing, retry of released FAILED/CANCELLED/SKIPPED runs, retry of never-started skipped runs, unreleased-runtime protection, source isolation, token authorization (403 for wrong run, 202 for correct), continuation delivery, pre-runtime context-failure subject release, and cleanup-failure subject release. All tests exercise real DB state.

Risk Assessment

[RISK ASSESSMENT]

• Overall PR: 🟢 LOW
  The endpoint is scoped, token-authenticated, and idempotent. The dispatcher change fixes a real failure mode (subject stuck when runtime cleanup fails) with matching test coverage. No existing behavior is altered - the dispatcher refactor preserves all existing release semantics while closing the gap. The signing secret gracefully degrades (no token injected if no secret configured). No new external dependencies (PyJWT already declared).

VERDICT: ✅ Worth merging. The previous material finding is resolved, and no new issues were found.

KEY INSIGHT: The unconditional subject_released_at on every terminal failure path - pre-runtime, runtime-gone, and cleanup-failed - ensures that the retry condition in submit_subject_turn can never be permanently blocked by a failed runtime release.

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 own 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.

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

Verdict: Worth merging

Taste Rating: Good taste

This PR adds a scoped POST /v1/runs/{run_id}/subject-turns endpoint that lets a running scanner fan out conversation-scoped work without receiving conversation credentials or controlling conversation IDs. The design is clean: the service owns conversation identity, profile selection, runtime attachment, and serialization, while the scanner only chooses an external subject and prompt.

Key strengths

1. Idempotency with retry semantics -- The AutomationSubjectTurn table with a unique constraint on (automation_id, source, subject_key, idempotency_key) combined with the can_retry check (retryable status + released subject) correctly distinguishes between deduplication of in-flight/successful work and retry of failed/canceled/skipped/timed-out work. The idempotency record is reused across retries (pointed at the new run), preserving the audit trail.

2. Subject release across all terminal paths -- The PR systematically releases subject_released_at in every path that marks a conversation run terminal:

   • Dispatcher: context failure, turn submission failure (with cleanup failure tolerated), concurrency limit skip
   • Watchdog: verification failure, timeout, exit-code failure
   • Cancel: before runtime cleanup, so cleanup failures can't block retries
   • _continue_conversation_locked: releases terminal runs so a new turn creates a fresh tracked run

3. Scoped capability token -- The run token JWT is scoped to subject_turn:submit for a specific (automation_id, run_id) pair. The scanner gets neither conversation credentials nor control over conversation IDs. Token verification checks both the run ID in the path and the automation ID on the fetched run.

4. Transaction-scoped subject lock -- _take_subject_lock (advisory lock) is taken before the idempotency check, so one retry cannot enqueue two conversations. The lock is transaction-scoped, released on commit.

5. Cross-database migration -- Uses generic SQLAlchemy types (sa.Uuid, sa.String, sa.DateTime(timezone=True)), no PostgreSQL-specific types.

6. Test coverage -- Subject-turn tests cover authorization, profile requirements, idempotency, source isolation, queued coalescing, continuation, retryable terminal outcomes, pre-runtime dispatch failure, unreleased-runtime protection, and tracked restart after terminal state. Dispatcher tests verify retry release when both turn submission and runtime cleanup fail.

Non-blocking observation

The signing_secret function reuses service_key (the SaaS API authentication secret) or local_api_key as the JWT signing key. This is consistent with the KV token pattern (which uses its own dedicated AUTOMATION_KV_SECRET) in spirit but reuses an existing deployment secret rather than introducing a new one. The risk is low -- the JWT payload structure is distinct from API key auth and scope-checked -- but a dedicated AUTOMATION_RUN_TOKEN_SECRET would provide cleaner secret isolation. This is a design trade-off, not a defect.

Risk Assessment

[RISK ASSESSMENT]

• Overall PR: LOW
  • The endpoint is authenticated via scoped JWT, not user credentials.
  • Subject release is handled consistently across all terminal paths.
  • Race conditions are managed via advisory locks and optimistic locking.
  • Migration is cross-database compatible.
  • No breaking changes to existing APIs.

Verdict: Worth merging

Key insight: The subject-release-then-retry pattern is the core innovation -- it ensures that failed conversation work is never permanently blocked by an unreleased subject, across dispatcher crashes, watchdog timeouts, cleanup failures, and user cancellations.

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 thumbs up or thumbs down to give feedback.