From 2883a0ab7194c7b185f3112cfdcbf8739cc15b86 Mon Sep 17 00:00:00 2001 From: Yvette Carlisle Date: Tue, 16 Jun 2026 16:24:44 +0800 Subject: [PATCH] Fix review handoff drift failure handling --- apps/decodex/src/orchestrator/execution.rs | 561 +++++++++++++++++- .../src/orchestrator/tests/runtime/failure.rs | 313 ++++++++++ apps/decodex/src/orchestrator/types.rs | 8 + apps/decodex/src/state/models.rs | 4 + docs/spec/lane-control.md | 9 +- docs/spec/post-review-lifecycle.md | 11 + docs/spec/runtime.md | 11 + 7 files changed, 886 insertions(+), 31 deletions(-) diff --git a/apps/decodex/src/orchestrator/execution.rs b/apps/decodex/src/orchestrator/execution.rs index 501fbd704..7d66f66e3 100644 --- a/apps/decodex/src/orchestrator/execution.rs +++ b/apps/decodex/src/orchestrator/execution.rs @@ -12,6 +12,11 @@ use crate::tracker::privacy_classifier::PublicProjectionPrivacyClassifier; const LOOP_GUARDRAIL_CONVERGENCE_BUDGET: i64 = 3; const ARCHITECTURE_RECOVERY_BUDGET: usize = 1; const ARCHITECTURE_RECOVERY_RETRY_KIND: &str = "architecture_recovery"; +const REVIEW_HANDOFF_STATE_DRIFT_DETECTED_EVENT_TYPE: &str = + "review_handoff_state_drift_detected"; +const REVIEW_HANDOFF_STATE_DRIFT_RECOVERED_EVENT_TYPE: &str = + "review_handoff_state_drift_recovered"; +const REVIEW_HANDOFF_REBOUND_ORCHESTRATION_PHASE: &str = "request_pending"; #[derive(Debug)] pub(crate) struct AppServerZeroEvidenceStartFailure { @@ -491,6 +496,33 @@ enum LoopGuardrailRecoveryDecision { HumanRequired(LoopGuardrailStopRequested), } +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +enum ReviewHandoffFailureDriftLineage { + Exact, + Descends, + Diverged, + Unknown, +} +impl ReviewHandoffFailureDriftLineage { + fn allows_lifecycle_recovery(self) -> bool { + matches!(self, Self::Exact | Self::Descends) + } + + fn as_str(self) -> &'static str { + match self { + Self::Exact => "exact", + Self::Descends => "descends", + Self::Diverged => "diverged", + Self::Unknown => "unknown", + } + } +} + +enum ReviewHandoffStateDriftTransition { + AlreadySuccess, + MoveToSuccess(String), +} + #[derive(Clone, Copy, Eq, PartialEq)] enum TerminalFailureEventRecordStatus { Recorded, @@ -2418,6 +2450,351 @@ fn truncate_private_diagnostic_text(text: &str) -> String { truncated } +fn try_recover_review_handoff_failure_drift( + tracker: &T, + project: &ServiceConfig, + workflow: &WorkflowDocument, + state_store: &StateStore, + issue_run: &IssueRunPlan, + error: &Report, +) -> Result +where + T: IssueTracker, +{ + if !review_handoff_failure_drift_can_handle(error) { + return Ok(false); + } + + let Some(worktree_fingerprint) = + loop_guardrail_worktree_fingerprint(&issue_run.worktree.path)? + else { + return Ok(false); + }; + + if worktree_fingerprint.effective_delta_present { + return Ok(false); + } + + let Some(review_handoff) = state_store.review_handoff_marker( + project.service_id(), + &issue_run.issue.id, + &issue_run.worktree.branch_name, + )? + else { + return Ok(false); + }; + + if review_handoff.branch_name() != issue_run.worktree.branch_name + || review_handoff.pr_head_ref_name() != issue_run.worktree.branch_name + { + return Ok(false); + } + + let lineage = review_handoff_failure_drift_lineage( + &issue_run.worktree.path, + review_handoff.pr_head_oid(), + &worktree_fingerprint.head_sha, + ); + + if !lineage.allows_lifecycle_recovery() { + return Ok(false); + } + + let tracker_policy = workflow.frontmatter().tracker(); + let success_state = tracker_policy.success_state(); + let current_state = issue_run.issue.state.name.as_str(); + let Some(success_state_transition) = + review_handoff_state_drift_success_transition(workflow, issue_run)? + else { + return Ok(false); + }; + let issue_state_recovered = + matches!(success_state_transition, ReviewHandoffStateDriftTransition::MoveToSuccess(_)); + let rebounded_orchestration = rebound_review_handoff_orchestration_marker( + project, + state_store, + issue_run, + &review_handoff, + &worktree_fingerprint.head_sha, + )?; + let needs_attention_cleared = tracker::set_issue_label_presence( + tracker, + &issue_run.issue, + tracker_policy.needs_attention_label(), + false, + )?; + + if let ReviewHandoffStateDriftTransition::MoveToSuccess(state_id) = + success_state_transition + { + tracker.update_issue_state(&issue_run.issue.id, &state_id)?; + } + + state_store + .clear_loop_guardrail_checkpoints_for_issue(project.service_id(), &issue_run.issue.id)?; + state_store.update_run_status(&issue_run.run_id, "succeeded")?; + state_store + .append_private_execution_event( + project.service_id(), + &issue_run.issue.id, + &issue_run.run_id, + issue_run.attempt_number, + REVIEW_HANDOFF_STATE_DRIFT_RECOVERED_EVENT_TYPE, + json!({ + "schema": "decodex.review_handoff_state_drift_recovered/1", + "reason": "current_review_handoff_marker", + "source_error_class": review_handoff_failure_drift_source_error_class(error), + "branch_name": issue_run.worktree.branch_name, + "pr_url": review_handoff.pr_url(), + "marker_head_sha": review_handoff.pr_head_oid(), + "local_head_sha": worktree_fingerprint.head_sha, + "lineage": lineage.as_str(), + "previous_issue_state": current_state, + "target_issue_state": success_state, + "issue_state_recovered": issue_state_recovered, + "needs_attention_cleared": needs_attention_cleared, + "orchestration_rebound": rebounded_orchestration, + }), + ) + .map(|_| ())?; + + tracing::warn!( + project_id = project.service_id(), + issue_id = issue_run.issue.id, + issue = issue_run.issue.identifier, + run_id = issue_run.run_id, + attempt = issue_run.attempt_number, + branch = issue_run.worktree.branch_name, + pr_url = review_handoff.pr_url(), + lineage = lineage.as_str(), + "Recovered review handoff state drift before retry/no-diff failure writeback." + ); + + Ok(true) +} + +fn review_handoff_state_drift_success_transition( + workflow: &WorkflowDocument, + issue_run: &IssueRunPlan, +) -> Result> { + let tracker_policy = workflow.frontmatter().tracker(); + let success_state = tracker_policy.success_state(); + let current_state = issue_run.issue.state.name.as_str(); + + if current_state == success_state { + return Ok(Some(ReviewHandoffStateDriftTransition::AlreadySuccess)); + } + if current_state != tracker_policy.in_progress_state() + && current_state != tracker_policy.failure_state() + { + return Ok(None); + } + + let state_id = issue_run.issue.state_id_for_name(success_state).ok_or_else(|| { + eyre::eyre!( + "State `{success_state}` was not found for issue `{}` during review handoff state drift recovery.", + issue_run.issue.identifier + ) + })?; + + Ok(Some(ReviewHandoffStateDriftTransition::MoveToSuccess( + state_id.to_owned(), + ))) +} + +fn rebound_review_handoff_orchestration_marker( + project: &ServiceConfig, + state_store: &StateStore, + issue_run: &IssueRunPlan, + review_handoff: &ReviewHandoffMarker, + local_head_sha: &str, +) -> Result { + let existing_orchestration = + state_store.review_orchestration_marker(project.service_id(), &issue_run.issue.id, review_handoff)?; + let rebounded_orchestration = existing_orchestration.as_ref().is_none_or(|marker| { + marker.branch_name() != review_handoff.branch_name() + || marker.pr_url() != review_handoff.pr_url() + || marker.head_sha() != local_head_sha + || marker.phase() != REVIEW_HANDOFF_REBOUND_ORCHESTRATION_PHASE + }); + let orchestration_marker = ReviewOrchestrationMarker::new( + review_handoff.run_id().to_owned(), + review_handoff.attempt_number(), + review_handoff.branch_name().to_owned(), + review_handoff.pr_url().to_owned(), + local_head_sha.to_owned(), + REVIEW_HANDOFF_REBOUND_ORCHESTRATION_PHASE, + None, + None, + None, + 0, + existing_orchestration + .as_ref() + .map_or(0, ReviewOrchestrationMarker::external_round_count), + None, + ); + + state_store.upsert_review_orchestration_marker( + project.service_id(), + &issue_run.issue.id, + &orchestration_marker, + )?; + + Ok(rebounded_orchestration) +} + +fn review_handoff_failure_drift_can_handle(error: &Report) -> bool { + !run_failure_requires_terminal_attention(error) + && error.downcast_ref::().is_none() + && error.downcast_ref::().is_none() + && error.downcast_ref::().is_none() + && error.downcast_ref::().is_none() + && error.downcast_ref::().is_none() + && error.downcast_ref::().is_none() +} + +fn review_handoff_failure_drift_source_error_class(error: &Report) -> &'static str { + retained_progress_source_error_class(error).unwrap_or("retryable_execution_failure") +} + +fn review_handoff_failure_drift_lineage( + worktree_path: &Path, + recorded_head_oid: &str, + local_head_oid: &str, +) -> ReviewHandoffFailureDriftLineage { + if recorded_head_oid == local_head_oid { + return ReviewHandoffFailureDriftLineage::Exact; + } + + let Ok(output) = Command::new("git") + .arg("-C") + .arg(worktree_path) + .args(["merge-base", "--is-ancestor", recorded_head_oid, local_head_oid]) + .output() + else { + return ReviewHandoffFailureDriftLineage::Unknown; + }; + + match output.status.code() { + Some(0) => ReviewHandoffFailureDriftLineage::Descends, + Some(1) => ReviewHandoffFailureDriftLineage::Diverged, + _ => ReviewHandoffFailureDriftLineage::Unknown, + } +} + +fn review_handoff_state_drift_attention_error( + project: &ServiceConfig, + workflow: &WorkflowDocument, + state_store: &StateStore, + issue_run: &IssueRunPlan, + error: &Report, +) -> Result> { + if !review_handoff_failure_drift_can_handle(error) { + return Ok(None); + } + + let Some(worktree_fingerprint) = + loop_guardrail_worktree_fingerprint(&issue_run.worktree.path)? + else { + return Ok(None); + }; + + if worktree_fingerprint.effective_delta_present { + return Ok(None); + } + + let checkpoint = state_store.review_policy_checkpoint( + project.service_id(), + &issue_run.issue.id, + &issue_run.run_id, + issue_run.attempt_number, + "handoff", + )?; + let drift_reason = match state_store.review_handoff_marker( + project.service_id(), + &issue_run.issue.id, + &issue_run.worktree.branch_name, + )? { + Some(review_handoff) => review_handoff_marker_drift_reason( + workflow, + issue_run, + &worktree_fingerprint, + &review_handoff, + )?, + None => { + let Some(checkpoint) = checkpoint.as_ref() else { + return Ok(None); + }; + + if checkpoint.status() != "clean" || checkpoint.head_sha() != worktree_fingerprint.head_sha + { + return Ok(None); + } + + Some(String::from("missing_review_handoff_marker")) + }, + }; + let Some(drift_reason) = drift_reason else { + return Ok(None); + }; + + state_store + .append_private_execution_event( + project.service_id(), + &issue_run.issue.id, + &issue_run.run_id, + issue_run.attempt_number, + REVIEW_HANDOFF_STATE_DRIFT_DETECTED_EVENT_TYPE, + json!({ + "schema": "decodex.review_handoff_state_drift_detected/1", + "reason": drift_reason, + "source_error_class": review_handoff_failure_drift_source_error_class(error), + "branch_name": issue_run.worktree.branch_name, + "checkpoint_status": checkpoint.as_ref().map(|checkpoint| checkpoint.status()), + "checkpoint_head_sha": checkpoint.as_ref().map(|checkpoint| checkpoint.head_sha()), + "local_head_sha": worktree_fingerprint.head_sha, + "next_action": "restore or rebind the retained review handoff marker before retrying execution", + }), + ) + .map(|_| ())?; + + Ok(Some(ManualAttentionRequested { + issue_identifier: issue_run.issue.identifier.clone(), + label: workflow.frontmatter().tracker().needs_attention_label().to_owned(), + run_id: issue_run.run_id.clone(), + error_class: Some(LoopGuardrailReason::ReviewHandoffStateDrift.error_class().to_owned()), + })) +} + +fn review_handoff_marker_drift_reason( + workflow: &WorkflowDocument, + issue_run: &IssueRunPlan, + worktree_fingerprint: &LoopGuardrailWorktreeFingerprint, + review_handoff: &ReviewHandoffMarker, +) -> Result> { + if review_handoff.branch_name() != issue_run.worktree.branch_name { + return Ok(Some(String::from("review_handoff_marker_branch_mismatch"))); + } + if review_handoff.pr_head_ref_name() != issue_run.worktree.branch_name { + return Ok(Some(String::from("review_handoff_marker_pr_head_ref_mismatch"))); + } + + let lineage = review_handoff_failure_drift_lineage( + &issue_run.worktree.path, + review_handoff.pr_head_oid(), + &worktree_fingerprint.head_sha, + ); + + if !lineage.allows_lifecycle_recovery() { + return Ok(Some(format!("review_handoff_marker_{}", lineage.as_str()))); + } + if review_handoff_state_drift_success_transition(workflow, issue_run)?.is_some() { + return Ok(None); + } + + Ok(Some(String::from("review_handoff_marker_issue_state_unsupported"))) +} + fn retryable_failure_loop_guardrail_stop( project: &ServiceConfig, state_store: &StateStore, @@ -3251,11 +3628,26 @@ where worktree_path: &worktree_path, retry_budget_attempts, }; - let loop_guardrail_stop = if requires_terminal_attention { - None - } else { - retryable_failure_loop_guardrail_stop(project, state_store, issue_run, error)? - }; + + if handle_review_handoff_failure_drift( + tracker, + project, + workflow, + state_store, + issue_run, + error, + &worktree_path, + )? { + return Ok(()); + } + + let loop_guardrail_stop = retryable_failure_loop_guardrail_stop_unless_terminal_attention( + project, + state_store, + issue_run, + error, + requires_terminal_attention, + )?; let retained_partial_progress = retained_partial_progress_error( error, issue_run, @@ -3298,46 +3690,106 @@ where } let terminal_error = retained_partial_progress.as_ref().unwrap_or(error); - let privacy_classifier = configured_public_projection_privacy_classifier(project)?; - let outcome = apply_terminal_failure_writeback( + + apply_terminal_attention_failure_writeback( + &failure_context, + manual_attention_requested, + terminal_error, + ) +} + +fn handle_review_handoff_failure_drift( + tracker: &T, + project: &ServiceConfig, + workflow: &WorkflowDocument, + state_store: &StateStore, + issue_run: &IssueRunPlan, + error: &Report, + worktree_path: &str, +) -> Result +where + T: IssueTracker, +{ + if try_recover_review_handoff_failure_drift( tracker, + project, + workflow, + state_store, + issue_run, + error, + )? { + return Ok(true); + } + + let Some(attention_error) = + review_handoff_state_drift_attention_error(project, workflow, state_store, issue_run, error)? + else { + return Ok(false); + }; + + apply_review_handoff_state_drift_attention_writeback( + tracker, + project, + workflow, + state_store, + issue_run, + worktree_path, + attention_error, + )?; + + Ok(true) +} + +fn apply_terminal_attention_failure_writeback( + context: &FailureHandlingContext<'_, T>, + manual_attention_requested: bool, + terminal_error: &Report, +) -> Result<()> +where + T: IssueTracker, +{ + let privacy_classifier = configured_public_projection_privacy_classifier(context.project)?; + let outcome = apply_terminal_failure_writeback( + context.tracker, TerminalFailureWritebackRuntime { - service_id: project.service_id(), - state_store: Some(state_store), + service_id: context.project.service_id(), + state_store: Some(context.state_store), privacy_classifier: &privacy_classifier, }, - workflow, - issue_run, - &worktree_path, + context.workflow, + context.issue_run, + context.worktree_path, manual_attention_requested, terminal_error, )?; if outcome.retry_guarded_by_state { write_terminal_guard_marker( - &issue_run.worktree.path, - &issue_run.run_id, - issue_run.attempt_number, + &context.issue_run.worktree.path, + &context.issue_run.run_id, + context.issue_run.attempt_number, )?; - state_store.update_run_status(&issue_run.run_id, TERMINAL_GUARDED_RUN_STATUS)?; + context + .state_store + .update_run_status(&context.issue_run.run_id, TERMINAL_GUARDED_RUN_STATUS)?; } write_retry_budget_marker( - &issue_run.worktree.path, - &issue_run.run_id, - issue_run.attempt_number, - retry_budget_attempts, + &context.issue_run.worktree.path, + &context.issue_run.run_id, + context.issue_run.attempt_number, + context.retry_budget_attempts, )?; tracing::warn!( - project_id = project.service_id(), - issue_id = issue_run.issue.id, - issue = issue_run.issue.identifier, - run_id = issue_run.run_id, - attempt = issue_run.attempt_number, - branch = issue_run.worktree.branch_name, - worktree_path = %worktree_path, + project_id = context.project.service_id(), + issue_id = context.issue_run.issue.id, + issue = context.issue_run.issue.identifier, + run_id = context.issue_run.run_id, + attempt = context.issue_run.attempt_number, + branch = context.issue_run.worktree.branch_name, + worktree_path = %context.worktree_path, error_class = outcome.error_class, "Run failed and now requires operator attention." ); @@ -3345,6 +3797,61 @@ where Ok(()) } +fn retryable_failure_loop_guardrail_stop_unless_terminal_attention( + project: &ServiceConfig, + state_store: &StateStore, + issue_run: &IssueRunPlan, + error: &Report, + requires_terminal_attention: bool, +) -> Result> { + if requires_terminal_attention { + Ok(None) + } else { + retryable_failure_loop_guardrail_stop(project, state_store, issue_run, error) + } +} + +fn apply_review_handoff_state_drift_attention_writeback( + tracker: &T, + project: &ServiceConfig, + workflow: &WorkflowDocument, + state_store: &StateStore, + issue_run: &IssueRunPlan, + worktree_path: &str, + attention_error: ManualAttentionRequested, +) -> Result<()> +where + T: IssueTracker, +{ + let terminal_error = Report::new(attention_error); + let privacy_classifier = configured_public_projection_privacy_classifier(project)?; + let outcome = apply_terminal_failure_writeback( + tracker, + TerminalFailureWritebackRuntime { + service_id: project.service_id(), + state_store: Some(state_store), + privacy_classifier: &privacy_classifier, + }, + workflow, + issue_run, + worktree_path, + true, + &terminal_error, + )?; + + if outcome.retry_guarded_by_state { + write_terminal_guard_marker( + &issue_run.worktree.path, + &issue_run.run_id, + issue_run.attempt_number, + )?; + + state_store.update_run_status(&issue_run.run_id, TERMINAL_GUARDED_RUN_STATUS)?; + } + + Ok(()) +} + fn apply_retryable_failure_writeback( context: &FailureHandlingContext<'_, T>, error: &Report, diff --git a/apps/decodex/src/orchestrator/tests/runtime/failure.rs b/apps/decodex/src/orchestrator/tests/runtime/failure.rs index f1bdc7c63..4cfb5cf0f 100644 --- a/apps/decodex/src/orchestrator/tests/runtime/failure.rs +++ b/apps/decodex/src/orchestrator/tests/runtime/failure.rs @@ -1157,6 +1157,319 @@ fn loop_guardrail_stops_no_effective_diff_for_retryable_errors_without_delta() { assert_eq!(checkpoint.consecutive_count(), 3); } +#[test] +fn handle_failure_recovers_review_handoff_state_drift_before_no_effective_diff_terminalization() { + let (_temp_dir, config, workflow) = temp_project_layout(); + let state_store = StateStore::open_in_memory().expect("state store should open"); + let active_label = tracker::automation_active_label(config.service_id()); + let issue = sample_issue("In Progress", &[active_label.as_str()]); + let tracker = FakeTracker::new(vec![issue.clone()]); + let pr_url = "https://github.com/hack-ink/decodex/pull/957"; + + for attempt_number in 1..=2 { + let issue_run = loop_guardrail_issue_run(&config, &issue, attempt_number); + let error = Report::msg("child exited without useful change"); + + assert!( + orchestrator::retryable_failure_loop_guardrail_stop( + &config, + &state_store, + &issue_run, + &error, + ) + .expect("guardrail observation should persist") + .is_none() + ); + } + + let issue_run = loop_guardrail_issue_run(&config, &issue, 3); + let head_oid = git_output(config.repo_root(), &["rev-parse", "HEAD"]); + let handoff = ReviewHandoffMarker::new( + &issue_run.run_id, + issue_run.attempt_number, + &issue_run.worktree.branch_name, + pr_url, + "main", + &issue_run.worktree.branch_name, + &head_oid, + ); + + state_store + .record_run_attempt(&issue_run.run_id, &issue.id, issue_run.attempt_number, "failed") + .expect("run attempt should record"); + state_store + .upsert_review_handoff_marker(config.service_id(), &issue.id, &handoff) + .expect("review handoff marker should record"); + + orchestrator::handle_failure( + &tracker, + &config, + &workflow, + &state_store, + &issue_run, + &Report::msg("child exited without useful change"), + ) + .expect("review handoff drift should recover before no-diff terminalization"); + + assert_eq!( + tracker.state_updates.borrow().last(), + Some(&(issue.id.clone(), String::from("state-review"))) + ); + assert!(tracker.label_additions.borrow().is_empty()); + assert!(tracker.comments.borrow().iter().all(|comment| { + !comment.contains("decodex run failed and needs attention") + && !comment.contains("no_effective_diff") + })); + assert!( + state_store + .loop_guardrail_checkpoint(config.service_id(), &issue.id, "no_effective_diff") + .expect("no-diff checkpoint should read") + .is_none(), + "handoff recovery owns the lifecycle and clears stale retry/no-diff guardrails" + ); + + let run_attempt = state_store + .run_attempt(&issue_run.run_id) + .expect("run attempt should read") + .expect("run attempt should remain present"); + + assert_eq!(run_attempt.status(), "succeeded"); + + let orchestration = state_store + .review_orchestration_marker(config.service_id(), &issue.id, &handoff) + .expect("review orchestration should read") + .expect("review orchestration should be rebound"); + + assert_eq!(orchestration.phase(), "request_pending"); + assert_eq!(orchestration.head_sha(), head_oid); + + let events = state_store + .list_private_execution_events( + config.service_id(), + &issue.id, + &issue_run.run_id, + issue_run.attempt_number, + ) + .expect("private events should list"); + + assert!(events.iter().any(|event| { + event.event_type() == "review_handoff_state_drift_recovered" + && event.payload()["reason"] == "current_review_handoff_marker" + && event.payload()["target_issue_state"] == "In Review" + })); +} + +#[test] +fn handle_failure_requires_rebind_when_clean_handoff_checkpoint_has_no_marker() { + let (_temp_dir, config, workflow) = temp_project_layout(); + let state_store = StateStore::open_in_memory().expect("state store should open"); + let issue = sample_issue("In Progress", &[]); + let tracker = FakeTracker::new(vec![issue.clone()]); + let issue_run = loop_guardrail_issue_run(&config, &issue, 3); + let head_oid = git_output(config.repo_root(), &["rev-parse", "HEAD"]); + + state_store + .record_run_attempt(&issue_run.run_id, &issue.id, issue_run.attempt_number, "failed") + .expect("run attempt should record"); + state_store + .upsert_review_policy_checkpoint(ReviewPolicyCheckpointInput { + project_id: config.service_id(), + issue_id: &issue.id, + run_id: &issue_run.run_id, + attempt_number: issue_run.attempt_number, + phase: "handoff", + status: "clean", + head_sha: &head_oid, + nonclean_rounds: 0, + details_json: "{}", + }) + .expect("clean handoff checkpoint should persist"); + + orchestrator::handle_failure( + &tracker, + &config, + &workflow, + &state_store, + &issue_run, + &Report::msg("child exited without useful change"), + ) + .expect("missing handoff marker should require explicit rebind attention"); + + assert_eq!( + tracker.state_updates.borrow().last(), + Some(&(issue.id.clone(), String::from("state-todo"))) + ); + assert_eq!( + tracker.label_additions.borrow().last(), + Some(&(issue.id.clone(), vec![String::from("label-needs-attention")])) + ); + assert!(tracker.comments.borrow().iter().any(|comment| { + comment.contains("review_handoff_state_drift") + && comment.contains("restore or rebind the post-review lifecycle") + })); + assert!(tracker.comments.borrow().iter().all(|comment| { + !comment.contains("no_effective_diff") + })); + assert!( + state_store + .loop_guardrail_checkpoint(config.service_id(), &issue.id, "no_effective_diff") + .expect("no-diff checkpoint should read") + .is_none(), + "missing handoff marker must not be reclassified as no effective diff" + ); + + let events = state_store + .list_private_execution_events( + config.service_id(), + &issue.id, + &issue_run.run_id, + issue_run.attempt_number, + ) + .expect("private events should list"); + + assert!(events.iter().any(|event| { + event.event_type() == "review_handoff_state_drift_detected" + && event.payload()["reason"] == "missing_review_handoff_marker" + && event.payload()["checkpoint_status"] == "clean" + })); +} + +#[test] +fn handle_failure_requires_rebind_when_handoff_marker_head_ref_mismatches_without_checkpoint() { + let (_temp_dir, config, workflow) = temp_project_layout(); + let state_store = StateStore::open_in_memory().expect("state store should open"); + let issue = sample_issue("In Progress", &[]); + let tracker = FakeTracker::new(vec![issue.clone()]); + let issue_run = loop_guardrail_issue_run(&config, &issue, 3); + let head_oid = git_output(config.repo_root(), &["rev-parse", "HEAD"]); + let handoff = ReviewHandoffMarker::new( + &issue_run.run_id, + issue_run.attempt_number, + &issue_run.worktree.branch_name, + "https://github.com/hack-ink/decodex/pull/957", + "main", + "x/pubfi-pub-101-stale", + &head_oid, + ); + + state_store + .record_run_attempt(&issue_run.run_id, &issue.id, issue_run.attempt_number, "failed") + .expect("run attempt should record"); + state_store + .upsert_review_handoff_marker(config.service_id(), &issue.id, &handoff) + .expect("review handoff marker should record"); + + orchestrator::handle_failure( + &tracker, + &config, + &workflow, + &state_store, + &issue_run, + &Report::msg("child exited without useful change"), + ) + .expect("mismatched handoff marker should require explicit rebind attention"); + + assert_eq!( + tracker.state_updates.borrow().last(), + Some(&(issue.id.clone(), String::from("state-todo"))) + ); + assert_eq!( + tracker.label_additions.borrow().last(), + Some(&(issue.id.clone(), vec![String::from("label-needs-attention")])) + ); + assert!(tracker.comments.borrow().iter().any(|comment| { + comment.contains("review_handoff_state_drift") + && comment.contains("restore or rebind the post-review lifecycle") + })); + assert!(tracker.comments.borrow().iter().all(|comment| { + !comment.contains("no_effective_diff") + })); + assert!( + state_store + .loop_guardrail_checkpoint(config.service_id(), &issue.id, "no_effective_diff") + .expect("no-diff checkpoint should read") + .is_none(), + "untrusted handoff marker must not fall through to no effective diff" + ); + + let events = state_store + .list_private_execution_events( + config.service_id(), + &issue.id, + &issue_run.run_id, + issue_run.attempt_number, + ) + .expect("private events should list"); + + assert!(events.iter().any(|event| { + event.event_type() == "review_handoff_state_drift_detected" + && event.payload()["reason"] == "review_handoff_marker_pr_head_ref_mismatch" + && event.payload()["checkpoint_status"].is_null() + })); +} + +#[test] +fn handle_failure_requires_rebind_when_handoff_marker_issue_state_is_unsupported() { + let (_temp_dir, config, workflow) = temp_project_layout(); + let state_store = StateStore::open_in_memory().expect("state store should open"); + let issue = sample_issue("Backlog", &[]); + let tracker = FakeTracker::new(vec![issue.clone()]); + let issue_run = loop_guardrail_issue_run(&config, &issue, 3); + let head_oid = git_output(config.repo_root(), &["rev-parse", "HEAD"]); + let handoff = ReviewHandoffMarker::new( + &issue_run.run_id, + issue_run.attempt_number, + &issue_run.worktree.branch_name, + "https://github.com/hack-ink/decodex/pull/957", + "main", + &issue_run.worktree.branch_name, + &head_oid, + ); + + state_store + .record_run_attempt(&issue_run.run_id, &issue.id, issue_run.attempt_number, "failed") + .expect("run attempt should record"); + state_store + .upsert_review_handoff_marker(config.service_id(), &issue.id, &handoff) + .expect("review handoff marker should record"); + + orchestrator::handle_failure( + &tracker, + &config, + &workflow, + &state_store, + &issue_run, + &Report::msg("child exited without useful change"), + ) + .expect("unsupported issue state should require explicit handoff recovery"); + + assert_eq!( + tracker.state_updates.borrow().last(), + Some(&(issue.id.clone(), String::from("state-todo"))) + ); + assert!(tracker.comments.borrow().iter().any(|comment| { + comment.contains("review_handoff_state_drift") + && comment.contains("restore or rebind the post-review lifecycle") + })); + assert!(tracker.comments.borrow().iter().all(|comment| { + !comment.contains("no_effective_diff") + })); + + let events = state_store + .list_private_execution_events( + config.service_id(), + &issue.id, + &issue_run.run_id, + issue_run.attempt_number, + ) + .expect("private events should list"); + + assert!(events.iter().any(|event| { + event.event_type() == "review_handoff_state_drift_detected" + && event.payload()["reason"] == "review_handoff_marker_issue_state_unsupported" + })); +} + #[test] fn loop_guardrail_does_not_classify_dirty_retained_diff_as_no_effective_diff() { let (_temp_dir, config, _workflow) = temp_project_layout(); diff --git a/apps/decodex/src/orchestrator/types.rs b/apps/decodex/src/orchestrator/types.rs index e42832230..ca713b573 100644 --- a/apps/decodex/src/orchestrator/types.rs +++ b/apps/decodex/src/orchestrator/types.rs @@ -244,6 +244,7 @@ enum LoopGuardrailReason { NoEffectiveDiff, RemainingDeltaUnchanged, ReviewChurn, + ReviewHandoffStateDrift, DependencyProgramStale, UncoveredDirection, AmbiguousRetainedProgress, @@ -255,6 +256,7 @@ impl LoopGuardrailReason { Self::NoEffectiveDiff => "no_effective_diff", Self::RemainingDeltaUnchanged => "remaining_delta_unchanged", Self::ReviewChurn => "review_churn", + Self::ReviewHandoffStateDrift => "review_handoff_state_drift", Self::DependencyProgramStale => "dependency_program_stale", Self::UncoveredDirection => "uncovered_direction", Self::AmbiguousRetainedProgress => "ambiguous_retained_progress", @@ -267,6 +269,9 @@ impl LoopGuardrailReason { "no_effective_diff" => Some(Self::NoEffectiveDiff), "remaining_delta_unchanged" => Some(Self::RemainingDeltaUnchanged), "review_churn" | "review_policy_exhausted" => Some(Self::ReviewChurn), + "review_handoff_state_drift" | "review_handoff_rebind_required" => { + Some(Self::ReviewHandoffStateDrift) + }, "dependency_program_stale" | "dependency_blocked" => { Some(Self::DependencyProgramStale) }, @@ -294,6 +299,9 @@ impl LoopGuardrailReason { Self::ReviewChurn => format!( "inspect the repeated review findings and current head; decide the next repair or architecture review manually before requeueing, {recovery_gate}" ), + Self::ReviewHandoffStateDrift => format!( + "inspect the retained review handoff marker, clean review checkpoint, PR head, and issue state; restore or rebind the post-review lifecycle before clearing attention, {recovery_gate}" + ), Self::DependencyProgramStale => format!( "inspect the dependency blocker and Execution Program readiness evidence; refresh dependencies or split/research the program before requeueing, {recovery_gate}" ), diff --git a/apps/decodex/src/state/models.rs b/apps/decodex/src/state/models.rs index ef7b0ecfd..1ae5a5d85 100644 --- a/apps/decodex/src/state/models.rs +++ b/apps/decodex/src/state/models.rs @@ -1393,6 +1393,10 @@ impl ReviewHandoffMarker { self.target_base_ref_name.as_deref() } + pub(crate) fn pr_head_ref_name(&self) -> &str { + &self.pr_head_ref_name + } + pub(crate) fn pr_head_oid(&self) -> &str { &self.pr_head_oid } diff --git a/docs/spec/lane-control.md b/docs/spec/lane-control.md index 2d328d046..db2ec35cd 100644 --- a/docs/spec/lane-control.md +++ b/docs/spec/lane-control.md @@ -231,10 +231,11 @@ recovery budget remains. Boundary, dependency, uncovered-direction, ownership, a exhausted-recovery outcomes become manual-attention stops. When status or failure writeback reports `validation_repeat`, `no_effective_diff`, -`remaining_delta_unchanged`, `review_churn`, `dependency_program_stale`, -`uncovered_direction`, or `ambiguous_retained_progress`, operators must inspect the -retained worktree, private evidence, blocker state, recovery packet, boundary check, -or review findings named by that reason before clearing `decodex:needs-attention`. +`remaining_delta_unchanged`, `review_churn`, `review_handoff_state_drift`, +`dependency_program_stale`, `uncovered_direction`, or `ambiguous_retained_progress`, +operators must inspect the retained worktree, private evidence, blocker state, +recovery packet, boundary check, review findings, or retained handoff marker named by +that reason before clearing `decodex:needs-attention`. Do not use steer, retry, label cleanup, or hard interrupt to bypass the guardrail without changing the underlying repair strategy, dependency readiness, research contract, authority decision, or retained-progress ownership decision. diff --git a/docs/spec/post-review-lifecycle.md b/docs/spec/post-review-lifecycle.md index 87958a64d..01df81a21 100644 --- a/docs/spec/post-review-lifecycle.md +++ b/docs/spec/post-review-lifecycle.md @@ -75,6 +75,17 @@ If these signals disagree and the disagreement cannot be resolved without guessi not infer a PR lineage from branch names, current heads, PR titles, or Linear comments, and `decodex run` must not repair this state automatically. +Failure writeback must also respect this post-review boundary. If an execution failure +arrives after a retained review handoff marker already binds the current issue, +branch, and local HEAD lineage, Decodex may self-heal state drift by rebuilding the +review orchestration marker, clearing loop guardrail checkpoints for the issue, and +moving the tracker issue back to `tracker.success_state` when the issue had drifted to +`tracker.in_progress_state` or `tracker.failure_state`. This is not implementation +repair and must happen before retry/no-diff loop guardrails run. If the retained marker +is absent, unverified, or diverged, Decodex must stop with +`review_handoff_state_drift` or the existing `missing_review_handoff_record` posture +and require explicit recovery evidence rather than guessing a PR lineage. + When the retained review handoff marker exists but a direct PR-state read or local worktree branch/head read fails, operator status must degrade the readback instead of replacing the bound lane with a null-PR blocked state. The status row must keep the diff --git a/docs/spec/runtime.md b/docs/spec/runtime.md index dae2b12d6..2885d68a5 100644 --- a/docs/spec/runtime.md +++ b/docs/spec/runtime.md @@ -448,6 +448,17 @@ effective worktree status and tracked diff against `HEAD`, so retained partial p remains inspectable instead of being deleted, hidden, or mislabeled as an empty-diff retry. +Review handoff is a lifecycle boundary for loop guardrails. If a retryable failure +occurs after Decodex has a retained review handoff marker for the current issue, +branch, and local HEAD lineage, failure handling must recover the post-review +orchestration marker and return the lane to the review lifecycle before recording a +new `no_effective_diff` checkpoint or terminalizing the run. If the worktree has a +clean handoff checkpoint for the current head but the retained handoff marker is +missing or has unverified/diverged lineage, Decodex must classify the failure as +`review_handoff_state_drift` and require explicit handoff recovery evidence. It must +not send this condition through implementation architecture recovery and must not +mislabel it as ordinary no-effective-diff repair churn. + When `[codex].review` is `"standard"` or `"strict"`, handoff and retained review-repair runs also consume the latest structured `issue_review_checkpoint` state for the current phase and current lane head from the owned lane: