diff --git a/crates/coven-cli/src/tui/cast/quest.rs b/crates/coven-cli/src/tui/cast/quest.rs index 296c39e..ae007ca 100644 --- a/crates/coven-cli/src/tui/cast/quest.rs +++ b/crates/coven-cli/src/tui/cast/quest.rs @@ -238,14 +238,20 @@ pub(crate) fn compose_sub_prompt(phase: &QuestPhase, quest_goal: &str) -> String pub(crate) fn advance(quest: &mut Quest, summary: QuestPhaseSummary) -> Option { let current = quest.current_index()?; let from_name = quest.phases[current].name.clone(); + let failed = phase_failed(&summary); let prior_status_label = phase_status_label(&summary); - let reason = handoff_reason(&from_name, &prior_status_label); + let reason = handoff_reason(&from_name, &prior_status_label, failed); let carried = summary.carried_context.clone(); quest.phases[current].status = QuestPhaseStatus::Complete(summary); - quest.cursor = current + 1; - - let next_index = quest.current_index()?; + // Advance the cursor to the next *pending* phase so we never leave + // it pointing at a Skipped or Complete row. When no pending phase + // remains, park the cursor at `phases.len()` so `current_index` + // reports the quest as exhausted. + let next_index = next_pending_index(quest, current + 1); + quest.cursor = next_index.unwrap_or(quest.phases.len()); + + let next_index = next_index?; let goal = quest.goal.clone(); let next = &mut quest.phases[next_index]; next.handoff = Some(QuestHandoff { @@ -336,7 +342,10 @@ pub(crate) fn skip_phase(quest: &mut Quest, index: usize, reason: String) -> Res } phase.status = QuestPhaseStatus::Skipped { reason }; if quest.cursor == index { - quest.cursor = index + 1; + // Skipping the current phase walks the cursor forward to the + // next pending row — never leaves it pointing at a Skipped or + // Complete phase. + quest.cursor = next_pending_index(quest, index + 1).unwrap_or(quest.phases.len()); } Ok(()) } @@ -437,12 +446,26 @@ fn phase_status_label(summary: &QuestPhaseSummary) -> String { } } -fn handoff_reason(from_phase: &str, prior_status_label: &str) -> String { - let lower = prior_status_label.to_ascii_lowercase(); - let failed = lower.starts_with("failed") - || lower.contains("error") - || lower.contains("exit 1") - || lower.contains("interrupted"); +/// Whether a phase summary should be framed as a failure when composing +/// the next sub-prompt. Keyed on structured data first: any non-zero exit +/// code is a failure regardless of the status string. The status-string +/// fallback catches cases where `exit_code` is `None` (e.g. interrupted +/// runs that never produced an exit code). +fn phase_failed(summary: &QuestPhaseSummary) -> bool { + if let Some(code) = summary.exit_code { + if code != 0 { + return true; + } + } + summary.exit_status.as_deref().is_some_and(|status| { + matches!( + status.trim().to_ascii_lowercase().as_str(), + "failed" | "error" | "interrupted" + ) + }) +} + +fn handoff_reason(from_phase: &str, prior_status_label: &str, failed: bool) -> String { if failed { format!( "Phase `{from_phase}` finished with `{prior_status_label}` — incorporate the failure context before continuing." @@ -454,6 +477,16 @@ fn handoff_reason(from_phase: &str, prior_status_label: &str) -> String { } } +/// First index >= `start` whose phase is still Pending. Used by +/// `advance` and `skip_phase` so the cursor always lands on real work +/// (never on a row that was already completed or explicitly skipped). +fn next_pending_index(quest: &Quest, start: usize) -> Option { + quest.phases[start..] + .iter() + .position(|phase| matches!(phase.status, QuestPhaseStatus::Pending)) + .map(|offset| start + offset) +} + fn derive_quest_title(goal: &str) -> String { let collapsed: String = goal.split_whitespace().collect::>().join(" "); if collapsed.is_empty() { @@ -651,12 +684,64 @@ mod tests { ..QuestPhaseSummary::default() }, ); - // Implement completes; cursor lands on the skipped verify (2). The - // next `current()` call shows verify as skipped so the shell can - // jump past it without re-prompting. + // Implement completes; verify is already Skipped, so advance walks + // the cursor straight past it — the quest is exhausted, not + // stranded on a Skipped row. + assert!(q.is_complete()); + assert_eq!(q.cursor, q.phases.len()); + assert!(q.current().is_none()); + assert!(matches!( + q.phases[2].status, + QuestPhaseStatus::Skipped { .. } + )); + } + + #[test] + fn advance_skips_over_skipped_phases_and_preserves_skip_reason() { + let mut q = quest("publish a release"); + skip_phase(&mut q, 1, "implementation already done".to_string()).unwrap(); + + let next = advance( + &mut q, + QuestPhaseSummary { + exit_status: Some("completed".to_string()), + exit_code: Some(0), + ..QuestPhaseSummary::default() + }, + ); + + assert_eq!( + next, + Some(2), + "advance should jump straight to the next pending phase" + ); assert_eq!(q.cursor, 2); - let current = q.current().expect("verify exists"); - assert!(matches!(current.status, QuestPhaseStatus::Skipped { .. })); + assert!(matches!( + q.phases[1].status, + QuestPhaseStatus::Skipped { ref reason } if reason == "implementation already done" + )); + } + + #[test] + fn non_zero_exit_codes_use_failure_handoff_reason() { + let mut q = quest("verify build"); + advance( + &mut q, + QuestPhaseSummary { + // status string says "completed" — the failure framing + // must come from the structured exit_code, not the label. + exit_status: Some("completed".to_string()), + exit_code: Some(2), + ..QuestPhaseSummary::default() + }, + ); + + let handoff = q.phases[1].handoff.as_ref().expect("handoff attached"); + assert!( + handoff.reason.contains("incorporate the failure context"), + "non-zero exit codes must produce failure framing, got: {}", + handoff.reason + ); } #[test]