diff --git a/lib/crates/fabro-cli/src/commands/run/attach.rs b/lib/crates/fabro-cli/src/commands/run/attach.rs index 63fc71004..a192c6a1f 100644 --- a/lib/crates/fabro-cli/src/commands/run/attach.rs +++ b/lib/crates/fabro-cli/src/commands/run/attach.rs @@ -46,6 +46,12 @@ enum PromptRead { Error, } +enum ParsedPromptAnswer { + Answer(Answer), + Invalid(String), + Interrupted, +} + #[cfg(unix)] use nix::errno::Errno; #[cfg(unix)] @@ -86,6 +92,14 @@ impl NonblockingStdin { fn read_line(&self, buffer: &mut Vec) -> LineRead { let mut chunk = [0_u8; 256]; loop { + if let Some(newline) = buffer.iter().position(|byte| *byte == b'\n') { + let line = buffer.drain(..=newline).collect::>(); + return LineRead::Complete( + String::from_utf8_lossy(&line) + .trim_end_matches(['\r', '\n']) + .to_string(), + ); + } match unistd::read(self.stdin.as_fd(), &mut chunk) { Ok(0) => { return if buffer.is_empty() { @@ -433,6 +447,7 @@ fn api_question_to_question(question: &types::ApiQuestion) -> Question { reason = "Interactive questions and options belong on stderr, not captured stdout." )] async fn ask_attach_question(question: Question, styles: &'static Styles) -> Answer { + let mut input_buffer = Vec::new(); if let Some(ref context_text) = question.context_display { let rendered = styles.render_markdown(context_text); eprint!("{rendered}"); @@ -454,12 +469,31 @@ async fn ask_attach_question(question: Question, styles: &'static Styles) -> Ans if question.allow_freeform { eprintln!(" Or type a free-text response"); } - parse_choice_response(&question, read_attach_line("Select: ").await) + loop { + match parse_choice_response( + &question, + read_attach_line("Select: ", &mut input_buffer).await, + ) { + ParsedPromptAnswer::Answer(answer) => return answer, + ParsedPromptAnswer::Invalid(message) => eprintln!("{message}"), + ParsedPromptAnswer::Interrupted => return Answer::interrupted(), + } + } } - QuestionType::YesNo | QuestionType::Confirmation => { - parse_confirm_response(read_attach_line("[Y/N]: ").await) - } - QuestionType::Freeform => parse_freeform_response(read_attach_line("> ").await), + QuestionType::YesNo | QuestionType::Confirmation => loop { + match parse_confirm_response(read_attach_line("[Y/N]: ", &mut input_buffer).await) { + ParsedPromptAnswer::Answer(answer) => return answer, + ParsedPromptAnswer::Invalid(message) => eprintln!("{message}"), + ParsedPromptAnswer::Interrupted => return Answer::interrupted(), + } + }, + QuestionType::Freeform => loop { + match parse_freeform_response(read_attach_line("> ", &mut input_buffer).await) { + ParsedPromptAnswer::Answer(answer) => return answer, + ParsedPromptAnswer::Invalid(message) => eprintln!("{message}"), + ParsedPromptAnswer::Interrupted => return Answer::interrupted(), + } + }, } } @@ -467,20 +501,19 @@ async fn ask_attach_question(question: Question, styles: &'static Styles) -> Ans clippy::print_stderr, reason = "Prompts go to stderr so piped stdout stays machine-readable." )] -async fn read_attach_line(prompt: &str) -> PromptRead { +async fn read_attach_line(prompt: &str, buffer: &mut Vec) -> PromptRead { eprint!("{prompt}"); let _ = std::io::stderr().flush(); - read_attach_line_after_prompt().await + read_attach_line_after_prompt(buffer).await } #[cfg(unix)] -async fn read_attach_line_after_prompt() -> PromptRead { +async fn read_attach_line_after_prompt(buffer: &mut Vec) -> PromptRead { let Some(stdin) = NonblockingStdin::new() else { return PromptRead::Error; }; - let mut buffer = Vec::new(); loop { - match stdin.read_line(&mut buffer) { + match stdin.read_line(buffer) { LineRead::Pending => sleep(PROMPT_READ_POLL_INTERVAL).await, LineRead::Complete(line) => return PromptRead::Line(line), LineRead::Eof => return PromptRead::Eof, @@ -490,7 +523,7 @@ async fn read_attach_line_after_prompt() -> PromptRead { } #[cfg(not(unix))] -async fn read_attach_line_after_prompt() -> PromptRead { +async fn read_attach_line_after_prompt(_buffer: &mut Vec) -> PromptRead { use tokio::io::{self, AsyncBufReadExt, BufReader}; let stdin = io::stdin(); @@ -503,12 +536,12 @@ async fn read_attach_line_after_prompt() -> PromptRead { } } -fn parse_choice_response(question: &Question, prompt_read: PromptRead) -> Answer { +fn parse_choice_response(question: &Question, prompt_read: PromptRead) -> ParsedPromptAnswer { let PromptRead::Line(response) = prompt_read else { - return Answer::interrupted(); + return ParsedPromptAnswer::Interrupted; }; if response.trim().is_empty() { - return Answer::interrupted(); + return ParsedPromptAnswer::Invalid(invalid_choice_message(question)); } if question.question_type == QuestionType::MultiSelect { let selected = response @@ -531,40 +564,54 @@ fn parse_choice_response(question: &Question, prompt_read: PromptRead) -> Answer }) .collect::>>(); if let Some(selected) = selected.filter(|keys| !keys.is_empty()) { - return Answer::multi_selected(selected); + return ParsedPromptAnswer::Answer(Answer::multi_selected(selected)); } + return ParsedPromptAnswer::Invalid(invalid_choice_message(question)); } if let Some(answer) = find_matching_option(&response, &question.options) { - return answer; + return ParsedPromptAnswer::Answer(answer); } if question.allow_freeform { - return Answer::text(response); + return ParsedPromptAnswer::Answer(Answer::text(response)); } - Answer::interrupted() + ParsedPromptAnswer::Invalid(invalid_choice_message(question)) } -fn parse_confirm_response(prompt_read: PromptRead) -> Answer { +fn parse_confirm_response(prompt_read: PromptRead) -> ParsedPromptAnswer { let PromptRead::Line(response) = prompt_read else { - return Answer::interrupted(); + return ParsedPromptAnswer::Interrupted; }; match response.trim().to_lowercase().as_str() { - "y" | "yes" => Answer::yes(), - "n" | "no" => Answer::no(), - _ => Answer::interrupted(), + "y" | "yes" => ParsedPromptAnswer::Answer(Answer::yes()), + "n" | "no" => ParsedPromptAnswer::Answer(Answer::no()), + _ => ParsedPromptAnswer::Invalid("Please enter y or n.".to_string()), } } -fn parse_freeform_response(prompt_read: PromptRead) -> Answer { +fn parse_freeform_response(prompt_read: PromptRead) -> ParsedPromptAnswer { let PromptRead::Line(response) = prompt_read else { - return Answer::interrupted(); + return ParsedPromptAnswer::Interrupted; }; if response.trim().is_empty() { - Answer::interrupted() + ParsedPromptAnswer::Invalid("Please enter a response.".to_string()) } else { - Answer::text(response) + ParsedPromptAnswer::Answer(Answer::text(response)) } } +fn invalid_choice_message(question: &Question) -> String { + let keys = question + .options + .iter() + .map(|option| option.key.as_str()) + .collect::>() + .join(", "); + if keys.is_empty() { + return "Please enter one of the listed options.".to_string(); + } + format!("Please enter one of: {keys}.") +} + fn find_matching_option(response: &str, options: &[InterviewOption]) -> Option { let trimmed = response.trim(); for opt in options { @@ -896,6 +943,79 @@ mod tests { assert!(!answer_requires_reattach(&answered)); } + #[test] + fn invalid_confirm_response_is_user_correctable() { + let response = parse_confirm_response(PromptRead::Line("dasf".to_string())); + + assert!(matches!(response, ParsedPromptAnswer::Invalid(_))); + } + + #[test] + fn eof_confirm_response_is_interrupted() { + let response = parse_confirm_response(PromptRead::Eof); + + assert!(matches!(response, ParsedPromptAnswer::Interrupted)); + } + + #[test] + fn invalid_multiple_choice_without_freeform_is_user_correctable() { + let mut question = Question::new("Pick one.", QuestionType::MultipleChoice); + question.options = vec![InterviewOption { + key: "A".to_string(), + label: "Approve".to_string(), + }]; + + let response = parse_choice_response(&question, PromptRead::Line("bogus".to_string())); + + assert!(matches!(response, ParsedPromptAnswer::Invalid(_))); + } + + #[test] + fn unmatched_multiple_choice_with_freeform_remains_text() { + let mut question = Question::new("Pick one.", QuestionType::MultipleChoice); + question.options = vec![InterviewOption { + key: "A".to_string(), + label: "Approve".to_string(), + }]; + question.allow_freeform = true; + + let response = parse_choice_response(&question, PromptRead::Line("custom".to_string())); + + assert!(matches!( + response, + ParsedPromptAnswer::Answer(Answer { + value: AnswerValue::Text(text), + .. + }) if text == "custom" + )); + } + + #[test] + fn invalid_multi_select_token_is_user_correctable() { + let mut question = Question::new("Pick many.", QuestionType::MultiSelect); + question.options = vec![ + InterviewOption { + key: "A".to_string(), + label: "Approve".to_string(), + }, + InterviewOption { + key: "N".to_string(), + label: "Notify".to_string(), + }, + ]; + + let response = parse_choice_response(&question, PromptRead::Line("A bogus".to_string())); + + assert!(matches!(response, ParsedPromptAnswer::Invalid(_))); + } + + #[test] + fn empty_freeform_response_is_user_correctable() { + let response = parse_freeform_response(PromptRead::Line(" ".to_string())); + + assert!(matches!(response, ParsedPromptAnswer::Invalid(_))); + } + #[test] fn json_pending_interview_requires_manual_input_when_auto_approve_is_disabled() { assert!(json_pending_interview_requires_manual_input(true, false)); diff --git a/lib/crates/fabro-cli/tests/it/cmd/attach.rs b/lib/crates/fabro-cli/tests/it/cmd/attach.rs index 9104592f3..b4fc5a53a 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/attach.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/attach.rs @@ -3,7 +3,7 @@ reason = "integration tests: read child-process stdout line-by-line via std::io::BufReader" )] -use std::io::{BufRead, BufReader, Read}; +use std::io::{BufRead, BufReader, Read, Write}; use std::process::{Output, Stdio}; use std::sync::mpsc; use std::time::{Duration, Instant}; @@ -146,6 +146,177 @@ fn wait_for_child_exit(child: &mut std::process::Child, label: &str) -> std::pro } } +fn start_detached_human_run( + context: &fabro_test::TestContext, + filename: &str, + source: &str, +) -> String { + context.ensure_home_server_auth_methods(); + let workflow = context.temp_dir.join(filename); + context.write_temp(filename, source); + + let output = context + .command() + .env("OPENAI_API_KEY", "test") + .args([ + "run", + "--detach", + "--no-retro", + "--sandbox", + "local", + "--provider", + "openai", + workflow.to_str().expect("workflow path should be UTF-8"), + ]) + .output() + .expect("detached run should execute"); + assert!( + output.status.success(), + "detached run failed:\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + output_stdout(&output).trim().to_string() +} + +fn wait_for_pending_question(context: &fabro_test::TestContext, run_id: &str) { + tokio::runtime::Runtime::new() + .expect("test runtime should build") + .block_on(async { + let (client, base_url) = + server_endpoint(&context.storage_dir).expect("server endpoint should exist"); + wait_for_server_question(&client, &base_url, run_id).await; + }); +} + +#[expect( + clippy::disallowed_methods, + reason = "This sync integration helper writes scripted answers to an attach child process." +)] +fn attach_with_stdin(context: &fabro_test::TestContext, run_id: &str, input: &[u8]) -> Output { + let mut attach_cmd = std::process::Command::new(env!("CARGO_BIN_EXE_fabro")); + fabro_test::apply_test_isolation(&mut attach_cmd, &context.home_dir); + attach_cmd.current_dir(&context.temp_dir); + attach_cmd.args(["attach", run_id]); + attach_cmd.stdin(Stdio::piped()); + attach_cmd.stdout(Stdio::piped()); + attach_cmd.stderr(Stdio::piped()); + + let mut child = attach_cmd.spawn().expect("attach should spawn"); + let mut stdout = child.stdout.take().expect("attach stdout should be piped"); + let mut stderr = child.stderr.take().expect("attach stderr should be piped"); + { + let mut stdin = child.stdin.take().expect("attach stdin should be piped"); + stdin + .write_all(input) + .expect("scripted attach input should be writable"); + } + + let status = wait_for_child_exit(&mut child, "attach"); + let mut stdout_bytes = Vec::new(); + stdout + .read_to_end(&mut stdout_bytes) + .expect("attach stdout should be readable"); + let mut stderr_bytes = Vec::new(); + stderr + .read_to_end(&mut stderr_bytes) + .expect("attach stderr should be readable"); + Output { + status, + stdout: stdout_bytes, + stderr: stderr_bytes, + } +} + +#[test] +fn attach_reprompts_invalid_yes_no_then_accepts_valid_answer() { + let context = test_context!(); + let run_id = start_detached_human_run( + &context, + "yes-no-gate.fabro", + r#"digraph HumanGate { + graph [goal="Wait for yes/no"] + start [shape=Mdiamond, label="Start"] + exit [shape=Msquare, label="Exit"] + approve [shape=hexagon, label="Continue?", question_type="yes_no"] + ship [shape=parallelogram, script="echo shipped"] + start -> approve + approve -> ship [label="[Y] Yes"] + ship -> exit +} +"#, + ); + let cleanup_run_id = run_id.clone(); + scopeguard::defer! { + let _ = context.command().args(["rm", "--force", &cleanup_run_id]).output(); + } + wait_for_pending_question(&context, &run_id); + + let output = attach_with_stdin(&context, &run_id, b"dasf\ny\n"); + + assert!( + output.status.success(), + "attach should succeed after corrected yes/no input:\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + let stderr = String::from_utf8(output.stderr).expect("stderr should be UTF-8"); + assert!( + stderr.contains("Please enter y or n."), + "attach should explain invalid yes/no input:\n{stderr}" + ); + assert!( + !stderr.contains("Interview ended without an answer"), + "invalid input should not detach the interview:\n{stderr}" + ); +} + +#[test] +fn attach_reprompts_invalid_choice_then_accepts_valid_answer() { + let context = test_context!(); + let run_id = start_detached_human_run( + &context, + "choice-gate.fabro", + r#"digraph HumanGate { + graph [goal="Wait for choice"] + start [shape=Mdiamond, label="Start"] + exit [shape=Msquare, label="Exit"] + approve [shape=hexagon, label="Approve?"] + ship [shape=parallelogram, script="echo shipped"] + revise [shape=parallelogram, script="echo revised"] + start -> approve + approve -> ship [label="[A] Approve"] + approve -> revise [label="[R] Revise"] + ship -> exit + revise -> exit +} +"#, + ); + let cleanup_run_id = run_id.clone(); + scopeguard::defer! { + let _ = context.command().args(["rm", "--force", &cleanup_run_id]).output(); + } + wait_for_pending_question(&context, &run_id); + + let output = attach_with_stdin(&context, &run_id, b"bogus\nA\n"); + + assert!( + output.status.success(), + "attach should succeed after corrected choice input:\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + let stderr = String::from_utf8(output.stderr).expect("stderr should be UTF-8"); + assert!( + stderr.contains("Please enter one of: A, R."), + "attach should explain invalid choice input:\n{stderr}" + ); + assert!( + !stderr.contains("Interview ended without an answer"), + "invalid input should not detach the interview:\n{stderr}" + ); +} + #[test] fn attach_replays_completed_detached_run() { let context = test_context!(); diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index 47ef28f5d..18e63dc3a 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -2593,10 +2593,9 @@ fn validate_answer_for_question( ) -> Result<(), Response> { match (&question.question_type, &answer.value) { ( - QuestionType::YesNo, + QuestionType::YesNo | QuestionType::Confirmation, fabro_interview::AnswerValue::Yes | fabro_interview::AnswerValue::No, ) - | (QuestionType::Confirmation, fabro_interview::AnswerValue::Yes) | ( _, fabro_interview::AnswerValue::Interrupted diff --git a/lib/crates/fabro-server/src/server/tests.rs b/lib/crates/fabro-server/src/server/tests.rs index 083bdc724..2693c406b 100644 --- a/lib/crates/fabro-server/src/server/tests.rs +++ b/lib/crates/fabro-server/src/server/tests.rs @@ -4049,6 +4049,24 @@ async fn submit_pending_interview_answer_rejects_invalid_answer_shape() { assert_status!(response, StatusCode::BAD_REQUEST).await; } +#[test] +fn validate_answer_for_question_accepts_no_for_confirmation() { + let question = InterviewQuestionRecord { + id: "q-1".to_string(), + text: "Continue?".to_string(), + stage: "gate".to_string(), + question_type: QuestionType::Confirmation, + options: vec![], + allow_freeform: false, + timeout_seconds: None, + context_display: None, + }; + + let result = validate_answer_for_question(&question, &Answer::no()); + + assert!(result.is_ok()); +} + #[test] fn answer_from_typed_yes_request_maps_to_yes_answer() { let question = InterviewQuestionRecord {