mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-08-28 05:27:41 +00:00
fix(cli): re-prompt invalid attach interview input
Keep invalid terminal answers local to attach prompts instead of treating them as interrupted interviews, and accept No for confirmation answers to match documented yes/no behavior.
This commit is contained in:
parent
1a43cf5abc
commit
9a3ab8bbba
4 changed files with 338 additions and 30 deletions
|
|
@ -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<u8>) -> 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::<Vec<_>>();
|
||||
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<u8>) -> 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<u8>) -> 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<u8>) -> 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::<Option<Vec<_>>>();
|
||||
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::<Vec<_>>()
|
||||
.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<Answer> {
|
||||
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));
|
||||
|
|
|
|||
|
|
@ -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!();
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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 {
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue