diff --git a/apps/decodex/src/orchestrator/execution.rs b/apps/decodex/src/orchestrator/execution.rs index 0eb3295af..7243927c3 100644 --- a/apps/decodex/src/orchestrator/execution.rs +++ b/apps/decodex/src/orchestrator/execution.rs @@ -397,6 +397,8 @@ struct LoopGuardrailWorktreeFingerprint { head_sha: String, tracked_status_hash: String, tracked_diff_hash: String, + effective_status_hash: String, + effective_delta_present: bool, } struct FailureHandlingContext<'a, T> @@ -1976,23 +1978,25 @@ fn retryable_failure_loop_guardrail_stop( "{}:{}:{}:{}", repo_gate_failure.error_class(), worktree_fingerprint.head_sha.as_str(), - worktree_fingerprint.tracked_status_hash.as_str(), + worktree_fingerprint.effective_status_hash.as_str(), worktree_fingerprint.tracked_diff_hash.as_str() ), Some(repo_gate_failure.error_class()), )); } - observations.push(( - LoopGuardrailReason::NoEffectiveDiff, - format!( - "{}:{}:{}", - worktree_fingerprint.head_sha.as_str(), - worktree_fingerprint.tracked_status_hash.as_str(), - worktree_fingerprint.tracked_diff_hash.as_str() - ), - retained_progress_source_error_class(error), - )); + if !worktree_fingerprint.effective_delta_present { + observations.push(( + LoopGuardrailReason::NoEffectiveDiff, + format!( + "{}:{}:{}", + worktree_fingerprint.head_sha.as_str(), + worktree_fingerprint.effective_status_hash.as_str(), + worktree_fingerprint.tracked_diff_hash.as_str() + ), + retained_progress_source_error_class(error), + )); + } for (reason, fingerprint, source_error_class) in observations { let checkpoint = state_store.observe_loop_guardrail_checkpoint( @@ -2010,6 +2014,8 @@ fn retryable_failure_loop_guardrail_stop( "head_sha": worktree_fingerprint.head_sha.as_str(), "tracked_status_hash": worktree_fingerprint.tracked_status_hash.as_str(), "tracked_diff_hash": worktree_fingerprint.tracked_diff_hash.as_str(), + "effective_status_hash": worktree_fingerprint.effective_status_hash.as_str(), + "effective_delta_present": worktree_fingerprint.effective_delta_present, "threshold": LOOP_GUARDRAIL_CONVERGENCE_BUDGET, }) .to_string(), @@ -2051,19 +2057,45 @@ fn loop_guardrail_worktree_fingerprint( else { return Ok(None); }; + let Some(raw_status) = git_guardrail_output(worktree_path, &["status", "--porcelain"])? else { + return Ok(None); + }; let Some(tracked_diff) = git_guardrail_output(worktree_path, &["diff", "--binary", "--no-ext-diff", "HEAD", "--"])? else { return Ok(None); }; + let effective_status = loop_guardrail_effective_status(&raw_status); Ok(Some(LoopGuardrailWorktreeFingerprint { head_sha, tracked_status_hash: loop_guardrail_text_hash(&tracked_status), tracked_diff_hash: loop_guardrail_text_hash(&tracked_diff), + effective_status_hash: loop_guardrail_text_hash(&effective_status), + effective_delta_present: !effective_status.trim().is_empty() + || !tracked_diff.trim().is_empty(), })) } +fn loop_guardrail_effective_status(raw_status: &str) -> String { + let lines = raw_status + .lines() + .map(str::trim_end) + .filter(|line| !line.is_empty()) + .filter(|line| !state::is_untracked_decodex_runtime_artifact_status_line(line)) + .collect::>(); + + if lines.is_empty() { + return String::new(); + } + + let mut status = lines.join("\n"); + + status.push('\n'); + + status +} + fn git_guardrail_output(worktree_path: &Path, args: &[&str]) -> Result> { let output = Command::new("git") .arg("-C") @@ -2502,6 +2534,8 @@ fn architecture_recovery_retained_worktree(worktree_path: &Path) -> Result Result, + clear_needs_attention_label: bool, } struct LegacyCloseoutValidation { @@ -176,6 +177,11 @@ struct RebindSuccessStateTransition { state_id: String, } +struct RebindLabelValidation { + active_label_present: bool, + clear_needs_attention_label: bool, +} + #[derive(Clone, Copy, Debug, Eq, PartialEq)] enum RebindMode { RestoreMissingHandoff, @@ -183,6 +189,14 @@ enum RebindMode { CompleteExistingHandoffState, } impl RebindMode { + fn allows_failure_state_drift_repair(self) -> bool { + self == Self::CompleteExistingHandoffState + } + + fn allows_partial_handoff_state_completion(self) -> bool { + matches!(self, Self::RestoreMissingHandoff | Self::CompleteExistingHandoffState) + } + fn evidence_value(self) -> &'static str { match self { Self::RestoreMissingHandoff => "absent", @@ -965,7 +979,7 @@ fn validate_rebind_request( &local_head_oid, )?; let success_state_transition = validate_rebind_issue_state(context, &issue, mode)?; - let active_label_present = validate_rebind_tracker_labels(context, &issue)?; + let label_validation = validate_rebind_tracker_labels(context, &issue, mode)?; let worktree_path_for_event = repository_relative_path(context.config.repo_root(), worktree.worktree_path()); @@ -977,9 +991,10 @@ fn validate_rebind_request( landing_state, local_head_oid, worktree_path_for_event, - active_label_present, + active_label_present: label_validation.active_label_present, mode, success_state_transition, + clear_needs_attention_label: label_validation.clear_needs_attention_label, }) } @@ -1086,13 +1101,6 @@ fn validate_rebind_issue_context( tracker_policy.opt_out_label() ); } - if issue.has_label(tracker_policy.needs_attention_label()) { - eyre::bail!( - "Issue `{}` has needs-attention label `{}`.", - issue.identifier, - tracker_policy.needs_attention_label() - ); - } let worktree = context.state_store.worktree_for_issue(&issue.id)?.ok_or_else(|| { eyre::eyre!("Issue `{}` has no retained worktree mapping.", issue.identifier) @@ -1119,7 +1127,7 @@ fn validate_rebind_issue_state_for_policy( if issue.state.name == success_state { return Ok(None); } - if matches!(mode, RebindMode::RestoreMissingHandoff | RebindMode::CompleteExistingHandoffState) + if mode.allows_partial_handoff_state_completion() && issue.state.name == tracker_policy.in_progress_state() { let state_id = issue.state_id_for_name(success_state).ok_or_else(|| { @@ -1131,17 +1139,34 @@ fn validate_rebind_issue_state_for_policy( state_id: state_id.to_owned(), })); } + if mode.allows_failure_state_drift_repair() + && issue.state.name == tracker_policy.failure_state() + { + let state_id = issue.state_id_for_name(success_state).ok_or_else(|| { + eyre::eyre!("State `{success_state}` was not found for issue `{}`.", issue.identifier) + })?; + + return Ok(Some(RebindSuccessStateTransition { + state_name: success_state.to_owned(), + state_id: state_id.to_owned(), + })); + } eyre::bail!( "Issue `{}` is in `{}`, but review handoff rebind requires `{}`{}.", issue.identifier, issue.state.name, success_state, - if matches!( - mode, - RebindMode::RestoreMissingHandoff | RebindMode::CompleteExistingHandoffState - ) { - format!(" or `{}` for a partial handoff recovery", tracker_policy.in_progress_state()) + if mode.allows_partial_handoff_state_completion() { + format!( + " or `{}`{} for a partial handoff recovery", + tracker_policy.in_progress_state(), + if mode.allows_failure_state_drift_repair() { + format!(" or `{}` for state drift recovery", tracker_policy.failure_state()) + } else { + String::new() + } + ) } else { String::new() } @@ -1207,7 +1232,9 @@ fn validate_existing_handoff_refresh( }); if existing_handoff.pr_head_oid() == local_head_oid && orchestration_is_current { - if issue.state.name == tracker_policy.in_progress_state() { + if issue.state.name == tracker_policy.in_progress_state() + || issue.state.name == tracker_policy.failure_state() + { return Ok(( existing_handoff.run_id().to_owned(), existing_handoff.attempt_number(), @@ -1358,7 +1385,11 @@ fn validate_retained_pr_worktree( Ok(local_head) } -fn validate_rebind_tracker_labels(context: &RecoveryContext, issue: &TrackerIssue) -> Result { +fn validate_rebind_tracker_labels( + context: &RecoveryContext, + issue: &TrackerIssue, + mode: RebindMode, +) -> Result { let active_label = tracker::automation_active_label(context.config.service_id()); let active_label_present = tracker::issue_has_label_with_server_confirmation(&context.tracker, issue, &active_label)?; @@ -1370,7 +1401,47 @@ fn validate_rebind_tracker_labels(context: &RecoveryContext, issue: &TrackerIssu ); } - Ok(active_label_present) + let tracker_policy = context.workflow.frontmatter().tracker(); + let needs_attention_label = tracker_policy.needs_attention_label(); + let needs_attention_present = tracker::issue_has_label_with_server_confirmation( + &context.tracker, + issue, + needs_attention_label, + )?; + + if !needs_attention_present { + return Ok(RebindLabelValidation { + active_label_present, + clear_needs_attention_label: false, + }); + } + if mode.allows_failure_state_drift_repair() + && issue.state.name == tracker_policy.failure_state() + { + if tracker::issue_team_label_id_with_server_confirmation( + &context.tracker, + issue, + needs_attention_label, + )? + .is_none() + { + eyre::bail!( + "Issue `{}` has needs-attention label `{needs_attention_label}`, but that label was not found on the team.", + issue.identifier + ); + } + + return Ok(RebindLabelValidation { + active_label_present, + clear_needs_attention_label: true, + }); + } + + eyre::bail!( + "Issue `{}` has needs-attention label `{}`.", + issue.identifier, + needs_attention_label + ) } fn apply_review_handoff_rebind( @@ -1425,6 +1496,16 @@ fn apply_review_handoff_rebind( return Err(error); } + + if validation.clear_needs_attention_label { + tracker::set_issue_label_presence( + &context.tracker, + &validation.issue, + context.workflow.frontmatter().tracker().needs_attention_label(), + false, + )?; + } + if let Some(transition) = validation.success_state_transition.as_ref() { context.tracker.update_issue_state(&validation.issue.id, &transition.state_id)?; } @@ -1473,6 +1554,7 @@ fn review_handoff_rebind_event( format!("pr_url={pr_url}"), format!("pr_head_sha={}", validation.local_head_oid), format!("existing_review_handoff_marker={}", validation.mode.evidence_value()), + format!("needs_attention_label_repair={}", validation.clear_needs_attention_label), ]); event.next_action = Some(String::from("continue retained post-review lifecycle")); @@ -1965,6 +2047,44 @@ Test workflow. assert_eq!(transition.state_id, "state-review"); } + #[test] + fn rebind_state_allows_current_marker_failure_state_drift_recovery() { + let workflow = sample_workflow(); + let issue = sample_issue("Todo"); + let transition = super::validate_rebind_issue_state_for_policy( + workflow.frontmatter().tracker(), + &issue, + super::RebindMode::CompleteExistingHandoffState, + ) + .expect("current-marker state completion should recover failure-state drift") + .expect("failure-state drift should transition to success state"); + + assert_eq!(transition.state_name, "In Review"); + assert_eq!(transition.state_id, "state-review"); + } + + #[test] + fn rebind_state_rejects_failure_state_without_current_marker_repair_mode() { + let workflow = sample_workflow(); + let issue = sample_issue("Todo"); + + for mode in + [super::RebindMode::RestoreMissingHandoff, super::RebindMode::RefreshExistingHandoff] + { + let error = super::validate_rebind_issue_state_for_policy( + workflow.frontmatter().tracker(), + &issue, + mode, + ) + .expect_err("failure-state repair requires current-marker completion mode"); + + assert!( + error.to_string().contains("review handoff rebind requires"), + "unexpected error for {mode:?}: {error}" + ); + } + } + #[test] fn rebind_state_requires_success_state_for_existing_marker_refresh() { let workflow = sample_workflow(); @@ -2248,6 +2368,47 @@ Test workflow. assert_eq!(mode, super::RebindMode::RefreshExistingHandoff); } + #[test] + fn rebind_validation_rejects_stale_marker_failure_state_drift_recovery() { + let workflow = sample_workflow(); + let issue = sample_issue("Todo"); + let branch_name = "x/pubfi-pub-718"; + let pr_url = "https://github.com/hack-ink/pubfi-mono-v2/pull/14"; + let worktree = sample_worktree(branch_name); + let handoff = ReviewHandoffMarker::new( + "pub-718-attempt-1", + 1, + branch_name, + pr_url, + "main", + branch_name, + "0123456789abcdef0123456789abcdef01234567", + ); + let landing_state = + sample_landing_state(pr_url, branch_name, "1123456789abcdef0123456789abcdef01234567"); + let (_run_id, _attempt_number, mode) = super::validate_existing_handoff_refresh( + workflow.frontmatter().tracker(), + &issue, + &worktree, + &handoff, + None, + &landing_state, + "1123456789abcdef0123456789abcdef01234567", + ) + .expect("stale existing marker should require marker refresh first"); + + assert_eq!(mode, super::RebindMode::RefreshExistingHandoff); + + let error = super::validate_rebind_issue_state_for_policy( + workflow.frontmatter().tracker(), + &issue, + mode, + ) + .expect_err("stale marker refresh must not repair failure-state drift"); + + assert!(error.to_string().contains("review handoff rebind requires")); + } + #[test] fn rebind_validation_rejects_current_existing_marker_as_noop() { let workflow = sample_workflow(); @@ -2342,6 +2503,54 @@ Test workflow. assert_eq!(mode, super::RebindMode::CompleteExistingHandoffState); } + #[test] + fn rebind_validation_completes_current_existing_marker_failure_state_drift() { + let workflow = sample_workflow(); + let issue = sample_issue("Todo"); + let branch_name = "x/pubfi-pub-718"; + let pr_url = "https://github.com/hack-ink/pubfi-mono-v2/pull/14"; + let head_oid = "1123456789abcdef0123456789abcdef01234567"; + let worktree = sample_worktree(branch_name); + let handoff = ReviewHandoffMarker::new( + "pub-718-attempt-1", + 1, + branch_name, + pr_url, + "main", + branch_name, + head_oid, + ); + let orchestration = ReviewOrchestrationMarker::new( + "pub-718-attempt-1", + 1, + branch_name, + pr_url, + head_oid, + "request_pending", + None, + None, + None, + 0, + 0, + None, + ); + let landing_state = sample_landing_state(pr_url, branch_name, head_oid); + let (run_id, attempt_number, mode) = super::validate_existing_handoff_refresh( + workflow.frontmatter().tracker(), + &issue, + &worktree, + &handoff, + Some(&orchestration), + &landing_state, + head_oid, + ) + .expect("current marker should allow failure-state drift completion"); + + assert_eq!(run_id, "pub-718-attempt-1"); + assert_eq!(attempt_number, 1); + assert_eq!(mode, super::RebindMode::CompleteExistingHandoffState); + } + #[test] fn review_handoff_rebind_event_validation_accepts_required_fields() { let mut record = LinearExecutionEventRecord::new( diff --git a/docs/runbook/recover-review-handoff.md b/docs/runbook/recover-review-handoff.md index 559e08e39..6e682dc5a 100644 --- a/docs/runbook/recover-review-handoff.md +++ b/docs/runbook/recover-review-handoff.md @@ -53,14 +53,22 @@ worktree head matches the PR head. For a partial normal handoff where the marker missing, or where an already-current marker exists but the issue state was not advanced, the command may also move the issue from the workflow `tracker.in_progress_state` to `tracker.success_state` after the rebind audit -succeeds. It does not merge the PR, queue follow-up issues, or clean worktrees. +succeeds. If stale failure writeback already moved an already-current marker lane to +`tracker.failure_state` and added `tracker.needs_attention_label`, rebind may clear that +label and move the issue to `tracker.success_state`; this is only for current same-PR +same-head markers, not for missing or stale marker recovery. It does not merge the PR, +queue follow-up issues, or clean worktrees. The command rejects the rebind unless all of these are true: -- the issue is in the workflow `tracker.success_state`, or the issue is still in +- the issue is in the workflow `tracker.success_state`, still in `tracker.in_progress_state` from a partial normal handoff with a missing or - already-current marker -- the issue does not have opt-out or needs-attention labels + already-current marker, or in `tracker.failure_state` only when an already-current + marker proves that stale failure writeback caused tracker state drift +- the issue does not have the opt-out label +- the issue does not have the needs-attention label, except for the already-current + marker plus `tracker.failure_state` drift case where rebind clears it after recording + the audit - the issue still has `decodex:active:` ownership - the retained worktree branch matches the runtime DB worktree mapping - the retained worktree has no local source changes except top-level Decodex runtime diff --git a/docs/spec/post-review-lifecycle.md b/docs/spec/post-review-lifecycle.md index 6efc5d378..bd6b3254d 100644 --- a/docs/spec/post-review-lifecycle.md +++ b/docs/spec/post-review-lifecycle.md @@ -107,8 +107,13 @@ success path. requires the workflow `tracker.success_state`. Partial normal handoff recovery may also accept the workflow `tracker.in_progress_state` when the marker is missing, or when an already-current marker exists but the issue state was not advanced, and the - validated PR plus retained worktree prove the handoff lineage; after the rebind audit - succeeds, Decodex must move that issue to `tracker.success_state`. + validated PR plus retained worktree prove the handoff lineage. If stale failure + writeback already moved that already-current marker lane back to + `tracker.failure_state` and applied `tracker.needs_attention_label`, explicit rebind + may clear that label and move the issue to `tracker.success_state` after the rebind + audit succeeds. Decodex must reject this failure-state recovery for missing or stale + markers because those still need explicit PR-lineage repair before tracker state can + be trusted. - If no review handoff marker exists, `rebind` restores the missing handoff and orchestration markers from the validated PR and retained worktree. If a marker already exists for the same branch and PR but its stored handoff or orchestration head is diff --git a/docs/spec/runtime.md b/docs/spec/runtime.md index 005430ec6..dbe6c9db3 100644 --- a/docs/spec/runtime.md +++ b/docs/spec/runtime.md @@ -398,10 +398,15 @@ and schedules a materially different recovery strategy. Otherwise it records a terminal recovery reason such as `contract_boundary_required`, `external_dependency_required`, or `architecture_recovery_exhausted` and routes the lane through the human-required failure path. Repo-gate failures record both -`validation_repeat` and `remaining_delta_unchanged` observations; retryable failures -with no changing tracked delta record `no_effective_diff`. Fingerprints use the lane -HEAD plus the tracked worktree status and diff against `HEAD`, so retained partial -progress remains inspectable instead of being deleted or hidden. +`validation_repeat` and `remaining_delta_unchanged` observations. Retryable failures +record `no_effective_diff` only when the retained worktree has no effective source +delta: no tracked status/diff against `HEAD` and no ordinary untracked files after +excluding Decodex runtime artifacts such as `.decodex-run-activity` and +`.decodex-run-control/`. Dirty retained tracked patches and untracked new source files +are retained progress, not no-effective work. Fingerprints use the lane HEAD plus the +effective worktree status and tracked diff against `HEAD`, so retained partial progress +remains inspectable instead of being deleted, hidden, or mislabeled as an empty-diff +retry. When `[codex].review` is `"standard"` or `"strict"`, handoff and retained review-repair runs also consume the latest structured `issue_review_checkpoint` @@ -530,9 +535,9 @@ preserves a structured failure-attribution `error_class` so operator status and Linear summaries can distinguish the stop class or recovery boundary: - `validation_repeat`: the same validation failure repeated three times. -- `no_effective_diff`: retryable attempts repeated without a changed tracked delta. -- `remaining_delta_unchanged`: validation text changed but the remaining tracked delta - stayed unchanged for three attempts. +- `no_effective_diff`: retryable attempts repeated without any effective worktree delta. +- `remaining_delta_unchanged`: validation text changed but the remaining effective + worktree delta stayed unchanged for three attempts. - `dependency_program_stale`: a queued issue kept the same open dependency blocker fingerprint across three status observations, indicating Execution Program readiness or issue decomposition is stale. diff --git a/plugins/decodex/skills/automation/SKILL.md b/plugins/decodex/skills/automation/SKILL.md index eabeab0f1..976912f21 100644 --- a/plugins/decodex/skills/automation/SKILL.md +++ b/plugins/decodex/skills/automation/SKILL.md @@ -71,7 +71,8 @@ cargo run -p decodex --bin decodex -- serve `decodex:active:`. - `decodex:manual-only` opts out of automation. - `decodex:needs-attention` is a human-required stop that automation must not silently - retry. + retry. The only runtime-owned clear path is the explicit review-handoff rebind + recovery where a current same-PR same-head marker proves stale failure-state drift. - A lane is terminal only after exactly one terminal path is finalized: `review_handoff` or `manual_attention`. - `phase = terminal_pending` means the agent already called terminal finalize and diff --git a/plugins/decodex/skills/labels/SKILL.md b/plugins/decodex/skills/labels/SKILL.md index 38efbe758..441585f55 100644 --- a/plugins/decodex/skills/labels/SKILL.md +++ b/plugins/decodex/skills/labels/SKILL.md @@ -44,6 +44,8 @@ If a project-scoped command supplies `--config `, read that project - Humans may add or clear `decodex:manual-only`. - Humans may clear `decodex:needs-attention` only after addressing the underlying blocker recorded by the runtime or agent. +- `decodex recover review-handoff rebind` may clear `decodex:needs-attention` itself + only when a current same-PR same-head handoff marker proves stale failure-state drift. - Humans should not normally add or clear `decodex:active:` unless doing explicit recovery and the runtime-owned state has been verified. @@ -72,7 +74,8 @@ If a project-scoped command supplies `--config `, read that project 1. Read the failure or attention comment first. 2. Resolve the underlying blocker. -3. Clear `decodex:needs-attention`. +3. Clear `decodex:needs-attention`, or use `decodex recover review-handoff rebind` + when the blocker is verified current-marker failure-state drift. 4. Re-add `decodex:queued:` only when the issue should resume automation. ## Boundaries diff --git a/plugins/decodex/skills/manual-cli/SKILL.md b/plugins/decodex/skills/manual-cli/SKILL.md index 3b1f2f987..a8202f123 100644 --- a/plugins/decodex/skills/manual-cli/SKILL.md +++ b/plugins/decodex/skills/manual-cli/SKILL.md @@ -58,6 +58,9 @@ registered project's `WORKFLOW.md`. - Use `land` only when the user asks to land a human-driven PR through Decodex. - Use `run --dry-run` only as an intake/worktree-planning check. It does not prove live tracker writes, PR handoff, closeout, or app-server execution will succeed. +- Use `recover review-handoff diagnose` and then `recover review-handoff rebind` for + retained PR handoff state drift; the live rebind owns marker refresh plus the narrow + current-marker failure-state label/state repair described in the runbook. - Use `probe stdio://` before relying on the Codex app-server boundary. - Use `POST /api/linear-scan` after label or issue-state changes when the scheduler should refresh before its next 5-minute Linear poll.