Skip to content

Restart reviewer terminal when a newer PR head supersedes an in-flight review #406

Description

@neversettle17-101

Problem

AO currently supersedes stale running review_run rows when a worker PR gets a newer head SHA, but the shared reviewer terminal is only notified with the new task if it is still alive.

That protects the daemon from accepting stale results, because a later ao review submit --run <old-run> is rejected once the old run is marked failed. However, the reviewer process itself may still be actively reasoning about the old commit. Injecting a new prompt into the same terminal can mix old and new review context.

Example:

  1. Trigger review for PR chore: scaffold backend/ and frontend/ skeletons for rewrite #1 at sha-old.
  2. AO creates run-old and starts reviewer terminal review-<worker>.
  3. Worker pushes sha-new while the reviewer is still working.
  4. User triggers reviews again.
  5. AO marks run-old failed and creates run-new for sha-new.
  6. AO currently reuses the live reviewer terminal and sends a new prompt.

The storage state is correct, but the live reviewer execution is not cleanly cancelled.

Desired behavior

When a new trigger supersedes one or more running review runs for an older SHA, AO should stop the current reviewer execution and start a fresh reviewer terminal for the current review queue.

High-level behavior:

  • Same current-head running run: reuse/skip as today.
  • Older running run for the same PR: mark stale run failed, destroy reviewer runtime, spawn fresh reviewer for the current queue.
  • If old reviewer later submits the old run, submit remains rejected because the old run is no longer running.

Probable fix

  • Extend the review launcher interface with Destroy(ctx, handleID).
  • Extend the private reviewer runtime interface to include existing ports.Runtime.Destroy.
  • In Trigger(sessionID), track whether SupersedeStaleRunningReviewRuns(...) superseded any old running runs.
  • If stale running work was superseded and the shared reviewer handle is alive:
    • destroy the reviewer runtime handle,
    • spawn a fresh reviewer instead of notifying the old terminal.
  • Keep the session-level reviewer handle stable (review-<workerID>); zellij can destroy/recreate the same runtime id.
  • If destroy/spawn fails, mark newly created runs failed using the existing trigger failure path.

Tests

  • Stale running old SHA causes destroy + spawn, not notify.
  • Current-head running same SHA does not destroy or spawn.
  • Multi-PR trigger with one stale running run restarts reviewer once and launches the fresh current queue.
  • Destroy failure fails newly created runs and returns an error.
  • Submit for the old stale run remains rejected because the run is failed.

Notes

This issue is separate from #395. #395 makes review runs PR-scoped and uses one shared reviewer terminal per worker session. This issue tightens the behavior when that shared reviewer is working on a commit that has already been superseded.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions