From 78d66507f7ef55e5981c6f1bc047ef9cebe7370b Mon Sep 17 00:00:00 2001 From: Happy Mahlangu Date: Sun, 6 Sep 2026 19:40:11 +0200 Subject: [PATCH] feat(agent,cli): recognize load-timing races as a distinct repair category MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The repair loop had two moves: rewrite a step's text, or declare it an engine gap. That meant a genuine timing race — the target was right, the page just hadn't settled — burned an attempt guessing new (wrong) text instead of just waiting longer, since there was no way to say "the target is fine, give it more time." Adds a third option to the patch contract: widen_timeout_seconds. It takes a narrower apply path than a full step rewrite (widen_timeout regex-replaces only the `within Ns` window, capped at 120s) so a timing-race fix can't also change what the step is waiting for. AgentError::PreviousStepIncomplete ("the page reports: still loading") now gets its own diagnosis category, page-not-ready-transient, instead of falling into the generic engine-gap bucket, so the prompt can ask the model to distinguish "wrong target" from "right target, not settled yet". Verified live: replayed the exact multi-drift scenario that previously exhausted its attempt budget on a real Fiori tenant. This time the timing-race step got its wait window widened (60s -> 120s) instead of a text guess, and the flow converged to a pass within budget. --- crates/flowproof-agent/src/lib.rs | 4 +- crates/flowproof-agent/src/repair.rs | 155 +++++++++++++++++++++++++-- crates/flowproof-cli/src/lib.rs | 35 +++--- 3 files changed, 163 insertions(+), 31 deletions(-) diff --git a/crates/flowproof-agent/src/lib.rs b/crates/flowproof-agent/src/lib.rs index 53a99c6..f072588 100644 --- a/crates/flowproof-agent/src/lib.rs +++ b/crates/flowproof-agent/src/lib.rs @@ -25,8 +25,8 @@ pub use recorder::{ record_with_author_and_options, surface_targets, Author, RecordError, RecordSummary, }; pub use repair::{ - apply_patch, diagnose, find_step_index, propose_patch, FailureContext, ProposedPatch, - RepairAttempt, RepairError, RepairOptions, RepairOutcome, RepairReport, + apply_patch, diagnose, find_step_index, propose_patch, widen_timeout, FailureContext, + ProposedPatch, RepairAttempt, RepairError, RepairOptions, RepairOutcome, RepairReport, }; pub use spec::{ check_control_ids, FlowSpec, LoginSpec, McpServerSpec, SessionRef, SpecStep, SuiteManifest, diff --git a/crates/flowproof-agent/src/repair.rs b/crates/flowproof-agent/src/repair.rs index 5a8d08d..059c185 100644 --- a/crates/flowproof-agent/src/repair.rs +++ b/crates/flowproof-agent/src/repair.rs @@ -9,6 +9,12 @@ //! What this module will never do: edit anything other than the `steps:` //! entry it was asked to patch, or touch a file that is not the flow spec //! itself. There is no code-editing capability here, by construction. +//! +//! Two apply paths exist deliberately: [`apply_patch`] can rewrite a whole +//! step, while [`widen_timeout`] can only change a step's `within Ns` wait +//! window. The narrower one exists so a load-timing race — the target was +//! right, the page just hadn't settled — gets fixed without giving the +//! model a chance to also change what the step is waiting for. use crate::llm::ModelClient; use crate::recorder::RecordError; @@ -76,6 +82,17 @@ pub fn diagnose(err: &RecordError) -> FailureContext { failing_intent: None, detail: driver_err.to_string(), }, + // The previous step's target hadn't actually settled when the next + // step already moved on — a load-timing race, not a wrong text + // target. Distinct from the generic engine-gap bucket so the loop + // can offer "widen the timeout" instead of guessing new step text. + RecordError::Agent(AgentError::PreviousStepIncomplete { step, evidence }) => { + FailureContext { + category: "page-not-ready-transient", + failing_intent: Some(step.clone()), + detail: evidence.clone(), + } + } other => FailureContext { category: "engine-gap", failing_intent: None, @@ -85,14 +102,22 @@ pub fn diagnose(err: &RecordError) -> FailureContext { } /// A model-proposed edit to exactly one `steps:` entry. +/// +/// Exactly one of `step_yaml`, `widen_timeout_seconds`, or `engine_gap` +/// should be set. `widen_timeout_seconds` is the case where the model judges +/// the step's target correct but the page hadn't settled yet — a load-timing +/// race rather than a wrong-text problem — so the fix is a longer wait on +/// the same step, not different wording. #[derive(Debug, Clone, serde::Deserialize)] pub struct ProposedPatch { - /// `None` means the model judged this an engine gap rather than a - /// fixable flow step — see plan 007's "Engine-gap escalation". + /// `None` means either an engine gap or a timeout-widen — see the other + /// two fields. pub step_yaml: Option, pub rationale: String, #[serde(default)] pub engine_gap: Option, + #[serde(default)] + pub widen_timeout_seconds: Option, } #[derive(Debug, thiserror::Error)] @@ -133,13 +158,25 @@ pub fn propose_patch( engine limitation rather than something a flow edit can fix, say so \ instead of proposing a patch. Reply with ONLY a JSON object, no \ markdown fences, no prose: \ - {\"step_yaml\": string or null, \"rationale\": string, \"engine_gap\": string or null}. \ + {\"step_yaml\": string or null, \"rationale\": string, \"engine_gap\": string or null, \ + \"widen_timeout_seconds\": number or null}. \ + Exactly ONE of step_yaml, engine_gap, or widen_timeout_seconds should be \ + non-null. \ step_yaml must be exactly the VALUE of one `steps:` list item (either \ a bare string step or a single-key mapping) — do NOT include the \ - leading `- ` list marker itself, only what would follow it. Set \ - step_yaml to null and \ - engine_gap to a short explanation if this is not fixable by editing \ - the flow."; + leading `- ` list marker itself, only what would follow it. \ + widen_timeout_seconds: if the error shows the step's target text or \ + condition is CORRECT but the page simply hadn't finished loading or \ + settling yet (e.g. the error says a previous step's result was \ + 'still loading', or the expected text appears in the DOM but the \ + step's wait window was too short) — this is a load-timing race, not \ + a wrong target — set widen_timeout_seconds to a larger number of \ + seconds for the SAME wait condition, and leave step_yaml null. Only \ + use this when the target itself is confirmed correct; if the target \ + text genuinely never appears anywhere in the captured DOM, that is a \ + wrong-target problem — use step_yaml instead. Set step_yaml and \ + widen_timeout_seconds to null and engine_gap to a short explanation \ + if this is not fixable by editing the flow."; let mut user = format!( "Flow file:\n```yaml\n{flow_yaml}\n```\n\n\ @@ -223,6 +260,70 @@ pub fn apply_patch( serde_yaml::to_string(&doc).map_err(RepairError::Yaml) } +/// Hard ceiling on a model-widened timeout. Without this, a model that +/// mistakes a genuinely broken target for a timing race could quietly turn +/// a flaky test into one with a multi-minute wait rather than a real fix. +const MAX_WIDENED_TIMEOUT_SECONDS: u64 = 120; + +fn timeout_pattern() -> &'static regex::Regex { + static RE: std::sync::OnceLock = std::sync::OnceLock::new(); + RE.get_or_init(|| regex::Regex::new(r"within \d+s").expect("static regex is valid")) +} + +/// Widen the `within Ns` wait window on the step at `step_index`, leaving +/// every other word of the step untouched. This is deliberately narrower +/// than [`apply_patch`]: a timing-race fix should not be able to change +/// *what* the step waits for, only *how long*. +pub fn widen_timeout( + flow_yaml: &str, + step_index: usize, + new_timeout_secs: u64, +) -> Result { + let capped = new_timeout_secs.min(MAX_WIDENED_TIMEOUT_SECONDS); + let mut doc: serde_yaml::Value = serde_yaml::from_str(flow_yaml)?; + let steps = doc + .get_mut("steps") + .and_then(|v| v.as_sequence_mut()) + .ok_or_else(|| { + RepairError::BadModelOutput("flow yaml has no `steps:` sequence".to_string()) + })?; + if step_index >= steps.len() { + return Err(RepairError::StepNotFound(format!( + "index {step_index} out of range ({} steps)", + steps.len() + ))); + } + let rewrite = |s: &mut String| { + if timeout_pattern().is_match(s) { + *s = timeout_pattern() + .replace(s, format!("within {capped}s")) + .into_owned(); + true + } else { + false + } + }; + let widened = match &mut steps[step_index] { + serde_yaml::Value::String(s) => rewrite(s), + serde_yaml::Value::Mapping(map) => { + let mut any = false; + for (_, v) in map.iter_mut() { + if let serde_yaml::Value::String(s) = v { + any |= rewrite(s); + } + } + any + } + _ => false, + }; + if !widened { + return Err(RepairError::BadModelOutput( + "step has no `within Ns` wait window to widen".to_string(), + )); + } + serde_yaml::to_string(&doc).map_err(RepairError::Yaml) +} + /// One iteration of the repair loop, for reporting purposes. #[derive(Debug, Clone, serde::Serialize)] pub struct RepairAttempt { @@ -310,6 +411,46 @@ mod tests { assert!(matches!(err, RepairError::StepNotFound(_))); } + #[test] + fn widen_timeout_replaces_only_the_wait_window() { + let flow = "name: demo\napp: web\nurl: https://example.com\nsteps:\n - Wait until page shows Home within 30s\n"; + let patched = widen_timeout(flow, 0, 60).expect("widen applies"); + let doc: serde_yaml::Value = serde_yaml::from_str(&patched).expect("patched yaml parses"); + let steps = doc["steps"].as_sequence().expect("steps is a sequence"); + assert_eq!( + steps[0].as_str(), + Some("Wait until page shows Home within 60s") + ); + } + + #[test] + fn widen_timeout_is_capped() { + let flow = "name: demo\napp: web\nurl: https://example.com\nsteps:\n - Wait until page shows Home within 30s\n"; + let patched = widen_timeout(flow, 0, 99_999).expect("widen applies"); + assert!(patched.contains("within 120s"), "got: {patched}"); + } + + #[test] + fn widen_timeout_errors_when_step_has_no_wait_window() { + let flow = "name: demo\napp: web\nurl: https://example.com\nsteps:\n - Click \"Save\"\n"; + let err = widen_timeout(flow, 0, 60).expect_err("no timeout to widen"); + assert!(matches!(err, RepairError::BadModelOutput(_))); + } + + #[test] + fn diagnose_maps_previous_step_incomplete_to_transient_page_not_ready() { + let err = RecordError::Agent(AgentError::PreviousStepIncomplete { + step: "Wait until page shows Purchasing Organization within 30s".to_string(), + evidence: "still loading".to_string(), + }); + let ctx = diagnose(&err); + assert_eq!(ctx.category, "page-not-ready-transient"); + assert_eq!( + ctx.failing_intent.as_deref(), + Some("Wait until page shows Purchasing Organization within 30s") + ); + } + #[test] fn diagnose_maps_element_not_found_to_missing_target() { let err = RecordError::ElementNotFound { diff --git a/crates/flowproof-cli/src/lib.rs b/crates/flowproof-cli/src/lib.rs index caea5a1..bb55f41 100644 --- a/crates/flowproof-cli/src/lib.rs +++ b/crates/flowproof-cli/src/lib.rs @@ -958,27 +958,6 @@ fn run_repair_loop( ); } - let Some(step_yaml) = &patch.step_yaml else { - attempts.push(flowproof_agent::RepairAttempt { - attempt, - category: ctx.category.to_string(), - failing_intent: Some(failing_intent.clone()), - error_detail: ctx.detail.clone(), - rationale: Some(patch.rationale.clone()), - applied: false, - }); - return RepairLoopOutcome::GaveUp( - err, - flowproof_agent::RepairReport { - attempts, - outcome: flowproof_agent::RepairOutcome::BudgetExhausted { - last_error: "model proposed neither a patch nor an engine-gap reason" - .to_string(), - }, - }, - ); - }; - let Some(step_index) = flowproof_agent::find_step_index(&spec, &failing_intent) else { attempts.push(flowproof_agent::RepairAttempt { attempt, @@ -1001,7 +980,19 @@ fn run_repair_loop( ); }; - let patched_yaml = match flowproof_agent::apply_patch(&raw, step_index, step_yaml) { + // Prefer widen_timeout_seconds when the model set it: it's the + // narrower edit (only the wait window, not the step's wording), and + // is how a load-timing race — the target was right, the page just + // hadn't settled — gets fixed without risking a wrong-target rewrite. + let apply_result = match (&patch.step_yaml, patch.widen_timeout_seconds) { + (_, Some(seconds)) => flowproof_agent::widen_timeout(&raw, step_index, seconds), + (Some(step_yaml), None) => flowproof_agent::apply_patch(&raw, step_index, step_yaml), + (None, None) => Err(flowproof_agent::RepairError::BadModelOutput( + "model proposed neither a patch, a timeout widen, nor an engine-gap reason" + .to_string(), + )), + }; + let patched_yaml = match apply_result { Ok(yaml) => yaml, Err(e) => { attempts.push(flowproof_agent::RepairAttempt {