From 47be58eb39f9fc3c5938020bf399f4a3432cabab Mon Sep 17 00:00:00 2001 From: Fini Date: Fri, 24 Jul 2026 21:12:05 +0800 Subject: [PATCH] feat(agent): gate loop completion on unresolved blockers --- crates/op-ai/src/chat_provider.rs | 77 ++++++++ crates/op-editor-host-core/src/chat.rs | 37 +++- crates/op-host-desktop/src/chat_session.rs | 23 +++ .../op-host-services/src/chat_agent_loop.rs | 101 ++++++++-- .../src/chat_agent_loop_blockers.rs | 106 ++++++++++ .../src/chat_agent_loop_blockers_tests.rs | 187 ++++++++++++++++++ .../src/chat_agent_loop_tests.rs | 50 ++++- .../src/design_agent_tools.rs | 14 +- crates/op-host-services/src/lib.rs | 1 + .../src/loop_blocker_ledger.rs | 82 ++++++++ .../src/loop_blocker_ledger_tests.rs | 140 +++++++++++++ 11 files changed, 794 insertions(+), 24 deletions(-) create mode 100644 crates/op-host-services/src/chat_agent_loop_blockers.rs create mode 100644 crates/op-host-services/src/chat_agent_loop_blockers_tests.rs create mode 100644 crates/op-host-services/src/loop_blocker_ledger.rs create mode 100644 crates/op-host-services/src/loop_blocker_ledger_tests.rs diff --git a/crates/op-ai/src/chat_provider.rs b/crates/op-ai/src/chat_provider.rs index 957ec917e..4ea7a2255 100644 --- a/crates/op-ai/src/chat_provider.rs +++ b/crates/op-ai/src/chat_provider.rs @@ -362,6 +362,44 @@ pub struct ChatToolResult { /// [`ChatToolExecutor::finalize`]. pub const LOOP_FINALIZE_OP: &str = "__loop_finalize"; +/// Reserved pseudo-tool name a [`ChatToolExecutor`] forwards over its tool +/// channel to ask the host for a read-only unresolved-blocker scan against +/// the live `EditorState` — the completion-gate counterpart of +/// [`LOOP_FINALIZE_OP`]'s promise-delivery check. Never mutates the +/// document (mirrors the `checkOnly: true` half of `LOOP_FINALIZE_OP`, not +/// the finalize half). Never a real model-visible tool — the loop sends it +/// itself via [`ChatToolExecutor::check_blockers`]. +pub const CHECK_BLOCKERS_OP: &str = "__loop_check_blockers"; + +/// One unresolved structural blocker found by the host's live blocker scan +/// (`op_host_services::loop_blocker_ledger::detect_blockers`). `category` is +/// a coarse bucket (`"structure"` / `"empty-shell"` / `"nav"` today) for +/// grouping in a corrective message; `detail` is the same human-readable, +/// node/screen-identifying line the per-batch `structureIssues` / +/// `shellsRemaining` / `navIssues` tool-result fields already carry, so the +/// loop-end nudge reads exactly like the per-batch hints the model has +/// already been following all run. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct BlockerEntry { + pub category: String, + pub detail: String, +} + +/// Unresolved-blocker scan result. Deliberately NOT an accumulating ledger — +/// like [`UnfilledScreensReport`], this is always a fresh recompute against +/// the CURRENT document, so an issue a later batch already fixed simply +/// never appears again; there is nothing to prune or expire. +#[derive(Debug, Clone, Default, PartialEq, Eq)] +pub struct BlockerReport { + pub blockers: Vec, +} + +impl BlockerReport { + pub fn has_blockers(&self) -> bool { + !self.blockers.is_empty() + } +} + /// Promise-delivery snapshot: every top-level "screen" the run committed to /// (`committed`, filled or not) alongside the subset still empty (`unfilled`, /// always a subset of `committed`). Carries enough for the loop to build the @@ -408,6 +446,18 @@ pub trait ChatToolExecutor: Send + Sync { fn check_unfilled_screens(&self) -> UnfilledScreensReport { UnfilledScreensReport::default() } + + /// Cheap, read-only unresolved-blocker scan — the completion-gate + /// counterpart of [`Self::check_unfilled_screens`]. The loop calls this + /// whenever it's deciding whether to let a `calls.is_empty()` model stop + /// count as done: a document that still has an unbound primary-mobile + /// nav tab or a structural defect (duplicate status bar / broken ring / + /// empty shell) gets a corrective nudge instead of a silent finish. The + /// default no-op mirrors [`Self::check_unfilled_screens`]'s — only the + /// host executor that owns a live `EditorState` overrides it. + fn check_blockers(&self) -> BlockerReport { + BlockerReport::default() + } } /// Test double — replays a fixed delta script. Lets the chat widget @@ -430,6 +480,33 @@ impl ChatProvider for EchoProvider { mod tests { use super::*; + /// An executor that overrides NOTHING but the mandatory `execute` — the + /// shape of every host that hasn't wired up the unresolved-blocker scan + /// (web, any future non-desktop host). Anchors the zero-impact + /// requirement: an unwired host must behave EXACTLY as it did before + /// `check_blockers` existed, purely by inheriting the trait default. + struct BareExecutor; + impl ChatToolExecutor for BareExecutor { + fn execute(&self, _name: &str, _args_json: &str) -> ChatToolResult { + ChatToolResult { + content: "{}".into(), + is_error: false, + } + } + } + + #[test] + fn unwired_executor_check_blockers_default_is_empty_and_never_blocks() { + let report = BareExecutor.check_blockers(); + assert_eq!(report, BlockerReport::default()); + assert!( + !report.has_blockers(), + "an executor that never overrides check_blockers must report no blockers, \ + so the loop's completion gate takes the exact same path it did before \ + this feature existed" + ); + } + #[test] fn cli_name_backend_table_matches_architecture_memo() { // project_agent_runtime memory: diff --git a/crates/op-editor-host-core/src/chat.rs b/crates/op-editor-host-core/src/chat.rs index 4c578a06c..7cdcb68ab 100644 --- a/crates/op-editor-host-core/src/chat.rs +++ b/crates/op-editor-host-core/src/chat.rs @@ -6,8 +6,8 @@ use std::sync::Mutex; use std::thread; use op_ai::chat_provider::{ - ChatDelta, ChatHistoryRole, ChatProvider, ChatRequest, ChatToolExecutor, ChatToolResult, - UnfilledScreensReport, LOOP_FINALIZE_OP, + BlockerReport, ChatDelta, ChatHistoryRole, ChatProvider, ChatRequest, ChatToolExecutor, + ChatToolResult, UnfilledScreensReport, CHECK_BLOCKERS_OP, LOOP_FINALIZE_OP, }; use op_editor_core::{ChatMessage, ChatRole, ChatToolCall}; @@ -54,6 +54,14 @@ impl ChatToolExecutor for UiChatToolExecutor { fn check_unfilled_screens(&self) -> UnfilledScreensReport { unfilled_report_from_ack(&self.forward(LOOP_FINALIZE_OP, r#"{"checkOnly":true}"#)) } + + /// Forward the reserved [`CHECK_BLOCKERS_OP`] over the same tool + /// channel so the host runs a read-only unresolved-blocker scan + /// (`op_host_services::loop_blocker_ledger::detect_blockers`) against + /// the live `EditorState`. Never mutates the document. + fn check_blockers(&self) -> BlockerReport { + blocker_report_from_ack(&self.forward(CHECK_BLOCKERS_OP, "{}")) + } } /// Parse the `{"success":true,"committed":[...],"unfilled":[...]}` envelope @@ -78,6 +86,31 @@ fn unfilled_report_from_ack(result: &ChatToolResult) -> UnfilledScreensReport { } } +/// Parse the `{"success":true,"blockers":[{"category":...,"detail":...}]}` +/// envelope the host's `CHECK_BLOCKERS_OP` interception acks with. Any other +/// shape (transport error, an old host build that doesn't know this op) +/// degrades to an empty report — same best-effort discipline as +/// [`unfilled_report_from_ack`]. +fn blocker_report_from_ack(result: &ChatToolResult) -> BlockerReport { + let Ok(v) = serde_json::from_str::(&result.content) else { + return BlockerReport::default(); + }; + let blockers = v + .get("blockers") + .and_then(|v| v.as_array()) + .map(|arr| { + arr.iter() + .filter_map(|entry| { + let category = entry.get("category")?.as_str()?.to_string(); + let detail = entry.get("detail")?.as_str()?.to_string(); + Some(op_ai::chat_provider::BlockerEntry { category, detail }) + }) + .collect() + }) + .unwrap_or_default(); + BlockerReport { blockers } +} + impl UiChatToolExecutor { /// Send one request over the tool channel and block until the host acks. fn forward(&self, name: &str, args_json: &str) -> ChatToolResult { diff --git a/crates/op-host-desktop/src/chat_session.rs b/crates/op-host-desktop/src/chat_session.rs index 2be640128..3e9202f6e 100644 --- a/crates/op-host-desktop/src/chat_session.rs +++ b/crates/op-host-desktop/src/chat_session.rs @@ -293,6 +293,29 @@ fn execute_tool_requests( }); continue; } + // Intercept the reserved unresolved-blocker scan op: the completion + // gate counterpart of `LOOP_FINALIZE_OP`'s `checkOnly` half. Always + // read-only — a live recompute against the current document, never + // an accumulating ledger (see + // `op_host_services::loop_blocker_ledger`'s module doc) — so an + // issue a later batch already fixed simply stops appearing here. + if req.name == op_ai::chat_provider::CHECK_BLOCKERS_OP { + let report = op_host_services::loop_blocker_ledger::detect_blockers(state); + let blockers: Vec = report + .blockers + .iter() + .map(|b| serde_json::json!({ "category": b.category, "detail": b.detail })) + .collect(); + let _ = req.ack.send(ChatToolResult { + content: serde_json::json!({ + "success": true, + "blockers": blockers, + }) + .to_string(), + is_error: false, + }); + continue; + } // Intercept `spawn_agents`: parse the specs, stash them for the // host to launch after this (parent) pump, and ack immediately // (fire-and-forget). A SUB calling `spawn_agents` is refused — diff --git a/crates/op-host-services/src/chat_agent_loop.rs b/crates/op-host-services/src/chat_agent_loop.rs index b58338869..8e709e818 100644 --- a/crates/op-host-services/src/chat_agent_loop.rs +++ b/crates/op-host-services/src/chat_agent_loop.rs @@ -30,6 +30,10 @@ use retry::{ CORRECTIVE_WRITE_PROGRESS, }; +#[path = "chat_agent_loop_blockers.rs"] +mod blockers; +use blockers::{blocker_nudge_if_owed, report_blockers_if_any}; + /// Everything one agent-loop run needs. `max_turns` is the TS `maxTurns` /// cap — `MAX_TOOL_TURNS = 20` for plain chat, `DESIGN_LOOP_MAX_TURNS = 28` /// for the gated design-generation loop (`chat_builtin_http.rs`; the two @@ -269,6 +273,26 @@ async fn report_unfilled_if_any(tx: &mpsc::Sender, names: &[String]) let _ = tx.send(ChatDelta::TextDelta(text)).await; } +/// Shared finalize + honest-report tail for every loop exit tier (turn-cap +/// salvage-exhausted, salvage-nothing-eligible, and the ordinary +/// voluntary-model-stop path) — replaces what used to be three near-identical +/// `run_loop_finalize` + `report_unfilled_if_any` call sites per provider +/// loop, and folds the unresolved-blocker report into the SAME tail so every +/// exit unconditionally surfaces both: a run that spent its whole turn +/// budget with blockers still open must never look, in the transcript, like +/// a run that had nothing wrong with it. +async fn finalize_and_report( + tx: &mpsc::Sender, + executor: &Arc, + enabled: bool, +) -> UnfilledScreensReport { + let still_unfilled = run_loop_finalize(executor, enabled).await; + report_unfilled_if_any(tx, &still_unfilled.unfilled).await; + let blocker_report = blockers::check_blockers(executor, enabled).await; + report_blockers_if_any(tx, &blocker_report).await; + still_unfilled +} + /// Self-diagnostic signal for the finalize-lifecycle invariant (0718-1-k3-1 /// postmortem) — a `run_anthropic_agent_loop` / `run_openai_agent_loop` /// outer wrapper's backstop just ran [`run_loop_finalize`] on an `Err` exit @@ -606,6 +630,11 @@ async fn run_anthropic_agent_loop_inner( // `fill_attempts` above (see `SALVAGE_MAX_ROUNDS`'s doc comment). let mut salvaged_screens: std::collections::HashSet = std::collections::HashSet::new(); let mut salvage_rounds_used = 0usize; + // Tier 2c — unresolved-blocker corrective-round budget. A SEPARATE pool + // from both of the above (see `chat_agent_loop_blockers:: + // BLOCKER_NUDGE_MAX_ROUNDS`'s doc comment): blockers and unfilled + // screens are different failure modes with independent budgets. + let mut blocker_rounds_used = 0usize; let mut write_retry = CorrectiveWriteRetry::default(); let turn_cap = cfg.max_turns.max(1); let mut turn = 0usize; @@ -620,8 +649,7 @@ async fn run_anthropic_agent_loop_inner( // against `turn`; it draws from `SALVAGE_MAX_ROUNDS` instead. if turn >= turn_cap && !corrective_write_round { if salvage_rounds_used >= SALVAGE_MAX_ROUNDS { - let still_unfilled = run_loop_finalize(&cfg.executor, cfg.finalize_on_exit).await; - report_unfilled_if_any(tx, &still_unfilled.unfilled).await; + finalize_and_report(tx, &cfg.executor, cfg.finalize_on_exit).await; let _ = tx .send(ChatDelta::Done { stop_reason: StopReason::MaxTokens, @@ -637,8 +665,7 @@ async fn run_anthropic_agent_loop_inner( // unfilled screen already spent its one salvage round. Still // run the Step-4 structural backstop once over whatever the // run assembled, and report unconditionally. - let still_unfilled = run_loop_finalize(&cfg.executor, cfg.finalize_on_exit).await; - report_unfilled_if_any(tx, &still_unfilled.unfilled).await; + finalize_and_report(tx, &cfg.executor, cfg.finalize_on_exit).await; let _ = tx .send(ChatDelta::Done { stop_reason: StopReason::MaxTokens, @@ -734,15 +761,32 @@ async fn run_anthropic_agent_loop_inner( ); continue; // Does not count against `turn`. } + // No screen left to chase — try one dedicated corrective round + // for any unresolved structural blocker (duplicate root / broken + // ring / empty shell / unbound primary-mobile nav tab), bounded + // by its own `BLOCKER_NUDGE_MAX_ROUNDS` budget so this can never + // compound into an unbounded loop alongside the fill/salvage + // budgets above. + if let Some(nudge) = blocker_nudge_if_owed( + &cfg.executor, + cfg.finalize_on_exit, + &mut blocker_rounds_used, + ) + .await + { + messages + .push(json!({ "role": "assistant", "content": collector.assistant_content() })); + messages.push(json!({ "role": "user", "content": nudge })); + continue; // Does not count against `turn`. + } // Nothing committed is left worth trying for: run the Step-4 // structural backstop ONCE over the assembled doc BEFORE the // Done delta, so the finalized document is what the UI // persists/displays for this turn. Tier 3 — honest report — - // fires unconditionally if anything is still unfilled (a screen - // outside this run's committed set, or the executor being a - // no-op). - let still_unfilled = run_loop_finalize(&cfg.executor, cfg.finalize_on_exit).await; - report_unfilled_if_any(tx, &still_unfilled.unfilled).await; + // fires unconditionally if anything is still unfilled/blocked (a + // screen outside this run's committed set, a blocker whose + // round budget ran out, or the executor being a no-op). + finalize_and_report(tx, &cfg.executor, cfg.finalize_on_exit).await; let _ = tx .send(ChatDelta::Done { stop_reason: reason, @@ -997,6 +1041,11 @@ async fn run_openai_agent_loop_inner( let mut fill_attempts: HashMap = HashMap::new(); let mut salvaged_screens: std::collections::HashSet = std::collections::HashSet::new(); let mut salvage_rounds_used = 0usize; + // Tier 2c — unresolved-blocker corrective-round budget. A SEPARATE pool + // from both of the above (see `chat_agent_loop_blockers:: + // BLOCKER_NUDGE_MAX_ROUNDS`'s doc comment): blockers and unfilled + // screens are different failure modes with independent budgets. + let mut blocker_rounds_used = 0usize; let mut write_retry = CorrectiveWriteRetry::default(); let turn_cap = cfg.max_turns.max(1); let mut turn = 0usize; @@ -1006,8 +1055,7 @@ async fn run_openai_agent_loop_inner( let corrective_write_round = write_retry.begin_round(); if turn >= turn_cap && !corrective_write_round { if salvage_rounds_used >= SALVAGE_MAX_ROUNDS { - let still_unfilled = run_loop_finalize(&cfg.executor, cfg.finalize_on_exit).await; - report_unfilled_if_any(tx, &still_unfilled.unfilled).await; + finalize_and_report(tx, &cfg.executor, cfg.finalize_on_exit).await; let _ = tx .send(ChatDelta::Done { stop_reason: StopReason::MaxTokens, @@ -1018,8 +1066,7 @@ async fn run_openai_agent_loop_inner( let report = check_unfilled(&cfg.executor, cfg.finalize_on_exit).await; let eligible = salvage_eligible(&report.unfilled, &salvaged_screens); if eligible.is_empty() { - let still_unfilled = run_loop_finalize(&cfg.executor, cfg.finalize_on_exit).await; - report_unfilled_if_any(tx, &still_unfilled.unfilled).await; + finalize_and_report(tx, &cfg.executor, cfg.finalize_on_exit).await; let _ = tx .send(ChatDelta::Done { stop_reason: StopReason::MaxTokens, @@ -1119,11 +1166,26 @@ async fn run_openai_agent_loop_inner( ); continue; // Does not count against `turn`. } + // No screen left to chase — try one dedicated corrective round + // for any unresolved structural blocker, bounded by its own + // `BLOCKER_NUDGE_MAX_ROUNDS` budget (see the Anthropic loop + // above for the full rationale). + if let Some(nudge) = blocker_nudge_if_owed( + &cfg.executor, + cfg.finalize_on_exit, + &mut blocker_rounds_used, + ) + .await + { + messages.push(json!({ "role": "assistant", "content": stop_content() })); + messages.push(json!({ "role": "user", "content": nudge })); + continue; // Does not count against `turn`. + } // Normal model-stop exit: run the Step-4 structural backstop ONCE // over the assembled doc BEFORE the Done delta. Tier 3 — honest - // report — fires unconditionally if anything is still unfilled. - let still_unfilled = run_loop_finalize(&cfg.executor, cfg.finalize_on_exit).await; - report_unfilled_if_any(tx, &still_unfilled.unfilled).await; + // report — fires unconditionally if anything is still + // unfilled/blocked. + finalize_and_report(tx, &cfg.executor, cfg.finalize_on_exit).await; let _ = tx .send(ChatDelta::Done { stop_reason: reason, @@ -1249,6 +1311,13 @@ mod tests; #[path = "chat_agent_loop_finalize_tests.rs"] mod finalize_tests; +// Unresolved-blocker completion-gate tests — same split rationale as +// `finalize_tests` above, reusing `chat_agent_loop_tests.rs`'s scripted- +// executor + loopback-SSE infra via `pub(super)`. +#[cfg(test)] +#[path = "chat_agent_loop_blockers_tests.rs"] +mod blockers_tests; + #[cfg(test)] #[path = "chat_agent_loop_retry_tests.rs"] mod retry_tests; diff --git a/crates/op-host-services/src/chat_agent_loop_blockers.rs b/crates/op-host-services/src/chat_agent_loop_blockers.rs new file mode 100644 index 000000000..81e121db2 --- /dev/null +++ b/crates/op-host-services/src/chat_agent_loop_blockers.rs @@ -0,0 +1,106 @@ +//! Unresolved-blocker completion gate — shared between the Anthropic and +//! OpenAI-compatible agent loops in `chat_agent_loop.rs` (split out as a +//! sibling module the same way `chat_agent_loop_retry.rs` splits out the +//! corrective-write retry policy, so neither loop function duplicates this +//! logic). See `op_host_services::loop_blocker_ledger`'s module doc for what +//! counts as a blocker (structure / empty-shell / nav) and why this is a +//! live recompute against the document rather than an accumulating ledger. + +use std::sync::Arc; + +use op_ai::chat_provider::{BlockerReport, ChatDelta, ChatToolExecutor}; +use tokio::sync::mpsc; + +/// Per-run budget for the unresolved-blocker corrective round — a SEPARATE +/// pool from `FILL_BUDGET_MAX_ROUNDS_PER_SCREEN` / `SALVAGE_MAX_ROUNDS` +/// (blockers and unfilled screens are different failure modes; see those +/// constants' doc comments in `chat_agent_loop.rs` for "budget guards +/// runaway retry, never truncates promised work"). Blockers are a +/// narrower, already-detected class — the model already saw each one as a +/// per-batch tool-result hint (`structureIssues` / `shellsRemaining` / +/// `navIssues`) — so a flat per-run cap (not a per-item pool like the fill +/// budget) is enough to give the model a real chance to react without room +/// for an avalanche. +pub(super) const BLOCKER_NUDGE_MAX_ROUNDS: usize = 2; + +/// Cheap, read-only unresolved-blocker scan. Gated the same way +/// `check_unfilled`/`run_loop_finalize` are: `enabled` mirrors +/// `cfg.finalize_on_exit`, so a plain (non-design) chat turn never touches +/// the document. +pub(super) async fn check_blockers( + executor: &Arc, + enabled: bool, +) -> BlockerReport { + if !enabled { + return BlockerReport::default(); + } + let executor = executor.clone(); + tokio::task::spawn_blocking(move || executor.check_blockers()) + .await + .unwrap_or_default() +} + +/// Corrective-message contract text for a still-blocked loop completion — +/// mirrors `contract_nudge_text`'s "state the full commitment, not just +/// what's missing" shape, but for structural blockers instead of unfilled +/// screens. +pub(super) fn blocker_nudge_text(report: &BlockerReport) -> String { + let lines: Vec = report + .blockers + .iter() + .map(|b| format!("- [{}] {}", b.category, b.detail)) + .collect(); + format!( + "The design still has {} unresolved blocker(s) that must be fixed before finishing:\n{}", + lines.len(), + lines.join("\n") + ) +} + +/// Decide whether this `calls.is_empty()` model-stop is owed one more +/// corrective round for unresolved blockers — mirrors +/// `eligible_for_fill_round`'s per-screen budget check, but against a flat +/// per-run round counter instead of a per-screen map (blockers aren't keyed +/// by screen name the way unfilled screens are). Spends a round from +/// `rounds_used` when it returns `Some`; the caller pushes the text as a +/// user message and `continue`s WITHOUT counting it against the ordinary +/// turn budget, exactly like the fill round. +pub(super) async fn blocker_nudge_if_owed( + executor: &Arc, + enabled: bool, + rounds_used: &mut usize, +) -> Option { + if !enabled || *rounds_used >= BLOCKER_NUDGE_MAX_ROUNDS { + return None; + } + let report = check_blockers(executor, enabled).await; + if !report.has_blockers() { + return None; + } + *rounds_used += 1; + Some(blocker_nudge_text(&report)) +} + +/// Tier-3 unconditional honest report for blockers — appended right +/// alongside `report_unfilled_if_any`'s screen line whenever the loop is +/// about to send `Done` with blockers still unresolved (round budget +/// spent, or this exit tier never spent a round at all — e.g. the +/// turn-cap/salvage tiers, which only nudge for unfilled screens, not +/// blockers). Never silently succeeds: any blocker still present at ANY +/// exit tier surfaces here. +pub(super) async fn report_blockers_if_any(tx: &mpsc::Sender, report: &BlockerReport) { + if !report.has_blockers() { + return; + } + let text = format!( + "\n\n• {} unresolved blocker(s): {}", + report.blockers.len(), + report + .blockers + .iter() + .map(|b| b.detail.as_str()) + .collect::>() + .join("; ") + ); + let _ = tx.send(ChatDelta::TextDelta(text)).await; +} diff --git a/crates/op-host-services/src/chat_agent_loop_blockers_tests.rs b/crates/op-host-services/src/chat_agent_loop_blockers_tests.rs new file mode 100644 index 000000000..d396de36b --- /dev/null +++ b/crates/op-host-services/src/chat_agent_loop_blockers_tests.rs @@ -0,0 +1,187 @@ +//! Unresolved-blocker completion-gate tests — mirrors the "承诺-交付" +//! fill-round tests in `chat_agent_loop_tests.rs` (search for that banner), +//! but for structural blockers (`check_blockers`) instead of unfilled +//! screens (`check_unfilled_screens`). Three tiers: no blocker → unaffected; +//! blocker present with round budget left → one corrective nudge, then +//! completes once fixed; blocker persists past `BLOCKER_NUDGE_MAX_ROUNDS` → +//! never silently succeeds, the honest report still lands. + +use super::tests::{run_loop_collect, serve_sse_script, update_node_tool_def, ScriptedExecutor}; +use super::*; + +fn anthropic_text_turn() -> String { + [ + r#"data: {"type":"content_block_start","index":0,"content_block":{"type":"text","text":""}}"#, + "", + r#"data: {"type":"content_block_delta","index":0,"delta":{"type":"text_delta","text":"Done."}}"#, + "", + r#"data: {"type":"message_delta","delta":{"stop_reason":"end_turn"}}"#, + "", + r#"data: {"type":"message_stop"}"#, + "", + "", + ] + .join("\n") +} + +fn base_cfg( + base: &str, + executor: std::sync::Arc, + max_turns: usize, +) -> AgentLoopConfig { + AgentLoopConfig { + url: format!("{base}/v1/messages"), + api_key: "sk-test".into(), + model: "claude-test".into(), + system_prompt: String::new(), + history: Vec::new(), + user_prompt: "build the app".into(), + max_output_tokens: 512, + tools: vec![update_node_tool_def()], + executor, + max_turns, + finalize_on_exit: true, + disable_thinking: false, + dial_policy: crate::provider_dial::EndpointDialPolicy::Trusted, + } +} + +#[test] +fn no_blockers_completes_without_nudge_or_report() { + // Nothing scripted for `check_blockers` — the default (empty) report — + // must behave EXACTLY like today: straight to finalize + Done, no + // corrective round, no "unresolved blocker" line anywhere. + let (base, req_rx) = serve_sse_script(vec![anthropic_text_turn()]); + let executor = ScriptedExecutor::ok(r#"{"success":true,"data":{}}"#); + let cfg = base_cfg(&base, executor.clone(), 5); + let (outcome, deltas) = run_loop_collect(cfg, true); + assert_eq!(outcome, Ok(true)); + + // Exactly one request went out — no corrective round was injected. + let _first = req_rx.recv().expect("first request captured"); + assert!( + req_rx.recv().is_err(), + "no follow-up request when there is nothing to nudge about" + ); + assert_eq!(executor.finalizes(), 1); + assert!( + !deltas + .iter() + .any(|d| matches!(d, ChatDelta::TextDelta(s) if s.contains("unresolved blocker"))), + "a clean run must never print a blocker report: {deltas:?}" + ); + assert!(matches!( + deltas.last(), + Some(ChatDelta::Done { + stop_reason: StopReason::EndTurn + }) + )); +} + +#[test] +fn blocker_present_gets_one_corrective_round_then_completes_once_fixed() { + // Turn 1: model stops with a structural blocker still on the canvas — + // the loop injects one corrective round instead of finalizing + // immediately. Turn 2: model stops again; this time the scan reports + // nothing left — straight to finalize, which also finds nothing. + let (base, req_rx) = serve_sse_script(vec![anthropic_text_turn(), anthropic_text_turn()]); + let executor = ScriptedExecutor::ok(r#"{"success":true,"data":{}}"#) + .with_blocker_check(&[("nav", "tab-profile is not bound to events.onTap yet")]) + .with_blocker_check(&[]); + let cfg = base_cfg(&base, executor.clone(), 5); + let (outcome, deltas) = run_loop_collect(cfg, true); + assert_eq!(outcome, Ok(true)); + + // 3 checks total: the nudge round's probe (finds it), the nudge round's + // probe on the NEXT pass (confirms it's fixed), and the final tail + // check inside `finalize_and_report` (also confirms nothing left). + assert_eq!(executor.blocker_checks(), 3); + assert_eq!( + executor.finalizes(), + 1, + "finalize still runs exactly once, at the real exit" + ); + + // The corrective round's contract line actually rode the follow-up + // request as a real turn. + let _first = req_rx.recv().expect("first request captured"); + let second = req_rx + .recv() + .expect("corrective-round follow-up request captured"); + assert!( + second.contains("unresolved blocker") + && second.contains("[nav] tab-profile is not bound to events.onTap yet"), + "the corrective round must name the exact blocker, got: {second}" + ); + + // Nothing left unresolved after the corrective round → no tier-3 report. + assert!( + !deltas + .iter() + .any(|d| matches!(d, ChatDelta::TextDelta(s) if s.contains("unresolved blocker"))), + "a blocker the corrective round fixed must not also be reported: {deltas:?}" + ); + assert!(matches!( + deltas.last(), + Some(ChatDelta::Done { + stop_reason: StopReason::EndTurn + }) + )); +} + +#[test] +fn blocker_persisting_past_round_budget_still_reports_needs_attention() { + // The model voluntarily stops 3 times in a row and the SAME blocker is + // still present every time. `BLOCKER_NUDGE_MAX_ROUNDS` (2) must stop the + // corrective nudging after 2 rounds — the 3rd stop must NOT spend a + // 3rd nudge round, but the run must still end with an honest report, + // never a silent success. + let (base, req_rx) = serve_sse_script(vec![ + anthropic_text_turn(), + anthropic_text_turn(), + anthropic_text_turn(), + ]); + let executor = ScriptedExecutor::ok(r#"{"success":true,"data":{}}"#) + .with_blocker_check(&[("structure", "duplicate Explore root r1/r2")]) + .with_blocker_check(&[("structure", "duplicate Explore root r1/r2")]) + .with_blocker_check(&[("structure", "duplicate Explore root r1/r2")]); + // Turn budget is generous — neither nudge round counts against `turn`, + // so ONLY the round budget, not the ordinary turn cap, ends this. + let cfg = base_cfg(&base, executor.clone(), 5); + let (outcome, deltas) = run_loop_collect(cfg, true); + assert_eq!(outcome, Ok(true)); + + // 3 checks: round 1 (nudge), round 2 (nudge), round 3 (budget spent — + // the gate short-circuits before probing again for the NUDGE decision, + // but the final tail check inside `finalize_and_report` still runs + // once more) — so exactly 3 scripted values get consumed. + assert_eq!(executor.blocker_checks(), 3); + assert_eq!(executor.finalizes(), 1); + + // Only 2 corrective requests went out (round 1 and round 2); the 3rd + // model stop goes straight to finalize+Done — no 3rd nudge request. + let _first = req_rx.recv().expect("initial request"); + let second = req_rx.recv().expect("round 1 corrective request"); + assert!(second.contains("unresolved blocker")); + let third = req_rx.recv().expect("round 2 corrective request"); + assert!(third.contains("unresolved blocker")); + assert!( + req_rx.recv().is_err(), + "the round budget must stop a 3rd corrective request from going out" + ); + + let report = deltas.iter().find_map(|d| match d { + ChatDelta::TextDelta(s) if s.contains("unresolved blocker") => Some(s.clone()), + _ => None, + }); + assert!( + report.is_some_and(|s| s.contains("duplicate Explore root r1/r2")), + "a blocker still present after exhausting its round budget must be reported, not silently dropped: {deltas:?}" + ); + assert!(matches!( + deltas.last(), + Some(ChatDelta::Done { + stop_reason: StopReason::EndTurn + }) + )); +} diff --git a/crates/op-host-services/src/chat_agent_loop_tests.rs b/crates/op-host-services/src/chat_agent_loop_tests.rs index 99a9d2d86..12f4226c2 100644 --- a/crates/op-host-services/src/chat_agent_loop_tests.rs +++ b/crates/op-host-services/src/chat_agent_loop_tests.rs @@ -14,7 +14,10 @@ use std::time::Duration; use super::*; use crate::chat_runtime::shared_runtime; use base64::Engine as _; -use op_ai::chat_provider::{ChatToolDef, ChatToolExecutor, ChatToolResult, UnfilledScreensReport}; +use op_ai::chat_provider::{ + BlockerEntry, BlockerReport, ChatToolDef, ChatToolExecutor, ChatToolResult, + UnfilledScreensReport, +}; /// Executor double — records calls + loop-finalize invocations, replays a /// fixed result. @@ -29,6 +32,10 @@ pub(super) struct ScriptedExecutor { unfilled_checks: Mutex>, /// Scripted `finalize` return values, popped the same way. unfilled_finalizes: Mutex>, + /// Scripted `check_blockers` return values, popped in call order; once + /// exhausted, further calls return the default (empty) report. + blocker_checks: Mutex>, + blocker_check_calls: std::sync::atomic::AtomicUsize, } impl ScriptedExecutor { @@ -52,6 +59,8 @@ impl ScriptedExecutor { ), unfilled_checks: Mutex::new(VecDeque::new()), unfilled_finalizes: Mutex::new(VecDeque::new()), + blocker_checks: Mutex::new(VecDeque::new()), + blocker_check_calls: std::sync::atomic::AtomicUsize::new(0), }) } @@ -89,6 +98,23 @@ impl ScriptedExecutor { .push_back(report_of(committed, unfilled)); self } + + /// Queue one scripted `check_blockers` return value. `entries` is + /// `(category, detail)` pairs. + pub(super) fn with_blocker_check(self: Arc, entries: &[(&str, &str)]) -> Arc { + self.blocker_checks + .lock() + .unwrap() + .push_back(blocker_report_of(entries)); + self + } + + /// How many times the loop ran the cheap read-only unresolved-blocker + /// probe (`check_blockers`). + pub(super) fn blocker_checks(&self) -> usize { + self.blocker_check_calls + .load(std::sync::atomic::Ordering::SeqCst) + } } fn report_of(committed: &[&str], unfilled: &[&str]) -> UnfilledScreensReport { @@ -98,6 +124,18 @@ fn report_of(committed: &[&str], unfilled: &[&str]) -> UnfilledScreensReport { } } +fn blocker_report_of(entries: &[(&str, &str)]) -> BlockerReport { + BlockerReport { + blockers: entries + .iter() + .map(|(category, detail)| BlockerEntry { + category: category.to_string(), + detail: detail.to_string(), + }) + .collect(), + } +} + impl ChatToolExecutor for ScriptedExecutor { fn execute(&self, name: &str, args_json: &str) -> ChatToolResult { self.calls @@ -131,6 +169,16 @@ impl ChatToolExecutor for ScriptedExecutor { .pop_front() .unwrap_or_default() } + + fn check_blockers(&self) -> BlockerReport { + self.blocker_check_calls + .fetch_add(1, std::sync::atomic::Ordering::SeqCst); + self.blocker_checks + .lock() + .unwrap() + .pop_front() + .unwrap_or_default() + } } fn read_http_request(stream: &mut TcpStream) -> String { diff --git a/crates/op-host-services/src/design_agent_tools.rs b/crates/op-host-services/src/design_agent_tools.rs index f7e7f74b1..6e8aac90a 100644 --- a/crates/op-host-services/src/design_agent_tools.rs +++ b/crates/op-host-services/src/design_agent_tools.rs @@ -924,7 +924,7 @@ const CONTRAST_ICON_TARGET: f64 = 3.0; /// empty AppContent and built everything in a second `Explore` — the user /// sees a blank artboard mid-run). Finalize's duplicate-root pass repairs /// the END state, but the in-loop model should merge NOW. -fn scan_duplicate_root_issues(nodes: &[PenNode]) -> Vec { +pub(crate) fn scan_duplicate_root_issues(nodes: &[PenNode]) -> Vec { use std::collections::HashMap; let mut by_name: HashMap<&str, Vec<&PenNode>> = HashMap::new(); for node in nodes { @@ -974,7 +974,7 @@ fn scan_duplicate_root_issues(nodes: &[PenNode]) -> Vec { /// as a SIBLING — the bell floats alone in a full-width strip above the /// greeting (measured: "Header Row" = [bell], "Good evening" outside it, /// test0711-22). Which text belongs in the row is intent, so this echoes. -fn scan_header_icon_row_issues(nodes: &[PenNode]) -> Vec { +pub(crate) fn scan_header_icon_row_issues(nodes: &[PenNode]) -> Vec { let mut out = Vec::new(); fn walk(nodes: &[PenNode], out: &mut Vec) { for node in nodes { @@ -1012,7 +1012,7 @@ fn scan_header_icon_row_issues(nodes: &[PenNode]) -> Vec { out } -fn scan_empty_shells(nodes: &[PenNode]) -> Vec { +pub(crate) fn scan_empty_shells(nodes: &[PenNode]) -> Vec { let mut out = Vec::new(); fn walk(nodes: &[PenNode], out: &mut Vec) { for node in nodes { @@ -1025,7 +1025,11 @@ fn scan_empty_shells(nodes: &[PenNode]) -> Vec { && !named.is_empty() && node.base().role.as_deref() != Some("status-bar") { - out.push(named.to_string()); + // Carry the node id alongside the name (matches the other + // structural scans' shape) so a loop-end corrective nudge + // can name a specific, D()-able / M()-able target instead + // of a possibly-ambiguous name alone. + out.push(format!("{named} ({})", node.id_str())); } else { walk(children, out); } @@ -1036,7 +1040,7 @@ fn scan_empty_shells(nodes: &[PenNode]) -> Vec { out } -fn scan_ring_issues(nodes: &[PenNode]) -> Vec { +pub(crate) fn scan_ring_issues(nodes: &[PenNode]) -> Vec { const MIN_RING_SIZE: f64 = 48.0; const HAIRLINE: f32 = 2.5; let mut out = op_design_lint::detect_missing_progress_rings(nodes) diff --git a/crates/op-host-services/src/lib.rs b/crates/op-host-services/src/lib.rs index 25c9e30f4..398e4f87e 100644 --- a/crates/op-host-services/src/lib.rs +++ b/crates/op-host-services/src/lib.rs @@ -59,6 +59,7 @@ pub mod export; pub mod export_pdf; mod figma_convert; mod import_html_url; +pub mod loop_blocker_ledger; pub mod mcp_live; pub mod mcp_serve; pub mod model_discovery; diff --git a/crates/op-host-services/src/loop_blocker_ledger.rs b/crates/op-host-services/src/loop_blocker_ledger.rs new file mode 100644 index 000000000..79823ab28 --- /dev/null +++ b/crates/op-host-services/src/loop_blocker_ledger.rs @@ -0,0 +1,82 @@ +//! Unresolved-blocker detection for agent-loop completion gating. +//! +//! [`detect_blockers`] is a LIVE recompute against the current `EditorState` +//! — deliberately NOT an accumulating ledger populated incrementally by +//! `design_agent_tools.rs`'s per-`batch_design` diagnostics. It mirrors the +//! discipline `op_orchestrator::unfilled_screens::detect_unfilled_screens` +//! already established for `ChatToolExecutor::check_unfilled_screens`: run +//! the same scans fresh every time, straight off the live document, so an +//! issue a later batch already fixed simply stops appearing — nothing to +//! prune, nothing that can go stale. +//! +//! Scope (MVP, conservative): only failure modes with an unambiguous +//! structural signal count as blockers — +//! +//! - **structure** — duplicate status bars / broken rings / duplicate +//! header-icon rows (`design_agent_tools::scan_duplicate_root_issues` / +//! `scan_ring_issues` / `scan_header_icon_row_issues` — the same scans +//! whose hits ride into `batch_design`'s `structureIssues` field). +//! - **empty-shell** — a scaffolded section never filled +//! (`design_agent_tools::scan_empty_shells`, `shellsRemaining` per-batch). +//! - **nav** — an unbound primary-mobile-screen nav tab +//! (`op_orchestrator::nav_issues::scan_nav_issues`, `navIssues` per-batch). +//! +//! `layoutIssues` (`op_orchestrator::geometry_validation::geometry_diagnostics`) +//! is deliberately EXCLUDED from this MVP: its ~15 detectors (overflow / +//! collapse / jam / starvation / spill / …) all return the same flat +//! `Vec` with no structured category field, so classifying "which of +//! these are overflow/collapse" would mean pattern-matching free-form +//! message text — exactly the name/string-heuristic fragility this codebase +//! has been burned by before. `layoutIssues` stays advisory (it still rides +//! in the per-batch tool result, unaffected by this module) until +//! `geometry_validation` grows a real category enum on its diagnostics. + +use op_ai::chat_provider::{BlockerEntry, BlockerReport}; +use op_editor_core::EditorState; + +use crate::design_agent_tools::{ + scan_duplicate_root_issues, scan_empty_shells, scan_header_icon_row_issues, scan_ring_issues, +}; + +/// Recompute every unresolved blocker against the CURRENT active page of +/// `state` — same scope `design_agent_tools`'s per-batch diagnostics use. +/// Read-only: never mutates the document. +pub fn detect_blockers(state: &EditorState) -> BlockerReport { + let children = state.active_children(); + let mut blockers = Vec::new(); + for detail in scan_duplicate_root_issues(children) { + blockers.push(BlockerEntry { + category: "structure".to_string(), + detail, + }); + } + for detail in scan_ring_issues(children) { + blockers.push(BlockerEntry { + category: "structure".to_string(), + detail, + }); + } + for detail in scan_header_icon_row_issues(children) { + blockers.push(BlockerEntry { + category: "structure".to_string(), + detail, + }); + } + for detail in scan_empty_shells(children) { + blockers.push(BlockerEntry { + category: "empty-shell".to_string(), + detail, + }); + } + for detail in op_orchestrator::nav_issues::scan_nav_issues(state) { + blockers.push(BlockerEntry { + category: "nav".to_string(), + detail, + }); + } + BlockerReport { blockers } +} + +#[cfg(test)] +#[path = "loop_blocker_ledger_tests.rs"] +mod tests; diff --git a/crates/op-host-services/src/loop_blocker_ledger_tests.rs b/crates/op-host-services/src/loop_blocker_ledger_tests.rs new file mode 100644 index 000000000..81ba1a080 --- /dev/null +++ b/crates/op-host-services/src/loop_blocker_ledger_tests.rs @@ -0,0 +1,140 @@ +//! Tests for the unresolved-blocker completion-gate scan — see +//! `loop_blocker_ledger.rs` module doc for scope (structure / empty-shell / +//! nav are blockers; layoutIssues stays advisory and out of scope here). + +use super::*; +use jian_ops_schema::PenDocument; +use op_editor_core::pen_node_ext::PenNodeExt; + +fn state_from_json(json: &str) -> EditorState { + let doc: PenDocument = serde_json::from_str(json).expect("valid PenDocument"); + EditorState::from_document(doc) +} + +#[test] +fn clean_document_has_no_blockers() { + let state = state_from_json( + r##"{ "version": "1.0", "children": [ + { "type": "frame", "id": "home", "name": "Home", "width": 390, "height": 844, + "children": [ { "type": "text", "id": "t1", "content": "Hello" } ] } + ] }"##, + ); + let report = detect_blockers(&state); + assert!(!report.has_blockers(), "{report:?}"); +} + +#[test] +fn duplicate_top_level_frame_is_a_structure_blocker() { + // Same test0711-1-m3.op shape `duplicate_root_tests` covers in + // design_agent_tools.rs: model abandoned the first `Explore` and + // rebuilt everything in a second one of the same name. + let state = state_from_json( + r##"{ "version": "1.0", "children": [ + { "type": "frame", "id": "r1", "name": "Explore", "width": 390, "height": 844, + "children": [ { "type": "frame", "id": "empty", "name": "AppContent", + "width": "fill_container", "height": "fit_content" } ] }, + { "type": "frame", "id": "r2", "name": "Explore", "width": 390, + "height": "fit_content", + "children": [ { "type": "frame", "id": "rich", "name": "AppContent", + "width": "fill_container", "height": "fit_content" } ] } + ] }"##, + ); + let report = detect_blockers(&state); + assert!(report.has_blockers()); + let hit = report + .blockers + .iter() + .find(|b| b.category == "structure") + .expect("a structure blocker"); + assert!( + hit.detail.contains("Explore") && hit.detail.contains("r1") && hit.detail.contains("r2") + ); +} + +#[test] +fn empty_named_shell_is_an_empty_shell_blocker() { + let state = state_from_json( + r##"{ "version": "1.0", "children": [ + { "type": "frame", "id": "home", "name": "Home", "width": 390, "height": 844, + "children": [ + { "type": "frame", "id": "section", "name": "RecentActivity", + "width": "fill_container", "height": "fit_content", "children": [] } + ] } + ] }"##, + ); + let report = detect_blockers(&state); + assert!(report.has_blockers()); + let hit = report + .blockers + .iter() + .find(|b| b.category == "empty-shell") + .expect("an empty-shell blocker"); + // Must carry a concrete, locatable node id alongside the name — a + // corrective nudge built from name alone could be ambiguous. + assert!(hit.detail.contains("RecentActivity"), "{hit:?}"); + assert!(hit.detail.contains("section"), "{hit:?}"); +} + +#[test] +fn unbound_nav_tab_is_a_nav_blocker() { + // Mirrors op_orchestrator::nav_issues_tests's + // TWO_SCREENS_UNBOUND_PROFILE_TAB fixture: two screen-marked frames, the + // "Profile" tab in Home's bottom nav has no events bound yet. + let state = state_from_json( + r##"{ "version": "1.0", "children": [ + { "type": "frame", "id": "home", "name": "Home", "screen": "/", + "width": 390, "height": 844, "layout": "vertical", + "children": [ + { "type": "frame", "id": "nav", "name": "Bottom Nav", "role": "bottom-tab-bar", + "layout": "horizontal", "width": "fill_container", + "children": [ + { "type": "frame", "id": "tab-home", "layout": "vertical", + "events": { "onTap": [ { "replace": "\"/\"" } ] }, + "children": [ { "type": "text", "id": "t1", "content": "Home" } ] }, + { "type": "frame", "id": "tab-profile", "layout": "vertical", + "children": [ { "type": "text", "id": "t2", "content": "Profile" } ] } + ] } + ] }, + { "type": "frame", "id": "profile", "name": "Profile", "screen": "/profile", + "width": 390, "height": 844 } + ] }"##, + ); + let report = detect_blockers(&state); + assert!(report.has_blockers()); + let hit = report + .blockers + .iter() + .find(|b| b.category == "nav") + .expect("a nav blocker"); + assert!(hit.detail.contains("tab-profile"), "{hit:?}"); +} + +#[test] +fn fixing_the_document_makes_the_blocker_disappear() { + // The "no accumulating ledger" contract: re-running the SAME scan + // against a document where the issue was fixed must report nothing — + // there is no stale entry to prune because nothing was ever stored. + let mut state = state_from_json( + r##"{ "version": "1.0", "children": [ + { "type": "frame", "id": "home", "name": "Home", "width": 390, "height": 844, + "children": [ + { "type": "frame", "id": "section", "name": "RecentActivity", + "width": "fill_container", "height": "fit_content", "children": [] } + ] } + ] }"##, + ); + assert!(detect_blockers(&state).has_blockers()); + + // Fill the shell with real content. + let home = &mut state.active_children_mut()[0]; + let section = &mut home.children_mut().unwrap()[0]; + *section.children_mut().unwrap() = vec![serde_json::from_value(serde_json::json!( + { "type": "text", "id": "t1", "content": "No recent activity" } + )) + .expect("text node")]; + + assert!( + !detect_blockers(&state).has_blockers(), + "fixed shell must not still be reported" + ); +}