diff --git a/crates/op-ai/src/chat_provider.rs b/crates/op-ai/src/chat_provider.rs index ab7c6d6f5..3908ed529 100644 --- a/crates/op-ai/src/chat_provider.rs +++ b/crates/op-ai/src/chat_provider.rs @@ -352,6 +352,18 @@ pub struct ChatToolResult { /// [`ChatToolExecutor::finalize`]. pub const LOOP_FINALIZE_OP: &str = "__loop_finalize"; +/// 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 +/// "you committed N screens (A/B/C); X is still empty" contract line, not +/// just a bare unfilled-names list — a model is more likely to act on an +/// explicit broken promise than a generic "fill it now". +#[derive(Debug, Clone, Default, PartialEq, Eq)] +pub struct UnfilledScreensReport { + pub committed: Vec, + pub unfilled: Vec, +} + pub trait ChatToolExecutor: Send + Sync { fn execute(&self, name: &str, args_json: &str) -> ChatToolResult; @@ -367,7 +379,25 @@ pub trait ChatToolExecutor: Send + Sync { /// channel so the host can call `op_orchestrator::apply_loop_finalize` /// against the live document. Scripted / read-only test executors and any /// non-design tool executor inherit the no-op and are unaffected. - fn finalize(&self) {} + /// + /// Returns the promise-delivery report AFTER finalize ran and any still- + /// unfilled screens got marked on the canvas — the loop's unconditional + /// honest-report tier reads `unfilled` to append a transcript line. Empty + /// for every executor that doesn't own a live document. + fn finalize(&self) -> UnfilledScreensReport { + UnfilledScreensReport::default() + } + + /// Cheap, read-only promise-delivery check — same detector as + /// [`Self::finalize`] but WITHOUT marking the canvas or otherwise + /// mutating the document. The loop calls this whenever it's deciding + /// whether a dedicated fill round is owed (at its model-stop exit, and + /// again once its ordinary turn budget is exhausted) — a still-eligible + /// screen is worth one more round instead of immediately branding it + /// "(unfilled)". The default no-op mirrors [`Self::finalize`]'s. + fn check_unfilled_screens(&self) -> UnfilledScreensReport { + UnfilledScreensReport::default() + } } /// Test double — replays a fixed delta script. Lets the chat widget diff --git a/crates/op-editor-host-core/src/chat.rs b/crates/op-editor-host-core/src/chat.rs index 7368dc1c3..3b41ef566 100644 --- a/crates/op-editor-host-core/src/chat.rs +++ b/crates/op-editor-host-core/src/chat.rs @@ -7,7 +7,7 @@ use std::thread; use op_ai::chat_provider::{ ChatDelta, ChatHistoryRole, ChatProvider, ChatRequest, ChatToolExecutor, ChatToolResult, - LOOP_FINALIZE_OP, + UnfilledScreensReport, LOOP_FINALIZE_OP, }; use op_editor_core::{ChatMessage, ChatRole, ChatToolCall}; @@ -37,11 +37,44 @@ impl ChatToolExecutor for UiChatToolExecutor { /// Forward the reserved [`LOOP_FINALIZE_OP`] over the same tool channel so /// the host (which owns the live `EditorState`) runs - /// `op_orchestrator::apply_loop_finalize`. Blocks on the ack like any tool - /// call; the result envelope is discarded (finalize is fire-and-forget from - /// the loop's view). - fn finalize(&self) { - let _ = self.forward(LOOP_FINALIZE_OP, "{}"); + /// `op_orchestrator::apply_loop_finalize` plus the promise-delivery + /// invariant's detect-and-mark pass. Blocks on the ack like any tool call; + /// the ack's `content` carries back the (already canvas-marked) + /// committed/unfilled report — no new IPC needed, this reuses the same + /// envelope every other tool result already rides. + fn finalize(&self) -> UnfilledScreensReport { + unfilled_report_from_ack(&self.forward(LOOP_FINALIZE_OP, r#"{"checkOnly":false}"#)) + } + + /// Cheap, read-only promise-delivery check — same reserved op, with + /// `checkOnly: true` so the host runs ONLY the detector + /// (`unfilled_screens::detect_unfilled_screens` / + /// `list_screen_candidates`), never `apply_loop_finalize` or the + /// canvas-marking side effect. + fn check_unfilled_screens(&self) -> UnfilledScreensReport { + unfilled_report_from_ack(&self.forward(LOOP_FINALIZE_OP, r#"{"checkOnly":true}"#)) + } +} + +/// Parse the `{"success":true,"committed":[...],"unfilled":[...]}` envelope +/// the host's `LOOP_FINALIZE_OP` interception acks with. Any other shape (a +/// transport error, an old host build that doesn't know the fields yet) +/// degrades to an empty report — the promise-delivery invariant is a +/// best-effort nicety on top of the existing finalize/check calls, never a +/// reason to fail the turn. +fn unfilled_report_from_ack(result: &ChatToolResult) -> UnfilledScreensReport { + let Ok(v) = serde_json::from_str::(&result.content) else { + return UnfilledScreensReport::default(); + }; + let names = |key: &str| { + v.get(key) + .cloned() + .and_then(|v| serde_json::from_value::>(v).ok()) + .unwrap_or_default() + }; + UnfilledScreensReport { + committed: names("committed"), + unfilled: names("unfilled"), } } diff --git a/crates/op-editor-host-core/tests/design.rs b/crates/op-editor-host-core/tests/design.rs index 2edb85751..416f993b0 100644 --- a/crates/op-editor-host-core/tests/design.rs +++ b/crates/op-editor-host-core/tests/design.rs @@ -88,6 +88,7 @@ fn design_session_drains_progress_and_command_requests() { subtask: None, }], total_nodes: 2, + unfilled_screens: Vec::new(), }))) .expect("done"); diff --git a/crates/op-host-desktop/src/chat_session.rs b/crates/op-host-desktop/src/chat_session.rs index 1bb8b3d9f..f1aa6b4f3 100644 --- a/crates/op-host-desktop/src/chat_session.rs +++ b/crates/op-host-desktop/src/chat_session.rs @@ -138,7 +138,16 @@ fn pump_with_channel_interleave( // MiniPlayer survived an aborted run, test0711-22). if let Some(session) = current.as_ref() { if session.is_design_loop() && !session.loop_finalized() { - op_orchestrator::apply_loop_finalize(host.editor_state_mut()); + let state = host.editor_state_mut(); + op_orchestrator::apply_loop_finalize(state); + // Same promise-delivery marking the normal finalize op gets + // (see `execute_tool_requests` below) — an early-death run + // (429 / quota / abort) is exactly the case that must not + // ship a silently blank scaffolded screen either. There is + // no live transcript to append the tier-3 report line to at + // this point (the session is already tearing down), so the + // canvas name suffix is this path's honest-delivery signal. + op_orchestrator::unfilled_screens::finalize_and_mark_unfilled_screens(state); host.mark_editor_state_dirty(); } } @@ -204,11 +213,46 @@ fn execute_tool_requests( session.defer_tool_request(req); continue; } - op_orchestrator::apply_loop_finalize(state); - session.mark_loop_finalized(); - changed = true; + // `checkOnly: true` is the promise-delivery invariant's tier-2 + // probe (`ChatToolExecutor::check_unfilled_screens`) — read-only, + // no `apply_loop_finalize`, no canvas marking, no + // `mark_loop_finalized` (the loop may still send the real + // finalize op after this). Absent/malformed `checkOnly` defaults + // to the real finalize path, matching every op sent before this + // flag existed. + let check_only = serde_json::from_str::(&req.args_json) + .ok() + .and_then(|v| v.get("checkOnly").and_then(|b| b.as_bool())) + .unwrap_or(false); + let unfilled = if check_only { + op_orchestrator::unfilled_screens::detect_unfilled_screens(state) + .into_iter() + .map(|hit| hit.name) + .collect::>() + } else { + op_orchestrator::apply_loop_finalize(state); + let names = + op_orchestrator::unfilled_screens::finalize_and_mark_unfilled_screens(state); + session.mark_loop_finalized(); + changed = true; + names + }; + // The full committed-screen roster (filled or not) — read + // AFTER any finalize-side mutation above, so it reflects the + // same document `unfilled` was computed against. Lets the loop + // build the "you committed N screens (A/B/C); X is still empty" + // contract line instead of a bare unfilled-names nudge. + let committed = op_orchestrator::unfilled_screens::list_screen_candidates(state) + .into_iter() + .map(|hit| hit.name) + .collect::>(); let _ = req.ack.send(ChatToolResult { - content: serde_json::json!({ "success": true }).to_string(), + content: serde_json::json!({ + "success": true, + "committed": committed, + "unfilled": unfilled, + }) + .to_string(), is_error: false, }); continue; diff --git a/crates/op-host-desktop/src/design_session.rs b/crates/op-host-desktop/src/design_session.rs index cd09a3e47..c57e2605c 100644 --- a/crates/op-host-desktop/src/design_session.rs +++ b/crates/op-host-desktop/src/design_session.rs @@ -434,6 +434,20 @@ fn apply_progress(msg: &mut ChatMessage, progress: &[Progress], locale: Locale) ChatActivityStatus::Done, Some(op_i18n::translate(locale, "ai.designProgress.detail.standardPath").into()), ), + // "承诺-交付" honest report — not translated, same diagnostic + // confirmation-line treatment as GeometryEcho above. The canvas + // itself already carries the " (unfilled)" name suffix + // (`unfilled_screens::mark_unfilled_screens`); this line is the + // transcript-side half so the user sees it without having to + // scroll the layer panel. + Progress::UnfilledScreens { names } => append_narration( + msg, + &format!( + "• {} screen(s) left unfilled: {}", + names.len(), + names.join(", ") + ), + ), }; } changed diff --git a/crates/op-host-desktop/src/design_session_tests.rs b/crates/op-host-desktop/src/design_session_tests.rs index 6a8df9376..9bc4c09dc 100644 --- a/crates/op-host-desktop/src/design_session_tests.rs +++ b/crates/op-host-desktop/src/design_session_tests.rs @@ -126,6 +126,7 @@ fn end_to_end_pump_round_trips_apply_and_progress_via_actor_channels() { subtask: None, }], total_nodes: 3, + unfilled_screens: Vec::new(), }))); // Hold the sink so its channel survives until the UI has // had a chance to drain (the test polls until `Done`). @@ -234,6 +235,7 @@ fn pump_progress_captures_failed_subtask_specs_for_manual_retry() { subtask: Some(subtask), }], total_nodes: 0, + unfilled_screens: Vec::new(), }))); sink }); diff --git a/crates/op-host-services/src/chat_agent_loop.rs b/crates/op-host-services/src/chat_agent_loop.rs index c9e34177c..38e46ce08 100644 --- a/crates/op-host-services/src/chat_agent_loop.rs +++ b/crates/op-host-services/src/chat_agent_loop.rs @@ -4,12 +4,13 @@ //! the model stops or the turn cap is reached. Production uses the UI-thread //! bridge; loopback tests keep this transport layer deterministic. -use std::collections::BTreeMap; +use std::collections::{BTreeMap, HashMap}; use std::sync::Arc; use futures::StreamExt; use op_ai::chat_provider::{ ChatDelta, ChatHistoryRole, ChatToolDef, ChatToolExecutor, ChatToolResult, StopReason, + UnfilledScreensReport, }; use serde_json::{json, Value}; use tokio::sync::mpsc; @@ -22,8 +23,12 @@ use crate::chat_agent_context::{ }; use crate::chat_builtin_http::{map_anthropic_stop_reason, map_openai_stop_reason}; -/// Everything one agent-loop run needs. `max_turns` is the TS -/// `maxTurns` cap (20 in production; tests shrink it). +/// 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 +/// callers pick per `finalize_on_exit`). This caps only ORDINARY turns — +/// a dedicated promise-delivery fill round (see `FILL_BUDGET_MAX_ROUNDS_PER_SCREEN` +/// below) is deliberately exempt from it. Tests shrink `max_turns` further. pub struct AgentLoopConfig { pub url: String, pub api_key: String, @@ -113,12 +118,121 @@ async fn execute_tool( /// `enabled` is `cfg.finalize_on_exit`: a no-op early-return when this loop run /// is a regular chat turn (the shared agent loop also serves plain builtin /// chat), so only the gated design-generation loop mutates the document here. -async fn run_loop_finalize(executor: &Arc, enabled: bool) { +/// +/// Returns the promise-delivery invariant's committed/unfilled report AFTER +/// finalize ran and any still-unfilled screens got canvas-marked — empty on +/// the default no-op and on the common path where nothing was left unfilled. +async fn run_loop_finalize( + executor: &Arc, + enabled: bool, +) -> UnfilledScreensReport { if !enabled { - return; + return UnfilledScreensReport::default(); } let executor = executor.clone(); - let _ = tokio::task::spawn_blocking(move || executor.finalize()).await; + tokio::task::spawn_blocking(move || executor.finalize()) + .await + .unwrap_or_default() +} + +/// Cheap, read-only promise-delivery probe — same detector [`run_loop_finalize`] +/// runs, but never mutates the document or marks the canvas. Called whenever +/// the loop is deciding whether a dedicated fill round is owed, so a +/// still-eligible screen gets one more shot instead of immediately being +/// branded "(unfilled)". Gated by the same `enabled` flag as +/// [`run_loop_finalize`] — a plain chat turn never touches the document. +async fn check_unfilled( + executor: &Arc, + enabled: bool, +) -> UnfilledScreensReport { + if !enabled { + return UnfilledScreensReport::default(); + } + let executor = executor.clone(); + tokio::task::spawn_blocking(move || executor.check_unfilled_screens()) + .await + .unwrap_or_default() +} + +/// Per-screen dedicated fill-round budget. "预算只防失控，绝不截断已承诺的 +/// 工作" (budget only guards against a runaway retry avalanche; it must +/// never itself be the reason a promised screen ships empty): a fill round +/// spent nudging the model about a still-unfilled COMMITTED screen is exempt +/// from the ordinary `max_turns` cap — see the two loops' `turn >= turn_cap` +/// gates below, which keep issuing dedicated rounds past the cap as long as +/// some committed screen is still eligible. This constant is the ONLY thing +/// that stops that from running forever: the SAME screen failing across +/// `FILL_BUDGET_MAX_ROUNDS_PER_SCREEN` dedicated attempts is accepted as a +/// real failure (reported honestly, tier 3) rather than retried indefinitely +/// — 2 rounds mirrors `geometry_echo`'s "detect, retry, then accept" +/// discipline stretched by one extra attempt, since a wholly blank screen is +/// a starker failure than a layout nit. +const FILL_BUDGET_MAX_ROUNDS_PER_SCREEN: usize = 2; + +/// Which of `unfilled` still has dedicated fill-round budget left, per +/// `attempts` (screen name -> dedicated rounds already spent on it this run). +fn eligible_for_fill_round(unfilled: &[String], attempts: &HashMap) -> Vec { + unfilled + .iter() + .filter(|name| { + attempts.get(name.as_str()).copied().unwrap_or(0) < FILL_BUDGET_MAX_ROUNDS_PER_SCREEN + }) + .cloned() + .collect() +} + +/// Record that a dedicated fill round was just spent on each of `names`. +fn spend_fill_round(attempts: &mut HashMap, names: &[String]) { + for name in names { + *attempts.entry(name.clone()).or_insert(0) += 1; + } +} + +/// Tier-2 nudge text — states the FULL commitment, not just what's still +/// missing, so the model sees an explicit broken promise instead of a +/// generic "fill it now" ("把承诺变成模型可见的契约" — turn the promise into +/// a contract the model can see). `committed` is every screen the run +/// scaffolded (filled or not, from [`UnfilledScreensReport::committed`]); +/// `still_empty` is the fill-budget-eligible subset this round is asking the +/// model to act on. +fn contract_nudge_text(committed: &[String], still_empty: &[String]) -> String { + let commit_clause = if committed.len() > 1 { + format!( + "You committed {} screens ({}); ", + committed.len(), + committed.join("/") + ) + } else { + String::new() + }; + let (subject, pronoun) = if still_empty.len() == 1 { + ("is", "it") + } else { + ("are", "them") + }; + format!( + "{commit_clause}{} {subject} still empty. Complete {pronoun} before finishing.", + still_empty.join(", ") + ) +} + +/// Tier-3 unconditional honest report — appended to the transcript right +/// before `Done` whenever [`run_loop_finalize`] still finds unfilled screens +/// (every dedicated fill round the screen was eligible for ran out, or the +/// executor never had a live document to check). Wording mirrors the classic +/// path's `Progress::UnfilledScreens` line (`op-host-desktop::design_session`'s +/// `apply_progress` / `op-host-services::web_chat_standard`'s +/// `progress_label`) so a user sees the same sentence shape on either path. +async fn report_unfilled_if_any(tx: &mpsc::Sender, names: &[String]) { + if names.is_empty() { + return; + } + let text = format!( + "\n\n• {} screen(s) left unfilled: {}", + names.len(), + names.join(", ") + ); + let _ = tx.send(ChatDelta::TextDelta(text)).await; } // --------------------------------------------------------------------------- @@ -384,7 +498,41 @@ pub async fn run_anthropic_agent_loop( } messages.push(json!({ "role": "user", "content": cfg.user_prompt })); - for _turn in 0..cfg.max_turns.max(1) { + // Per-screen dedicated fill-round budget for this loop run (see + // `FILL_BUDGET_MAX_ROUNDS_PER_SCREEN`'s doc comment for the "budget + // never truncates committed work" rationale). + let mut fill_attempts: HashMap = HashMap::new(); + let turn_cap = cfg.max_turns.max(1); + let mut turn = 0usize; + loop { + // Ordinary turn budget exhausted — the ONLY reason to send another + // request is a committed screen that's still eligible for its own + // dedicated fill round. This request, if sent, does NOT count + // against `turn`. + if turn >= turn_cap { + let report = check_unfilled(&cfg.executor, cfg.finalize_on_exit).await; + let eligible = eligible_for_fill_round(&report.unfilled, &fill_attempts); + if eligible.is_empty() { + // Turn cap reached, and nothing committed is left worth + // trying for — the TS engine's error_max_turns shape. 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; + let _ = tx + .send(ChatDelta::Done { + stop_reason: StopReason::MaxTokens, + }) + .await; + return Ok(true); + } + spend_fill_round(&mut fill_attempts, &eligible); + messages.push( + json!({ "role": "user", "content": contract_nudge_text(&report.committed, &eligible) }), + ); + // Falls through to send this dedicated fill-round request below. + } + let mut body = json!({ "model": cfg.model, "max_tokens": cfg.max_output_tokens, @@ -428,10 +576,41 @@ pub async fn run_anthropic_agent_loop( .as_deref() .map(map_anthropic_stop_reason) .unwrap_or(StopReason::EndTurn); - // Normal model-stop exit: 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. - run_loop_finalize(&cfg.executor, cfg.finalize_on_exit).await; + if turn >= turn_cap { + // Already in dedicated-fill-round territory — the top-of-loop + // gate above owns deciding whether ANOTHER round is owed (it + // re-checks fresh on the next pass). Record this turn's reply + // in history and defer, rather than deciding again here — + // deciding in both places would double-nudge. + messages + .push(json!({ "role": "assistant", "content": collector.assistant_content() })); + continue; + } + // Model voluntarily stopped, still within ordinary budget. Try + // one dedicated fill round for any committed screen that's still + // eligible — NOT gated by remaining turn budget (only by the + // per-screen cap above), so budget is never the reason a + // promised screen ships empty. + let report = check_unfilled(&cfg.executor, cfg.finalize_on_exit).await; + let eligible = eligible_for_fill_round(&report.unfilled, &fill_attempts); + if !eligible.is_empty() { + spend_fill_round(&mut fill_attempts, &eligible); + messages + .push(json!({ "role": "assistant", "content": collector.assistant_content() })); + messages.push( + json!({ "role": "user", "content": contract_nudge_text(&report.committed, &eligible) }), + ); + 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; let _ = tx .send(ChatDelta::Done { stop_reason: reason, @@ -439,6 +618,9 @@ pub async fn run_anthropic_agent_loop( .await; return Ok(true); } + if turn < turn_cap { + turn += 1; + } messages.push(json!({ "role": "assistant", "content": collector.assistant_content() })); let mut results: Vec = Vec::new(); @@ -495,17 +677,6 @@ pub async fn run_anthropic_agent_loop( } messages.push(json!({ "role": "user", "content": results })); } - - // Turn cap reached with the model still calling tools — stop the - // loop the way the TS engine reports error_max_turns. Still run the Step-4 - // structural backstop ONCE over whatever the truncated turn assembled. - run_loop_finalize(&cfg.executor, cfg.finalize_on_exit).await; - let _ = tx - .send(ChatDelta::Done { - stop_reason: StopReason::MaxTokens, - }) - .await; - Ok(true) } // --------------------------------------------------------------------------- @@ -646,7 +817,34 @@ pub async fn run_openai_agent_loop( } messages.push(json!({ "role": "user", "content": cfg.user_prompt })); - for _turn in 0..cfg.max_turns.max(1) { + // Per-screen dedicated fill-round budget for this loop run — see the + // Anthropic loop above (`FILL_BUDGET_MAX_ROUNDS_PER_SCREEN`'s doc + // comment) for the full "budget never truncates committed work" + // rationale. + let mut fill_attempts: HashMap = HashMap::new(); + let turn_cap = cfg.max_turns.max(1); + let mut turn = 0usize; + loop { + if turn >= turn_cap { + let report = check_unfilled(&cfg.executor, cfg.finalize_on_exit).await; + let eligible = eligible_for_fill_round(&report.unfilled, &fill_attempts); + 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; + let _ = tx + .send(ChatDelta::Done { + stop_reason: StopReason::MaxTokens, + }) + .await; + return Ok(true); + } + spend_fill_round(&mut fill_attempts, &eligible); + messages.push( + json!({ "role": "user", "content": contract_nudge_text(&report.committed, &eligible) }), + ); + // Falls through to send this dedicated fill-round request below. + } + let mut body = json!({ "model": cfg.model, "stream": true, @@ -696,9 +894,39 @@ pub async fn run_openai_agent_loop( .as_deref() .map(map_openai_stop_reason) .unwrap_or(StopReason::EndTurn); + let stop_content = || { + if collector.text.is_empty() { + Value::Null + } else { + Value::String(collector.text.clone()) + } + }; + if turn >= turn_cap { + // Already in dedicated-fill-round territory — the + // top-of-loop gate above owns deciding whether ANOTHER + // round is owed. Record this turn's reply and defer. + messages.push(json!({ "role": "assistant", "content": stop_content() })); + continue; + } + // Model voluntarily stopped, still within ordinary budget. Try + // one dedicated fill round for any committed screen that's + // still eligible — NOT gated by remaining turn budget (only by + // the per-screen cap above). + let report = check_unfilled(&cfg.executor, cfg.finalize_on_exit).await; + let eligible = eligible_for_fill_round(&report.unfilled, &fill_attempts); + if !eligible.is_empty() { + spend_fill_round(&mut fill_attempts, &eligible); + messages.push(json!({ "role": "assistant", "content": stop_content() })); + messages.push( + json!({ "role": "user", "content": contract_nudge_text(&report.committed, &eligible) }), + ); + 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. - run_loop_finalize(&cfg.executor, cfg.finalize_on_exit).await; + // 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; let _ = tx .send(ChatDelta::Done { stop_reason: reason, @@ -706,6 +934,9 @@ pub async fn run_openai_agent_loop( .await; return Ok(true); } + if turn < turn_cap { + turn += 1; + } let tool_calls_json: Vec = calls .iter() @@ -784,16 +1015,6 @@ pub async fn run_openai_agent_loop( } } } - - // Turn cap reached — run the Step-4 structural backstop ONCE over whatever - // the truncated turn assembled. - run_loop_finalize(&cfg.executor, cfg.finalize_on_exit).await; - let _ = tx - .send(ChatDelta::Done { - stop_reason: StopReason::MaxTokens, - }) - .await; - Ok(true) } #[cfg(test)] 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 6697b35ee..748d09693 100644 --- a/crates/op-host-services/src/chat_agent_loop_tests.rs +++ b/crates/op-host-services/src/chat_agent_loop_tests.rs @@ -14,14 +14,21 @@ use std::time::Duration; use super::*; use crate::chat_runtime::shared_runtime; use base64::Engine as _; -use op_ai::chat_provider::{ChatToolDef, ChatToolExecutor, ChatToolResult}; +use op_ai::chat_provider::{ChatToolDef, ChatToolExecutor, ChatToolResult, UnfilledScreensReport}; /// Executor double — records calls + loop-finalize invocations, replays a /// fixed result. struct ScriptedExecutor { calls: Mutex>, finalize_calls: std::sync::atomic::AtomicUsize, + unfilled_check_calls: std::sync::atomic::AtomicUsize, results: Mutex>, + /// Scripted `check_unfilled_screens` return values, popped in call + /// order; once exhausted, further calls return the default (empty) + /// report — the default for every test that never scripts this. + unfilled_checks: Mutex>, + /// Scripted `finalize` return values, popped the same way. + unfilled_finalizes: Mutex>, } impl ScriptedExecutor { @@ -33,6 +40,7 @@ impl ScriptedExecutor { Arc::new(Self { calls: Mutex::new(Vec::new()), finalize_calls: std::sync::atomic::AtomicUsize::new(0), + unfilled_check_calls: std::sync::atomic::AtomicUsize::new(0), results: Mutex::new( contents .iter() @@ -42,6 +50,8 @@ impl ScriptedExecutor { }) .collect(), ), + unfilled_checks: Mutex::new(VecDeque::new()), + unfilled_finalizes: Mutex::new(VecDeque::new()), }) } @@ -54,6 +64,38 @@ impl ScriptedExecutor { self.finalize_calls .load(std::sync::atomic::Ordering::SeqCst) } + + /// How many times the loop ran the cheap read-only promise-delivery + /// probe (`check_unfilled_screens`). + fn unfilled_checks(&self) -> usize { + self.unfilled_check_calls + .load(std::sync::atomic::Ordering::SeqCst) + } + + /// Queue one scripted `check_unfilled_screens` return value. + fn with_unfilled_check(self: Arc, committed: &[&str], unfilled: &[&str]) -> Arc { + self.unfilled_checks + .lock() + .unwrap() + .push_back(report_of(committed, unfilled)); + self + } + + /// Queue one scripted `finalize` return value. + fn with_unfilled_finalize(self: Arc, committed: &[&str], unfilled: &[&str]) -> Arc { + self.unfilled_finalizes + .lock() + .unwrap() + .push_back(report_of(committed, unfilled)); + self + } +} + +fn report_of(committed: &[&str], unfilled: &[&str]) -> UnfilledScreensReport { + UnfilledScreensReport { + committed: committed.iter().map(|s| s.to_string()).collect(), + unfilled: unfilled.iter().map(|s| s.to_string()).collect(), + } } impl ChatToolExecutor for ScriptedExecutor { @@ -70,9 +112,24 @@ impl ChatToolExecutor for ScriptedExecutor { } } - fn finalize(&self) { + fn finalize(&self) -> UnfilledScreensReport { self.finalize_calls .fetch_add(1, std::sync::atomic::Ordering::SeqCst); + self.unfilled_finalizes + .lock() + .unwrap() + .pop_front() + .unwrap_or_default() + } + + fn check_unfilled_screens(&self) -> UnfilledScreensReport { + self.unfilled_check_calls + .fetch_add(1, std::sync::atomic::Ordering::SeqCst); + self.unfilled_checks + .lock() + .unwrap() + .pop_front() + .unwrap_or_default() } } @@ -163,8 +220,7 @@ fn get_screenshot_tool_def() -> ChatToolDef { /// A 1×1 transparent PNG, base64-encoded — stands in for a rendered design /// so the tests can decode it back and prove it round-trips through the wire. -const TINY_PNG_B64: &str = - "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNkYPhfDwAChwGA60e6kgAAAABJRU5ErkJggg=="; +const TINY_PNG_B64: &str = "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNkYPhfDwAChwGA60e6kgAAAABJRU5ErkJggg=="; const SECOND_TINY_PNG_B64: &str = "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mP8/x8AAusB9Y9Zl9sAAAAASUVORK5CYII="; @@ -782,6 +838,201 @@ fn openai_loop_keeps_only_latest_screenshot_without_dropping_user_intent() { ); } +// ── "承诺-交付" invariant: dedicated fill-round budget + tier-3 honest report ── +// Budget only guards a runaway retry avalanche — it must never itself be the +// reason a promised screen ships empty (user policy upgrade, 2026-07-18). +// Dedicated fill rounds are therefore exempt from `max_turns`; only a +// per-screen cap (`FILL_BUDGET_MAX_ROUNDS_PER_SCREEN = 2`) stops the SAME +// screen from being nudged forever. + +#[test] +fn model_stop_with_budget_left_gets_a_fill_round_then_reports_nothing_once_filled() { + // Turn 1: model stops without calling any tool (calls.is_empty()) while + // budget remains (max_turns: 3). The cheap check finds "Saved" unfilled + // → the loop injects one dedicated fill round instead of finalizing + // immediately. Turn 2: model stops again; this time the check reports + // nothing left unfilled (the fill round "worked") → straight to + // finalize, which also finds nothing left. + let (base, req_rx) = serve_sse_script(vec![anthropic_text_turn(), anthropic_text_turn()]); + let executor = ScriptedExecutor::ok(r#"{"success":true,"data":{}}"#) + .with_unfilled_check(&["Trips", "Destination", "Saved"], &["Saved"]) + .with_unfilled_check(&["Trips", "Destination", "Saved"], &[]); + let cfg = AgentLoopConfig { + url: format!("{base}/v1/messages"), + api_key: "sk-test".into(), + model: "claude-test".into(), + system_prompt: "You are a design editor.".into(), + history: Vec::new(), + user_prompt: "build the 3-screen app".into(), + max_output_tokens: 512, + tools: vec![update_node_tool_def()], + executor: executor.clone(), + max_turns: 3, + finalize_on_exit: true, + disable_thinking: false, + dial_policy: crate::provider_dial::EndpointDialPolicy::Trusted, + }; + let (outcome, deltas) = run_loop_collect(cfg, true); + assert_eq!(outcome, Ok(true)); + + // One fill-round probe finds it, a second confirms it's fixed; finalize + // still runs exactly once at the real exit. + assert_eq!(executor.unfilled_checks(), 2); + assert_eq!( + executor.finalizes(), + 1, + "finalize still runs exactly once, at the real exit" + ); + + // The fill round's contract line — the FULL commitment, not just the + // gap — 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("fill-round follow-up request captured"); + assert!( + second.contains("You committed 3 screens (Trips/Destination/Saved)") + && second.contains("Saved is still empty") + && second.contains("Complete it before finishing"), + "the fill round's contract line must state the full commitment, got: {second}" + ); + + // Nothing left unfilled after the fill round → no tier-3 report line. + assert!( + !deltas + .iter() + .any(|d| matches!(d, ChatDelta::TextDelta(s) if s.contains("left unfilled"))), + "a screen the fill round filled must not also be reported as unfilled" + ); + assert!(matches!( + deltas.last(), + Some(ChatDelta::Done { + stop_reason: StopReason::EndTurn + }) + )); +} + +#[test] +fn model_stop_repeatedly_failing_the_same_screen_is_capped_then_honestly_reported() { + // The model voluntarily stops 3 times in a row, and "Saved" is STILL + // unfilled every time — the per-screen cap (2 dedicated rounds) must + // stop trying after the 2nd nudge and accept the failure on the 3rd + // check, rather than nudging forever. Turn budget (max_turns: 5) is + // deliberately generous so ONLY the per-screen cap — not the ordinary + // turn budget — is what ends the retrying. + 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_unfilled_check(&["Saved"], &["Saved"]) + .with_unfilled_check(&["Saved"], &["Saved"]) + .with_unfilled_check(&["Saved"], &["Saved"]) + .with_unfilled_finalize(&["Saved"], &["Saved"]); + let cfg = AgentLoopConfig { + url: format!("{base}/v1/messages"), + api_key: "sk-test".into(), + model: "claude-test".into(), + system_prompt: "You are a design editor.".into(), + history: Vec::new(), + user_prompt: "build the 3-screen app".into(), + max_output_tokens: 512, + tools: vec![update_node_tool_def()], + executor: executor.clone(), + max_turns: 5, + finalize_on_exit: true, + disable_thinking: false, + dial_policy: crate::provider_dial::EndpointDialPolicy::Trusted, + }; + let (outcome, deltas) = run_loop_collect(cfg, true); + assert_eq!(outcome, Ok(true)); + + // 3 checks: round 1 (eligible, nudge), round 2 (still eligible, nudge), + // round 3 (attempts == cap, no longer eligible → stop trying). + assert_eq!( + executor.unfilled_checks(), + 3, + "the per-screen cap, not the ordinary turn budget, is what stops the retrying here" + ); + assert_eq!(executor.finalizes(), 1); + + let report = deltas.iter().find_map(|d| match d { + ChatDelta::TextDelta(s) if s.contains("left unfilled") => Some(s.clone()), + _ => None, + }); + assert!( + report.is_some_and(|s| s.contains("Saved")), + "a screen still unfilled after exhausting its dedicated fill budget must be reported by name, got: {deltas:?}" + ); +} + +#[test] +fn turn_cap_exhausted_still_gets_dedicated_fill_rounds_before_reporting() { + // The model calls a tool on every turn until the ordinary budget + // (max_turns: 2) is exhausted — the 0718-1-glm-1 incident's shape. Per + // the "budget only guards runaway" policy, running out of ordinary + // budget must NOT itself be the reason "Saved" ships empty: the loop + // still owes it up to 2 dedicated fill rounds (uncounted against + // max_turns) before accepting the failure and reporting honestly. + let (base, req_rx) = serve_sse_script(vec![ + anthropic_tool_use_turn(), // turn 1/2 — normal budget + anthropic_tool_use_turn(), // turn 2/2 — normal budget exhausted + anthropic_text_turn(), // dedicated fill round 1 (uncounted) + anthropic_text_turn(), // dedicated fill round 2 (uncounted) + ]); + let executor = ScriptedExecutor::ok(r#"{"success":true,"data":{}}"#) + .with_unfilled_check(&["Saved"], &["Saved"]) + .with_unfilled_check(&["Saved"], &["Saved"]) + .with_unfilled_check(&["Saved"], &["Saved"]) + .with_unfilled_finalize(&["Saved"], &["Saved"]); + let cfg = 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 3-screen app".into(), + max_output_tokens: 128, + tools: vec![update_node_tool_def()], + executor: executor.clone(), + max_turns: 2, + finalize_on_exit: true, + disable_thinking: false, + dial_policy: crate::provider_dial::EndpointDialPolicy::Trusted, + }; + let (outcome, deltas) = run_loop_collect(cfg, true); + assert_eq!(outcome, Ok(true)); + + // 2 real tool-call turns consumed the ordinary budget, THEN 2 more + // requests went out as dedicated fill rounds (uncounted) before the + // per-screen cap finally stopped the retrying. + assert_eq!(executor.calls().len(), 2, "the 2 ordinary tool-call turns"); + for _ in 0..4 { + req_rx + .recv() + .expect("all 4 requests, including the 2 dedicated fill rounds, went out"); + } + assert_eq!( + executor.unfilled_checks(), + 3, + "a turn-cap exit must still spend its dedicated fill-round budget, not skip straight to reporting" + ); + assert_eq!(executor.finalizes(), 1); + assert!( + deltas + .iter() + .any(|d| matches!(d, ChatDelta::TextDelta(s) if s.contains("left unfilled") && s.contains("Saved"))), + "a run that exhausts its dedicated fill rounds without success must still get the honest report" + ); + assert!(matches!( + deltas.last(), + Some(ChatDelta::Done { + stop_reason: StopReason::MaxTokens + }) + )); +} + #[test] fn tool_card_envelope_wraps_args_with_level_and_running_status() { let envelope = tool_card_envelope("modify", r#"{"nodeId":"n1"}"#); diff --git a/crates/op-host-services/src/web_chat_standard.rs b/crates/op-host-services/src/web_chat_standard.rs index 38b211ec0..bc9b41858 100644 --- a/crates/op-host-services/src/web_chat_standard.rs +++ b/crates/op-host-services/src/web_chat_standard.rs @@ -709,6 +709,13 @@ fn progress_label(p: &Progress) -> String { } } Progress::VisualRefFallback { reason } => format!("• Visual-ref fallback: {reason}"), + Progress::UnfilledScreens { names } => { + format!( + "• {} screen(s) left unfilled: {}", + names.len(), + names.join(", ") + ) + } } } diff --git a/crates/op-orchestrator/src/cleanup.rs b/crates/op-orchestrator/src/cleanup.rs index 0ff6d55ac..0c45b3612 100644 --- a/crates/op-orchestrator/src/cleanup.rs +++ b/crates/op-orchestrator/src/cleanup.rs @@ -1397,6 +1397,14 @@ pub fn run_cleanup_passes(sink: &mut dyn DocSink, plan: &OrchestratorPlan, root_ strip_decorative_filled_strokes(sink, rid); crate::radial_repair::repair_radial_stacks(sink, rid); crate::stub_repair::remove_empty_decorated_stubs(sink, rid); + // A section's header row whose second child is the ENTIRE content + // body (not a chevron/badge), with the body redundantly repeating + // the header's own title as its own first child — the loop's + // freeform fill step drew a title the section already had and + // misnested the body as the header's flex sibling instead of the + // header's own sibling. Runs before geometry validation so it sees + // the corrected tree, not the pre-repair one. + crate::section_shell_fill_repair::repair_section_shell_fill_ownership(sink, rid); // Geometry-driven validation LOOP: run the REAL jian layout, detect + // fix what the resolved rects prove wrong (table columns overflowing // their row, fill containers collapsed to 0 height by a hugging ancestor diff --git a/crates/op-orchestrator/src/lib.rs b/crates/op-orchestrator/src/lib.rs index 417a6d7eb..a1e2641ea 100644 --- a/crates/op-orchestrator/src/lib.rs +++ b/crates/op-orchestrator/src/lib.rs @@ -68,6 +68,7 @@ pub mod role_post_pass; pub mod run; pub mod scaffold; pub mod screen_groups; +pub(crate) mod section_shell_fill_repair; pub(crate) mod sidebar_archetype; pub mod spacing_repair; pub mod spawn_concurrent; @@ -76,6 +77,7 @@ pub mod stub_repair; pub mod subagent; pub mod table_repair; pub mod tree_heuristics; +pub mod unfilled_screens; pub(crate) mod unify_shared_nav; pub mod wire_screen_navigation; diff --git a/crates/op-orchestrator/src/nav_issues.rs b/crates/op-orchestrator/src/nav_issues.rs index 7bbb3d65b..1b99a483f 100644 --- a/crates/op-orchestrator/src/nav_issues.rs +++ b/crates/op-orchestrator/src/nav_issues.rs @@ -25,7 +25,7 @@ use jian_ops_schema::node::PenNode; use op_editor_core::{EditorState, PenNodeExt}; use crate::wire_screen_navigation::{ - collect_nav_containers, first_text_content, labels_match, node_has_events, normalize_label, + collect_nav_containers, first_text_content, labels_match, node_has_events, }; /// A top-level `Frame` already carrying an authored `screen` route. @@ -69,9 +69,12 @@ pub fn scan_nav_issues(state: &EditorState) -> Vec { if screens.len() < 2 { return Vec::new(); } - let targets: Vec<(String, String)> = screens + // Raw (un-normalized) names — `labels_match` normalizes + tokenizes + // internally, so a brand-prefixed screen name still matches a bare tab + // label (see that function's doc). + let targets: Vec<(&str, &str)> = screens .iter() - .map(|s| (normalize_label(&s.name), s.path.clone())) + .map(|s| (s.name.as_str(), s.path.as_str())) .collect(); let mut issues = Vec::new(); @@ -89,10 +92,9 @@ pub fn scan_nav_issues(state: &EditorState) -> Vec { let Some(label) = first_text_content(item) else { continue; }; - let tab_key = normalize_label(label); let Some((_, target_path)) = targets .iter() - .find(|(screen_key, _)| labels_match(&tab_key, screen_key)) + .find(|(screen_name, _)| labels_match(label, screen_name)) else { continue; }; diff --git a/crates/op-orchestrator/src/run.rs b/crates/op-orchestrator/src/run.rs index d6387b8c7..203d93760 100644 --- a/crates/op-orchestrator/src/run.rs +++ b/crates/op-orchestrator/src/run.rs @@ -717,6 +717,21 @@ impl Orchestrator { ); } + // -- 阶段 6:承诺-交付不变量(classic 路径的诚实上报,与 loop 路径共享检测器)-- + // Runs AFTER cleanup + validation so a screen a structural pass or + // the vision loop still touched is judged on its FINAL state, not + // a stale mid-run snapshot. `mark_unfilled_screens` labels the + // canvas itself (not just this summary) — a user scrolling the + // layer panel sees it even without reading `RunSummary`. + let unfilled_hits = crate::unfilled_screens::detect_unfilled_screens(sink.state()); + let unfilled_screens: Vec = unfilled_hits.iter().map(|h| h.name.clone()).collect(); + if !unfilled_hits.is_empty() { + crate::unfilled_screens::mark_unfilled_screens(sink, &unfilled_hits); + on_progress(Progress::UnfilledScreens { + names: unfilled_screens.clone(), + }); + } + let total_nodes = outcomes.iter().map(|o| o.node_count).sum(); Ok(RunSummary { // First surviving root is the "primary" root_frame_id — mirrors @@ -725,6 +740,7 @@ impl Orchestrator { root_frame_id: root_ids.first().cloned().unwrap_or_default(), subtasks: outcomes, total_nodes, + unfilled_screens, }) } } diff --git a/crates/op-orchestrator/src/run_tests_screen_groups.rs b/crates/op-orchestrator/src/run_tests_screen_groups.rs index 02dbe5237..5ac8e6a33 100644 --- a/crates/op-orchestrator/src/run_tests_screen_groups.rs +++ b/crates/op-orchestrator/src/run_tests_screen_groups.rs @@ -205,10 +205,20 @@ const THREE_SCREEN_PLAN_JSON: &str = r##"{ ] }"##; +/// Matches on the base name OR that name with the promise-delivery +/// invariant's " (unfilled)" suffix (`unfilled_screens::mark_unfilled_screens`) +/// — a subtask whose group never delivered content is exactly the case that +/// suffix exists to flag, so a root under test here may legitimately carry +/// it by the time `run` returns. fn find_root_by_name<'a>(roots: &'a [PenNode], name: &str) -> &'a PenNode { roots .iter() - .find(|r| r.base().name.as_deref() == Some(name)) + .find(|r| { + r.base() + .name + .as_deref() + .is_some_and(|n| n == name || n == format!("{name} (unfilled)")) + }) .unwrap_or_else(|| panic!("no root named {name} among {roots:?}")) } @@ -332,6 +342,20 @@ fn one_group_failure_does_not_take_down_the_others() { !contains_text(library, "l"), "Library's failed subtask must never backfill with the wrong content" ); + // Promise-delivery invariant: a scaffolded screen that never received + // subtask content must not ship silently — `run` marks it on the canvas + // itself (`unfilled_screens::mark_unfilled_screens`) and reports it in + // `RunSummary`. + assert_eq!( + library.base().name.as_deref(), + Some("Library (unfilled)"), + "a screen whose subtask never delivered content must be marked unfilled on the canvas" + ); + assert_eq!( + summary.unfilled_screens, + vec!["Library".to_string()], + "the classic-path summary must also report the same unfilled screen" + ); assert_eq!(summary.subtasks.len(), 3); assert!( diff --git a/crates/op-orchestrator/src/section_shell_fill_repair.rs b/crates/op-orchestrator/src/section_shell_fill_repair.rs new file mode 100644 index 000000000..bb4b4c5d5 --- /dev/null +++ b/crates/op-orchestrator/src/section_shell_fill_repair.rs @@ -0,0 +1,175 @@ +//! Section shell/fill ownership repair — a horizontal "header row" whose +//! second child is actually the ENTIRE content body of the section (not a +//! chevron/badge), and that body redundantly repeats the header's own +//! title as its own first child. +//! +//! Measured (`0718-1-glm-1.op`, GLM-5.2 built-in loop provider): a +//! "Must-See" checklist section wanted the shape `[header-row(title), +//! content-section]` as two SIBLINGS, but the model instead nested the +//! whole `ChecklistSection` INSIDE the header row as its second +//! `layout:"horizontal"` flex child, alongside a "Must-See" title it +//! separately drew again as the checklist's own first child. With +//! `justifyContent:"space_between"` on the row, the short title pins to +//! the left while the much-taller checklist fills the remaining width to +//! its right — visually a giant title floating mid-list (confirmed by +//! render). Two symptoms, one root cause: the fill step didn't know a +//! title already existed for this section and didn't know the checklist +//! belonged as the header's sibling, not its child. +//! +//! This did NOT come from a Rust scaffold function seeding a pre-built +//! "header row with title" shell — `design-agent.md`'s skeleton-first +//! protocol only pre-authors a bare, empty, NAMED top-level section frame +//! (no title drawn yet); everything below that is freeform model output +//! across one or more `batch_design` calls. So there is no shell BUILDER +//! to restructure — the fix has to be a deterministic REPAIR of the +//! resulting shape, the same "shared throat-layer net, not model-specific +//! patch" pattern the rest of `cleanup.rs` already follows. +//! +//! Detection is deliberately narrow and evidence-driven: a horizontal row +//! with EXACTLY two children where one is a short title (a `text` node, or +//! a small wrapper whose own descendant text) and the other is a +//! container whose OWN FIRST CHILD's text is byte-for-byte IDENTICAL to +//! the title's. A design legitimately repeating the exact same string as +//! both a row's direct child AND its sibling's first child is not a +//! pattern real design ever produces on purpose — the duplicate-text +//! signal alone carries enough precision that no additional shape gating +//! (e.g. requiring `justifyContent:"space_between"`) is needed to avoid +//! false positives. +//! +//! Repair: delete the duplicate title inside the content container, then +//! move the (now title-free) container out of the header row to become +//! its SIBLING, inserted immediately after it — producing exactly +//! `[header-row(title), content-section]`. + +use op_editor_core::{EditorCommand, NodeId}; +use serde_json::Value; + +use crate::types::DocSink; + +fn children(v: &Value) -> &[Value] { + v.get("children") + .and_then(Value::as_array) + .map(Vec::as_slice) + .unwrap_or(&[]) +} + +fn layout_str(v: &Value) -> Option<&str> { + v.get("layout").and_then(Value::as_str) +} + +/// The text this node "is" or "starts with" — itself if it's a `text` +/// node, otherwise its first descendant `text` node (covers a title +/// wrapped in a small alignment/padding frame). +fn text_of(v: &Value) -> Option<&str> { + if v.get("type").and_then(Value::as_str) == Some("text") { + return v.get("content").and_then(Value::as_str); + } + children(v).iter().find_map(text_of) +} + +/// Detect + queue fixes for the shell/fill duplicate-title shape, walking +/// `v`'s subtree. `parent_id` / `index_in_parent` describe `v` itself +/// (needed so a match can move the misnested container to be `v`'s own +/// sibling) — `None` at the tree root, where nothing can be promoted out +/// of (a root has no parent to become a sibling under). +fn collect_fixes( + v: &Value, + parent_id: Option<&str>, + index_in_parent: Option, + cmds: &mut Vec, +) { + if layout_str(v) == Some("horizontal") { + let kids = children(v); + if kids.len() == 2 { + if let Some(fix) = matched_duplicate_title(v, &kids[0], &kids[1]) + .or_else(|| matched_duplicate_title(v, &kids[1], &kids[0])) + { + if let (Some(parent_id), Some(row_index)) = (parent_id, index_in_parent) { + cmds.push(EditorCommand::DeleteNode { + node_id: NodeId::new(fix.duplicate_title_id), + page_id: None, + }); + cmds.push(EditorCommand::MoveNode { + node_id: NodeId::new(fix.container_id), + target_parent: NodeId::new(parent_id.to_string()), + page_id: None, + index: Some(row_index + 1), + }); + } + // Matched (or would have, but sits at the document root with + // nothing to become a sibling under) — either way this row + // is not a candidate for further recursion. + return; + } + } + } + let this_id = v.get("id").and_then(Value::as_str); + for (i, c) in children(v).iter().enumerate() { + collect_fixes(c, this_id, Some(i), cmds); + } +} + +struct DuplicateTitleFix { + /// The content container's own first child — the redundant title to + /// delete. + duplicate_title_id: String, + /// The content container itself — to promote out as the row's sibling. + container_id: String, +} + +/// `title` and `container` are the row's two children, in either order. +/// Matches iff `container`'s OWN FIRST CHILD's text is identical to +/// `title`'s text. +fn matched_duplicate_title( + _row: &Value, + title: &Value, + container: &Value, +) -> Option { + let title_text = text_of(title)?; + if title_text.is_empty() { + return None; + } + let first_child = children(container).first()?; + if text_of(first_child)? != title_text { + return None; + } + Some(DuplicateTitleFix { + duplicate_title_id: first_child.get("id")?.as_str()?.to_string(), + container_id: container.get("id")?.as_str()?.to_string(), + }) +} + +/// Repair every `[header-row(title, misnested-content-with-duplicate-title)]` +/// shape under `root_id`. Returns the number of sections repaired. +pub(crate) fn repair_section_shell_fill_ownership(sink: &mut dyn DocSink, root_id: &str) -> usize { + let Some(root) = op_editor_core::walkers::find_node( + sink.state().active_children(), + &NodeId::new(root_id.to_string()), + ) else { + return 0; + }; + let Ok(v) = serde_json::to_value(root) else { + return 0; + }; + let mut cmds = Vec::new(); + // The root itself has no parent to promote a sibling under, so start + // the walk from its children (each already carries `root_id` as its + // parent). + let root_id_owned = v.get("id").and_then(Value::as_str).map(str::to_string); + for (i, c) in children(&v).iter().enumerate() { + collect_fixes(c, root_id_owned.as_deref(), Some(i), &mut cmds); + } + if cmds.is_empty() { + return 0; + } + // Each match contributes exactly one DeleteNode + one MoveNode. + let repaired = cmds.len() / 2; + for cmd in cmds { + sink.apply(cmd); + } + repaired +} + +#[cfg(test)] +#[path = "section_shell_fill_repair_tests.rs"] +mod tests; diff --git a/crates/op-orchestrator/src/section_shell_fill_repair_tests.rs b/crates/op-orchestrator/src/section_shell_fill_repair_tests.rs new file mode 100644 index 000000000..1d8b2cf01 --- /dev/null +++ b/crates/op-orchestrator/src/section_shell_fill_repair_tests.rs @@ -0,0 +1,230 @@ +use super::*; +use crate::test_support::VecDocSink; +use jian_ops_schema::node::PenNode; +use op_editor_core::PenNodeExt; +use serde_json::json; + +fn sink_with_root(root: serde_json::Value) -> VecDocSink { + let node: PenNode = serde_json::from_value(root).expect("valid root"); + let mut sink = VecDocSink::new(); + sink.state.apply(EditorCommand::InsertSubtree { + nodes: vec![node], + parent_id: NodeId::NONE, + page_id: None, + }); + sink.applied.clear(); + sink +} + +fn find_by_name<'a>(nodes: &'a [PenNode], name: &str) -> Option<&'a PenNode> { + for n in nodes { + if n.base().name.as_deref() == Some(name) { + return Some(n); + } + if let Some(children) = n.children() { + if let Some(hit) = find_by_name(children, name) { + return Some(hit); + } + } + } + None +} + +fn child_names(node: &PenNode) -> Vec { + node.children() + .into_iter() + .flatten() + .map(|c| c.base().name.clone().unwrap_or_default()) + .collect() +} + +/// The de-identified `0718-1-glm-1.op` shape: a section wrapper whose only +/// child is a "header row" — title text on one side, the ENTIRE checklist +/// misnested as the row's other flex child, the checklist redundantly +/// repeating the title as its own first child. +fn duplicate_title_section() -> serde_json::Value { + json!({ + "type": "frame", "id": "wrapper", "name": "SectionWrapper", + "width": "fill_container", "height": "fit_content", "layout": "vertical", + "children": [ + { + "type": "frame", "id": "header-row", "name": "HeaderRow", + "width": "fill_container", "height": "fit_content", + "layout": "horizontal", "justifyContent": "space_between", + "children": [ + { "type": "text", "id": "title", "name": "Title", "content": "Must-See", + "width": "fit_content", "height": "fit_content" }, + { + "type": "frame", "id": "checklist", "name": "ChecklistSection", + "width": "fill_container", "height": "fit_content", "layout": "vertical", + "children": [ + { "type": "text", "id": "dup-title", "name": "DupTitle", + "content": "Must-See", "width": "fit_content", "height": "fit_content" }, + { "type": "text", "id": "item-1", "name": "Item1", "content": "Fushimi Inari", + "width": "fit_content", "height": "fit_content" }, + { "type": "text", "id": "item-2", "name": "Item2", "content": "Arashiyama", + "width": "fit_content", "height": "fit_content" } + ] + } + ] + } + ] + }) +} + +#[test] +fn duplicate_title_header_row_gets_repaired() { + let mut sink = sink_with_root(duplicate_title_section()); + let wrapper_id = find_by_name(sink.state.active_children(), "SectionWrapper") + .unwrap() + .id_str() + .to_string(); + + let repaired = repair_section_shell_fill_ownership(&mut sink, &wrapper_id); + assert_eq!(repaired, 1); + + let wrapper = find_by_name(sink.state.active_children(), "SectionWrapper").unwrap(); + assert_eq!( + child_names(wrapper), + vec!["HeaderRow", "ChecklistSection"], + "checklist promoted to be the header row's sibling, right after it" + ); + + let header_row = find_by_name(sink.state.active_children(), "HeaderRow").unwrap(); + assert_eq!( + child_names(header_row), + vec!["Title"], + "header row keeps only its own title" + ); + + let checklist = find_by_name(sink.state.active_children(), "ChecklistSection").unwrap(); + assert_eq!( + child_names(checklist), + vec!["Item1", "Item2"], + "the duplicate title inside the checklist is gone, real items untouched" + ); + assert!( + find_by_name(sink.state.active_children(), "DupTitle").is_none(), + "the duplicate title node itself must be deleted, not just unlinked" + ); +} + +#[test] +fn distinct_titles_are_left_untouched() { + // Same shape, but the checklist's first child is a DIFFERENT string — + // a legitimate "title, then a container whose first item happens to be + // text" pattern, not the duplicate-title bug. + let mut root = duplicate_title_section(); + root["children"][0]["children"][1]["children"][0]["content"] = json!("On track"); + let mut sink = sink_with_root(root); + let wrapper_id = find_by_name(sink.state.active_children(), "SectionWrapper") + .unwrap() + .id_str() + .to_string(); + + let repaired = repair_section_shell_fill_ownership(&mut sink, &wrapper_id); + assert_eq!(repaired, 0); + let header_row = find_by_name(sink.state.active_children(), "HeaderRow").unwrap(); + assert_eq!(child_names(header_row), vec!["Title", "ChecklistSection"]); +} + +#[test] +fn vertical_row_with_the_same_shape_is_untouched() { + // The bug is specifically about a HORIZONTAL row misnesting a vertical + // content body as a flex sibling — the same duplicate-text shape inside + // a vertical stack is an ordinary (if odd) authored structure, not this + // failure mode. + let mut root = duplicate_title_section(); + root["children"][0]["layout"] = json!("vertical"); + let mut sink = sink_with_root(root); + let wrapper_id = find_by_name(sink.state.active_children(), "SectionWrapper") + .unwrap() + .id_str() + .to_string(); + + let repaired = repair_section_shell_fill_ownership(&mut sink, &wrapper_id); + assert_eq!(repaired, 0); +} + +#[test] +fn three_child_row_is_untouched() { + // Scoped tight to the observed 2-child shape — a row with a third + // child (e.g. a trailing chevron icon) is not this failure mode. + let mut root = duplicate_title_section(); + root["children"][0]["children"] + .as_array_mut() + .unwrap() + .push( + json!({ "type": "icon_font", "id": "chevron", "name": "Chevron", + "iconFontName": "chevron-right", "width": 16, "height": 16 }), + ); + let mut sink = sink_with_root(root); + let wrapper_id = find_by_name(sink.state.active_children(), "SectionWrapper") + .unwrap() + .id_str() + .to_string(); + + let repaired = repair_section_shell_fill_ownership(&mut sink, &wrapper_id); + assert_eq!(repaired, 0); +} + +#[test] +fn repair_is_idempotent() { + let mut sink = sink_with_root(duplicate_title_section()); + let wrapper_id = find_by_name(sink.state.active_children(), "SectionWrapper") + .unwrap() + .id_str() + .to_string(); + + assert_eq!( + repair_section_shell_fill_ownership(&mut sink, &wrapper_id), + 1 + ); + assert_eq!( + repair_section_shell_fill_ownership(&mut sink, &wrapper_id), + 0, + "nothing left to fix on a second run" + ); +} + +/// End-to-end: the repair is wired into `run_cleanup_passes` and its +/// output survives the rest of the shared cleanup pipeline (no later pass +/// re-collapses the promoted checklist back into the header row). +#[test] +fn wired_into_run_cleanup_passes_end_to_end() { + let mut sink = sink_with_root(duplicate_title_section()); + let root_ids: Vec = sink + .state + .active_children() + .iter() + .map(|n| n.id_str().to_string()) + .collect(); + let root_id_refs: Vec<&str> = root_ids.iter().map(String::as_str).collect(); + let plan = crate::plan::OrchestratorPlan { + root_frame: crate::plan::RootFrameSpec { + id: "root".into(), + name: "Page".into(), + width: 390.0, + height: 844.0, + layout: None, + gap: None, + padding: None, + fill: None, + }, + subtasks: Vec::new(), + style_guide_name: None, + }; + crate::cleanup::run_cleanup_passes(&mut sink, &plan, &root_id_refs); + + let value = serde_json::to_value(sink.state.active_children()).expect("serialize"); + let title_count = value.to_string().matches("\"Must-See\"").count(); + assert_eq!( + title_count, 1, + "exactly one \"Must-See\" title must survive the full cleanup pipeline: {value}" + ); + let checklist = find_by_name(sink.state.active_children(), "ChecklistSection").unwrap(); + assert!( + child_names(checklist).contains(&"Item1".to_string()), + "checklist content survives cleanup: {value}" + ); +} diff --git a/crates/op-orchestrator/src/types.rs b/crates/op-orchestrator/src/types.rs index fdd4168b9..1c8ff8fc2 100644 --- a/crates/op-orchestrator/src/types.rs +++ b/crates/op-orchestrator/src/types.rs @@ -354,6 +354,16 @@ pub enum Progress { id: String, issue_count: usize, }, + /// Emitted ONCE, right before `RunSummary` is returned, whenever the + /// "promise-delivery" invariant check (`unfilled_screens::detect_unfilled_screens`) + /// finds a scaffolded screen that never received real content — the + /// classic-path honest report (postmortem 0718-1-glm-1: a silently + /// delivered blank screen). `names` are already marked on the canvas + /// (`" (unfilled)"` suffix) by the time this fires. Never emitted when + /// nothing is unfilled. + UnfilledScreens { + names: Vec, + }, /// Emitted after each sub-agent LLM reply is applied — lets the UI /// show a live node count while the subtask is still running. SubtaskNodes { @@ -519,6 +529,14 @@ pub struct RunSummary { pub root_frame_id: String, pub subtasks: Vec, pub total_nodes: usize, + /// Names of top-level "screen" roots (`unfilled_screens::detect_unfilled_screens`) + /// that never received real content by the time the run finished — the + /// "promise-delivery" invariant's classic-path honest report. Empty on + /// the common path. Each name is also already marked on the canvas + /// itself (`unfilled_screens::mark_unfilled_screens`'s " (unfilled)" + /// suffix) before this summary is built, so a caller that only reads + /// this field and one that only looks at the canvas see the same story. + pub unfilled_screens: Vec, } /// `run()` 的失败。 diff --git a/crates/op-orchestrator/src/unfilled_screens.rs b/crates/op-orchestrator/src/unfilled_screens.rs new file mode 100644 index 000000000..911528997 --- /dev/null +++ b/crates/op-orchestrator/src/unfilled_screens.rs @@ -0,0 +1,219 @@ +//! "Promise-delivery" invariant — a shared, deterministic check for a +//! top-level screen the model (or a scaffold/seed pass) promised but +//! never actually filled. +//! +//! Root-cause postmortem (0718-1-glm-1): a skeleton-first design run +//! planned 3 screens, scaffolded all 3 top-level roots up front, then ran +//! out of turn budget before ever touching the 3rd ("Saved") — which +//! shipped as a bare shell, silently, with no signal anywhere that +//! anything was missing. [`detect_unfilled_screens`] is the shared +//! detector both the agentic loop (`op-host-services::chat_agent_loop`, +//! via the host bridge) and the classic orchestrator (`run.rs`) call at +//! their own completion points; [`mark_unfilled_screens`] is the honest- +//! delivery side effect (renames each hit so the canvas shows it, not +//! just a log line). +//! +//! ## Screen candidates +//! +//! A top-level node is a "screen" candidate when it's a `frame` sized like a +//! mobile screen artboard (`MIN_SCREEN_WIDTH..=MAX_SCREEN_WIDTH` wide, at +//! least `MIN_SCREEN_HEIGHT` tall). The width/height bounds mirror the +//! `MOBILE_ROOT_MAX_WIDTH` convention `geometry_validation.rs` already uses +//! for mobile-root detection. This intentionally excludes desktop multi-root +//! layouts (a dashboard's separate sidebar + content roots are not +//! mobile-shaped) — those were never "promised screens" in the +//! skeleton-first sense this check exists for. +//! +//! Deliberately NOT keyed off `FrameNode.screen.is_some()`, even though the +//! spec's own framing ("带 screen 标记或屏形") suggested it as an OR-branch: +//! `wire_screen_navigation` (`cleanup::finalize_design`, which +//! `apply_loop_finalize`/the classic path's cleanup stage both run before +//! this detector ever sees the document) auto-tags EVERY top-level frame +//! with a `screen` path the moment a document has 2+ top-level roots — it is +//! App-Mode routing plumbing, not a "this was promised/scaffolded as a +//! screen" signal. Trusting it flagged a 1200×64 navbar `Header` and a blank +//! 1200×800 desktop canvas as "unfilled screens" in testing (both carried a +//! `screen` tag, neither is mobile-shaped). Per the parallel-work agreement +//! for this task (skeleton-structure internals are still moving under +//! `wire_screen_navigation.rs`), this stays a pure shape check for now — a +//! follow-up can revisit once that module's contract for "this frame IS a +//! promised screen" (vs. "this frame got auto-tagged for nav routing") +//! stabilizes. +//! +//! ## "Unfilled" — the false-positive risk this is tuned against +//! +//! The obvious criterion — "fewer than N nodes" — was rejected: a +//! genuinely minimal but REAL screen (a background + one heading + one +//! button, 3-4 nodes) must never be flagged just for being small +//! ("稀疏但有真内容不误伤" — sparse-but-real must not be false-flagged). +//! Instead [`has_real_content`] asks a content QUESTION, not a size +//! question: does this subtree contain any non-empty text, any image, or +//! any icon with a real glyph, ANYWHERE beneath it — outside the +//! auto-provided chrome (`role: "status-bar"` / `"bottom-tab-bar"`, which +//! every screen carries regardless of how much the model has done, so +//! their own text/icons must not count as "the model did something +//! here"). A screen with even ONE real content node, however sparse, +//! reads as filled; a screen with only chrome (or literally nothing) +//! reads as unfilled — no node-count threshold needed at all. + +use jian_ops_schema::node::PenNode; +use op_editor_core::{EditorState, PenNodeExt}; + +/// A top-level "screen" root that never received real content. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct UnfilledScreen { + pub node_id: String, + pub name: String, +} + +/// Roles that are auto-provided chrome, not model effort — present on +/// every screen regardless of how much of it the model actually built. +/// A screen carrying ONLY these subtrees is still, for the model's +/// purposes, unfilled. +const CHROME_ROLES: &[&str] = &["status-bar", "bottom-tab-bar"]; + +/// Mobile-artboard width band a screen-shaped root falls into, absent an +/// explicit `screen` tag — mirrors `geometry_validation.rs`'s +/// `MOBILE_ROOT_MAX_WIDTH` (480px) convention; `MIN_SCREEN_WIDTH` rules +/// out narrow desktop side-panels that are not full screens. +const MIN_SCREEN_WIDTH: f64 = 300.0; +const MAX_SCREEN_WIDTH: f64 = 480.0; +/// A screen-shaped root is also taller than it is a shallow strip — a +/// full mobile artboard, not a header band. +const MIN_SCREEN_HEIGHT: f64 = 480.0; + +/// Every top-level "screen" root in `state`, filled or not — the full +/// "committed" set the promise-delivery contract is judged against. Read-only. +/// Order matches document order (`state.active_children()`). +pub fn list_screen_candidates(state: &EditorState) -> Vec { + state + .active_children() + .iter() + .filter(|node| looks_like_a_screen(node)) + .map(screen_hit) + .collect() +} + +/// Detect every top-level "screen" root in `state` that has no real +/// content yet — the subset of [`list_screen_candidates`] still unfilled. +/// Read-only — never mutates the document. Order matches document order +/// (`state.active_children()`). +pub fn detect_unfilled_screens(state: &EditorState) -> Vec { + state + .active_children() + .iter() + .filter(|node| looks_like_a_screen(node)) + .filter(|node| !has_real_content(node)) + .map(screen_hit) + .collect() +} + +fn screen_hit(node: &PenNode) -> UnfilledScreen { + UnfilledScreen { + node_id: node.id_str().to_string(), + name: node + .base() + .name + .clone() + .filter(|n| !n.trim().is_empty()) + .unwrap_or_else(|| node.id_str().to_string()), + } +} + +/// Idempotently append " (unfilled)" to each hit's node name so the +/// canvas itself carries the signal, not just a transcript line — a +/// user scrolling the layer panel or the canvas must not have to +/// discover a hidden empty screen by clicking into it. Callers apply +/// this ONLY as a FINAL step (after any in-session nudge attempt has +/// already run its course) — marking before a nudge would falsely +/// brand a screen the model was about to fill in the very next turn. +pub fn mark_unfilled_screens(sink: &mut dyn crate::types::DocSink, hits: &[UnfilledScreen]) { + const SUFFIX: &str = " (unfilled)"; + for hit in hits { + let Some(node) = op_editor_core::walkers::find_node( + sink.state().active_children(), + &op_editor_core::NodeId::new(hit.node_id.clone()), + ) else { + continue; + }; + let current_name = node.base().name.clone().unwrap_or_default(); + if current_name.ends_with(SUFFIX) { + continue; + } + sink.apply(op_editor_core::EditorCommand::UpdateNode { + node_id: op_editor_core::NodeId::new(hit.node_id.clone()), + x: None, + y: None, + width: None, + height: None, + name: Some(format!("{current_name}{SUFFIX}")), + fill_hex: None, + page_id: None, + }); + } +} + +/// Detect, then mark, over an already-mutated `EditorState` (the loop +/// host's live document, post `apply_loop_finalize`). Returns the +/// (pre-mark) names for the caller to report. A thin combinator so the +/// host bridge (`op-host-desktop::chat_session`) doesn't need to build +/// its own [`crate::loop_finalize::StateDocSink`] wrapper. +pub fn finalize_and_mark_unfilled_screens(state: &mut EditorState) -> Vec { + let hits = detect_unfilled_screens(state); + if hits.is_empty() { + return Vec::new(); + } + let names = hits.iter().map(|h| h.name.clone()).collect(); + let mut sink = crate::loop_finalize::StateDocSink { state }; + mark_unfilled_screens(&mut sink, &hits); + names +} + +fn looks_like_a_screen(node: &PenNode) -> bool { + if !matches!(node, PenNode::Frame(_)) { + return false; + } + let (Some(w), Some(h)) = (node.width_px(), node.height_px()) else { + return false; + }; + (MIN_SCREEN_WIDTH..=MAX_SCREEN_WIDTH).contains(&w) && h >= MIN_SCREEN_HEIGHT +} + +/// Whether `node`'s OWN role marks it as auto-provided chrome — its +/// subtree is excluded from the content search below. +fn is_chrome_role(node: &PenNode) -> bool { + node.base() + .role + .as_deref() + .is_some_and(|r| CHROME_ROLES.contains(&r)) +} + +/// Whether `node` itself (ignoring descendants) is a real content leaf: +/// non-empty text, any image, or an icon with a real glyph name. +fn is_real_leaf_content(node: &PenNode) -> bool { + match node { + PenNode::Text(t) => match &t.content { + jian_ops_schema::node::text::TextContent::Plain(s) => !s.trim().is_empty(), + jian_ops_schema::node::text::TextContent::Styled(segments) => !segments.is_empty(), + }, + PenNode::Image(_) => true, + PenNode::IconFont(icon) => !icon.icon_font_name.trim().is_empty(), + _ => false, + } +} + +/// Whether `node`'s subtree (itself + descendants) contains any real +/// content, skipping auto-chrome subtrees entirely. +fn has_real_content(node: &PenNode) -> bool { + if is_chrome_role(node) { + return false; + } + if is_real_leaf_content(node) { + return true; + } + node.children().into_iter().flatten().any(has_real_content) +} + +#[cfg(test)] +#[path = "unfilled_screens_tests.rs"] +mod tests; diff --git a/crates/op-orchestrator/src/unfilled_screens_tests.rs b/crates/op-orchestrator/src/unfilled_screens_tests.rs new file mode 100644 index 000000000..b8ac7d6e3 --- /dev/null +++ b/crates/op-orchestrator/src/unfilled_screens_tests.rs @@ -0,0 +1,285 @@ +use super::*; +use crate::test_support::VecDocSink; +use crate::types::DocSink; +use serde_json::json; + +fn state_from(children: serde_json::Value) -> EditorState { + let doc: jian_ops_schema::PenDocument = serde_json::from_value(json!({ + "version": "1.0", + "children": children, + })) + .expect("doc"); + EditorState::from_document(doc) +} + +/// The exact auto-provided status-bar chrome shape — a status-bar-role +/// frame carrying real text (the clock) that must NOT, on its own, count +/// as "the model filled this screen". +fn status_bar_chrome(id_suffix: &str) -> serde_json::Value { + json!({ + "type": "frame", "id": format!("status-{id_suffix}"), "name": "Status Bar", + "role": "status-bar", "width": "fill_container", "height": 44, + "children": [ + { "type": "text", "id": format!("clock-{id_suffix}"), "name": "Time", "content": "9:41" } + ] + }) +} + +fn mobile_screen( + id: &str, + name: &str, + extra_children: Vec, +) -> serde_json::Value { + let mut children = vec![status_bar_chrome(id)]; + children.extend(extra_children); + json!({ + "type": "frame", "id": id, "name": name, "layout": "vertical", + "width": 390, "height": 844, "children": children + }) +} + +#[test] +fn a_screen_with_only_the_status_bar_is_unfilled() { + let state = state_from(json!([mobile_screen("root-a", "Saved", vec![])])); + let hits = detect_unfilled_screens(&state); + assert_eq!(hits.len(), 1); + assert_eq!(hits[0].node_id, "root-a"); + assert_eq!(hits[0].name, "Saved"); +} + +#[test] +fn a_screen_with_zero_children_at_all_is_unfilled() { + let state = state_from(json!([{ + "type": "frame", "id": "root-a", "name": "Blank", + "width": 390, "height": 844, "children": [] + }])); + let hits = detect_unfilled_screens(&state); + assert_eq!(hits.len(), 1); + assert_eq!(hits[0].name, "Blank"); +} + +#[test] +fn a_screen_with_real_content_is_not_flagged() { + let state = state_from(json!([mobile_screen( + "root-a", + "Home", + vec![json!({ + "type": "frame", "id": "body", "name": "Body", "layout": "vertical", + "width": "fill_container", "height": "fit_content", "children": [ + { "type": "text", "id": "greeting", "content": "Welcome back" } + ] + })] + )])); + assert!(detect_unfilled_screens(&state).is_empty()); +} + +/// Sparse but REAL content (a background + one heading, nothing else) +/// must never be false-flagged just for being small — the whole reason +/// a node-count threshold was rejected in favor of a content check. +#[test] +fn sparse_but_real_content_is_not_falsely_flagged() { + let state = state_from(json!([mobile_screen( + "root-a", + "Saved", + vec![json!({ "type": "text", "id": "only-label", "content": "No saved items yet" })] + )])); + assert!( + detect_unfilled_screens(&state).is_empty(), + "one real text node is enough to count as filled, however sparse the screen otherwise is" + ); +} + +/// An icon with a real glyph counts as real content too (an icon-led +/// empty state is still authored content, not a shell). +#[test] +fn an_icon_with_a_real_glyph_counts_as_real_content() { + let state = state_from(json!([mobile_screen( + "root-a", + "Saved", + vec![json!({ "type": "icon_font", "id": "empty-icon", "iconFontName": "bookmark" })] + )])); + assert!(detect_unfilled_screens(&state).is_empty()); +} + +/// A screen-shaped root with no chrome at all and only decorative, +/// content-free shapes (a background rectangle) is still unfilled. +#[test] +fn decorative_only_content_does_not_count_as_filled() { + let state = state_from(json!([mobile_screen( + "root-a", + "Saved", + vec![json!({ "type": "rectangle", "id": "bg", "width": 390, "height": 400 })] + )])); + assert_eq!( + detect_unfilled_screens(&state).len(), + 1, + "a bare decorative rectangle is not evidence the model did anything here" + ); +} + +/// A desktop-shaped root (not mobile-sized, no explicit `screen` tag) is +/// never a "promised screen" candidate, even if it's empty — dashboards +/// legitimately split sidebar + content into separate top-level roots. +#[test] +fn a_desktop_shaped_empty_root_is_not_a_screen_candidate() { + let state = state_from(json!([{ + "type": "frame", "id": "root-a", "name": "Sidebar", + "width": 240, "height": 900, "children": [] + }])); + assert!(detect_unfilled_screens(&state).is_empty()); +} + +/// A `screen`-tagged root that is mobile-shaped AND empty is still a +/// candidate — the tag doesn't disqualify a genuine mobile screen, it's just +/// no longer the thing that QUALIFIES one (see the module doc's "screen +/// candidates" section for why the tag alone is untrustworthy). +#[test] +fn a_screen_tagged_mobile_shaped_root_is_still_a_candidate_via_shape() { + let state = state_from(json!([{ + "type": "frame", "id": "root-a", "name": "Checkout", + "screen": "/checkout", "width": 390, "height": 844, "children": [] + }])); + let hits = detect_unfilled_screens(&state); + assert_eq!(hits.len(), 1); + assert_eq!(hits[0].name, "Checkout"); +} + +/// A screen-shaped root that is BOTH `screen`-tagged AND has real +/// content is not flagged. +#[test] +fn a_filled_explicitly_tagged_screen_is_not_flagged() { + let state = state_from(json!([{ + "type": "frame", "id": "root-a", "name": "Checkout", "screen": "/checkout", + "width": 390, "height": 844, "children": [ + { "type": "text", "id": "t", "content": "Order summary" } + ] + }])); + assert!(detect_unfilled_screens(&state).is_empty()); +} + +/// Regression (found live in `pump_runs_loop_finalize_backstop_against_live_state`, +/// op-host-desktop): `wire_screen_navigation` (run inside +/// `cleanup::finalize_design`, which both `apply_loop_finalize` and the +/// classic path's cleanup stage run BEFORE this detector ever sees the +/// document) auto-tags EVERY top-level frame with a `screen` path once a +/// document has 2+ top-level roots — including a 1200×64 navbar `Header` +/// fragment and a blank 1200×800 desktop canvas, neither of which is a +/// "promised screen" in any meaningful sense. A `screen` tag alone must +/// never make a non-mobile-shaped frame a candidate. +#[test] +fn a_screen_tagged_but_non_mobile_shaped_root_is_never_a_candidate() { + let state = state_from(json!([ + { "type": "frame", "id": "root-a", "name": "Frame", "screen": "/", "width": 1200, "height": 800, "children": [] }, + { "type": "frame", "id": "root-b", "name": "Header", "role": "navbar", "screen": "/header", "width": 1200, "height": 64, "children": [] }, + ])); + assert!( + detect_unfilled_screens(&state).is_empty(), + "a nav-routing `screen` tag on a desktop-shaped frame must not, by itself, flag it as an unfilled screen" + ); +} + +/// `list_screen_candidates` returns every committed screen — filled or not +/// — used to build the "you committed N screens (A/B/C)" contract line the +/// in-session fill-round nudge states. +#[test] +fn list_screen_candidates_includes_both_filled_and_unfilled_screens() { + let state = state_from(json!([ + mobile_screen( + "root-a", + "Trips", + vec![json!({"type":"text","id":"t1","content":"hi"})] + ), + mobile_screen("root-b", "Destination", vec![]), + mobile_screen("root-c", "Saved", vec![]), + ])); + let candidates = list_screen_candidates(&state); + let names: Vec<&str> = candidates.iter().map(|h| h.name.as_str()).collect(); + assert_eq!(names, vec!["Trips", "Destination", "Saved"]); +} + +/// A desktop-shaped / non-screen-shaped root is never a candidate here +/// either — `list_screen_candidates` shares `looks_like_a_screen` with +/// `detect_unfilled_screens`, so the same shape rule applies to both. +#[test] +fn list_screen_candidates_excludes_non_mobile_shaped_roots() { + let state = state_from(json!([{ + "type": "frame", "id": "root-a", "name": "Sidebar", + "width": 240, "height": 900, "children": [] + }])); + assert!(list_screen_candidates(&state).is_empty()); +} + +/// Multiple unfilled screens are all reported, in document order. +#[test] +fn multiple_unfilled_screens_are_all_reported() { + let state = state_from(json!([ + mobile_screen( + "root-a", + "Home", + vec![json!({"type":"text","id":"t1","content":"hi"})] + ), + mobile_screen("root-b", "Saved", vec![]), + mobile_screen("root-c", "Profile", vec![]), + ])); + let hits = detect_unfilled_screens(&state); + let names: Vec<&str> = hits.iter().map(|h| h.name.as_str()).collect(); + assert_eq!(names, vec!["Saved", "Profile"]); +} + +#[test] +fn mark_unfilled_screens_appends_the_suffix_once() { + let mut sink = VecDocSink { + state: state_from(json!([mobile_screen("root-a", "Saved", vec![])])), + applied: Vec::new(), + batch_depth: 0, + }; + let hits = detect_unfilled_screens(sink.state()); + mark_unfilled_screens(&mut sink, &hits); + let node = op_editor_core::walkers::find_node( + sink.state().active_children(), + &op_editor_core::NodeId::new("root-a"), + ) + .expect("root still present"); + assert_eq!(node.base().name.as_deref(), Some("Saved (unfilled)")); + + // Idempotent: marking an already-marked screen again must not + // double the suffix. + let hits_again = detect_unfilled_screens(sink.state()); + mark_unfilled_screens(&mut sink, &hits_again); + let node = op_editor_core::walkers::find_node( + sink.state().active_children(), + &op_editor_core::NodeId::new("root-a"), + ) + .expect("root still present"); + assert_eq!(node.base().name.as_deref(), Some("Saved (unfilled)")); +} + +#[test] +fn finalize_and_mark_unfilled_screens_returns_names_and_marks_the_canvas() { + let mut state = state_from(json!([mobile_screen("root-a", "Saved", vec![])])); + let names = finalize_and_mark_unfilled_screens(&mut state); + assert_eq!(names, vec!["Saved".to_string()]); + let node = op_editor_core::walkers::find_node( + state.active_children(), + &op_editor_core::NodeId::new("root-a"), + ) + .expect("root still present"); + assert_eq!(node.base().name.as_deref(), Some("Saved (unfilled)")); +} + +#[test] +fn finalize_and_mark_is_a_true_no_op_when_nothing_is_unfilled() { + let mut state = state_from(json!([mobile_screen( + "root-a", + "Home", + vec![json!({"type":"text","id":"t","content":"hi"})] + )])); + let before = serde_json::to_value(&state.doc).expect("serialize before"); + let names = finalize_and_mark_unfilled_screens(&mut state); + assert!(names.is_empty()); + let after = serde_json::to_value(&state.doc).expect("serialize after"); + assert_eq!( + after, before, + "must not touch the document when nothing is unfilled" + ); +} diff --git a/crates/op-orchestrator/src/unify_shared_nav.rs b/crates/op-orchestrator/src/unify_shared_nav.rs index bf7842655..5f0fbe0aa 100644 --- a/crates/op-orchestrator/src/unify_shared_nav.rs +++ b/crates/op-orchestrator/src/unify_shared_nav.rs @@ -264,10 +264,8 @@ fn retarget_active_tab(nav: &mut PenNode, screen_name: &str) { /// should become active) and [`resolve_target`] (decide whether a nav-less /// screen is eligible for injection) — one matching rule, one place. fn find_tab_index_for_screen(children: &[PenNode], screen_name: &str) -> Option { - let target_key = normalize_label(screen_name); children.iter().position(|tab| { - first_text_content(tab) - .is_some_and(|label| labels_match(&normalize_label(label), &target_key)) + first_text_content(tab).is_some_and(|label| labels_match(label, screen_name)) }) } diff --git a/crates/op-orchestrator/src/wire_screen_navigation.rs b/crates/op-orchestrator/src/wire_screen_navigation.rs index 220f841b0..ddd5c8747 100644 --- a/crates/op-orchestrator/src/wire_screen_navigation.rs +++ b/crates/op-orchestrator/src/wire_screen_navigation.rs @@ -260,10 +260,13 @@ fn wire_nav_tabs( screens: &[ScreenCandidate], screen_paths: &[(String, String)], ) { - let normalized_screens: Vec<(String, String)> = screen_paths + // Raw (un-normalized) names — `labels_match` normalizes + tokenizes + // internally, so a brand-prefixed screen name ("Wander — Trips") still + // matches a bare tab label ("Trips"). + let screen_names: Vec<(&str, &str)> = screen_paths .iter() .filter(|(_, path)| !path.is_empty()) - .map(|(name, path)| (normalize_label(name), path.clone())) + .map(|(name, path)| (name.as_str(), path.as_str())) .collect(); let mut patches: Vec<(String, String)> = Vec::new(); @@ -287,11 +290,10 @@ fn wire_nav_tabs( let Some(label) = first_text_content(item) else { continue; }; - let tab_key = normalize_label(label); - let matched_path = normalized_screens + let matched_path = screen_names .iter() - .find(|(screen_key, _)| labels_match(&tab_key, screen_key)) - .map(|(_, path)| path.clone()); + .find(|(screen_name, _)| labels_match(label, screen_name)) + .map(|(_, path)| path.to_string()); let Some(path) = matched_path else { continue; }; @@ -390,13 +392,43 @@ pub(crate) fn normalize_label(s: &str) -> String { .collect() } -/// Match iff normalized forms are equal or one is a prefix of the other -/// (covers "Profile" vs "Profile Screen" → "profile" / "profilescreen"). +/// Split a label/screen name on the separators an authored app commonly +/// uses for a brand prefix, a hyphenated subtitle, or a breadcrumb (space, +/// em dash, en dash, hyphen, colon, middle dot, pipe) into normalized, +/// non-empty word tokens. "Wander — Trips" → `["wander", "trips"]`. +fn label_tokens(s: &str) -> Vec { + s.split([' ', '—', '–', '-', ':', '·', '|']) + .map(normalize_label) + .filter(|t| !t.is_empty()) + .collect() +} + +/// Match iff the fully-normalized forms are equal or one is a prefix of the +/// other (covers "Profile" vs "Profile Screen" → "profile" / +/// "profilescreen") — OR, failing that, the two labels share a whole word +/// TOKEN once split on the usual separators (covers a brand-prefixed screen +/// name, "Wander — Trips", matching a bare tab label "Trips": neither +/// normalized form is a prefix of the other — "trips" vs "wandertrips" — +/// but "trips" is a token of both). Token matching compares WHOLE tokens +/// only, never a bare substring — "Roadtrips" tokenizes to `["roadtrips"]`, +/// which never equals the "trips" token, so it correctly stays unmatched. /// Ambiguous (neither) never binds — a wrong navigate is worse than a dead -/// tap. `pub(crate)` for the `navIssues` echo scan — see +/// tap. Takes RAW (un-normalized) label/name text — both checks normalize +/// internally, so every caller passes its text straight through instead of +/// pre-normalizing (pre-normalizing would destroy the word boundaries the +/// token check needs). `pub(crate)` for the `navIssues` echo scan — see /// [`collect_nav_containers`]. pub(crate) fn labels_match(a: &str, b: &str) -> bool { - !a.is_empty() && !b.is_empty() && (a == b || a.starts_with(b) || b.starts_with(a)) + let (na, nb) = (normalize_label(a), normalize_label(b)); + if na.is_empty() || nb.is_empty() { + return false; + } + if na == nb || na.starts_with(&nb) || nb.starts_with(&na) { + return true; + } + let ta = label_tokens(a); + let tb = label_tokens(b); + ta.iter().any(|t| tb.contains(t)) } // ── Back-button wiring ────────────────────────────────────────────────── diff --git a/crates/op-orchestrator/src/wire_screen_navigation_tests.rs b/crates/op-orchestrator/src/wire_screen_navigation_tests.rs index f079892a9..a88f2510f 100644 --- a/crates/op-orchestrator/src/wire_screen_navigation_tests.rs +++ b/crates/op-orchestrator/src/wire_screen_navigation_tests.rs @@ -57,6 +57,34 @@ fn labels_match_exact_and_prefix() { assert!(!labels_match("", "profile")); } +/// Measured (`0718-1-glm-1.op`): screen names carry a brand prefix +/// ("Wander — Trips" / "Wander — Saved") — neither normalized form is a +/// prefix of the other ("trips" vs "wandertrips"), so every one of the +/// document's nav tabs (bare "Trips" / "Saved" labels) went unbound. The +/// token fallback must recover this WITHOUT opening the door to a bare +/// substring match. +#[test] +fn labels_match_token_fallback_for_brand_prefixed_names() { + // The exact failure this was built from: brand-prefixed screen name, + // bare tab label, either argument order. + assert!(labels_match("Trips", "Wander — Trips")); + assert!(labels_match("Wander — Trips", "Trips")); + assert!(labels_match("Saved", "Wander — Saved")); + + // A bare SUBSTRING must never count as a token — "Roadtrips" tokenizes + // to a single ["roadtrips"] token, which never equals "trips". + assert!(!labels_match("Trips", "Roadtrips")); + assert!(!labels_match("Trips", "Wander — Roadtrips")); + + // Multi-word tab label: "Your Library" should match a brand-prefixed + // screen sharing either whole word as a token. + assert!(labels_match("Your Library", "Wander — Library")); + assert!(labels_match("Library", "Wander — Your Library")); + + // Still ambiguous when nothing at all is shared. + assert!(!labels_match("Explore", "Wander — Trips")); +} + /// Contract point 1: the navigate body must be a Tier-1 string-LITERAL /// expression source — a bare `/path` lexes as division and fails to /// compile (`Expression::compile`). The correct wire form is exactly the @@ -287,6 +315,72 @@ fn mismatched_tab_label_is_not_bound() { assert!(node_events_json(settings_tab).is_none()); } +/// De-identified reproduction of `0718-1-glm-1.op`: three screens already +/// spread apart (x = 0 / 430 / 860, non-overlapping), each brand-prefixed +/// ("Wander — Trips" etc.), one bottom nav with bare tab labels ("Trips" / +/// "Saved" / "Explore" / "Profile" — the last two have no matching screen +/// in this 3-screen document, matching the real file). Before the token +/// fallback this bound ZERO tabs. +fn brand_prefixed_three_screen_doc() -> &'static str { + r#"{"version":"1.0","children":[ + {"type":"frame","id":"trips","name":"Wander — Trips","x":0,"y":0,"width":390,"height":844, + "layout":"vertical","children":[ + {"type":"frame","id":"nav-trips","name":"Tab Bar","role":"bottom-tab-bar", + "width":"fill_container","height":72,"layout":"horizontal","children":[ + {"type":"frame","id":"tab-trips","width":80,"height":40, + "children":[{"type":"text","id":"t1","content":"Trips"}]}, + {"type":"frame","id":"tab-saved","width":80,"height":40, + "children":[{"type":"text","id":"t2","content":"Saved"}]}, + {"type":"frame","id":"tab-explore","width":80,"height":40, + "children":[{"type":"text","id":"t3","content":"Explore"}]}, + {"type":"frame","id":"tab-profile","width":80,"height":40, + "children":[{"type":"text","id":"t4","content":"Profile"}]} + ]} + ]}, + {"type":"frame","id":"destination","name":"Wander — Destination","x":430,"y":0,"width":390,"height":844, + "layout":"vertical","children":[]}, + {"type":"frame","id":"saved","name":"Wander — Saved","x":860,"y":0,"width":390,"height":844, + "layout":"vertical","children":[]} + ]}"# +} + +#[test] +fn brand_prefixed_screen_names_get_their_matching_tabs_bound() { + let mut state = state_from_json(brand_prefixed_three_screen_doc()); + run_pass(&mut state); + + let trips_path = frame_screen(find_by_id(state.active_children(), "trips").unwrap()) + .unwrap() + .to_string(); + let saved_path = frame_screen(find_by_id(state.active_children(), "saved").unwrap()) + .unwrap() + .to_string(); + + let trips_tab = find_by_id(state.active_children(), "tab-trips").unwrap(); + assert_eq!( + node_events_json(trips_tab).unwrap()["onTap"][0]["replace"], + serde_json::json!(format!("\"{trips_path}\"")), + "\"Trips\" must bind to \"Wander — Trips\" via the token fallback" + ); + let saved_tab = find_by_id(state.active_children(), "tab-saved").unwrap(); + assert_eq!( + node_events_json(saved_tab).unwrap()["onTap"][0]["replace"], + serde_json::json!(format!("\"{saved_path}\"")), + "\"Saved\" must bind to \"Wander — Saved\" via the token fallback" + ); + + // "Explore" / "Profile" have no matching screen in this 3-screen + // document (same as the real file) — must stay unbound, not guess. + let explore_tab = find_by_id(state.active_children(), "tab-explore").unwrap(); + assert!(node_events_json(explore_tab).is_none()); + let profile_tab = find_by_id(state.active_children(), "tab-profile").unwrap(); + assert!(node_events_json(profile_tab).is_none()); + + // Destination (a push-style detail screen, no bottom nav of its own) + // still gets tagged even though it has nothing to bind. + assert!(frame_screen(find_by_id(state.active_children(), "destination").unwrap()).is_some()); +} + #[test] fn existing_tab_events_are_left_alone() { let json = r#"{"version":"1.0","children":[ diff --git a/crates/op-smoke/src/audit_rubric.rs b/crates/op-smoke/src/audit_rubric.rs index 4b49ae70c..567d3209e 100644 --- a/crates/op-smoke/src/audit_rubric.rs +++ b/crates/op-smoke/src/audit_rubric.rs @@ -490,6 +490,7 @@ mod tests { root_frame_id: "root".to_string(), total_nodes: subtasks.iter().map(|s| s.node_count).sum(), subtasks, + unfilled_screens: Vec::new(), } } diff --git a/crates/op-smoke/src/loop_mode.rs b/crates/op-smoke/src/loop_mode.rs index 504cf32b6..bd4a7c2fe 100644 --- a/crates/op-smoke/src/loop_mode.rs +++ b/crates/op-smoke/src/loop_mode.rs @@ -8,8 +8,9 @@ //! (`batch_design` / `get_screenshot` / …) over a real //! `/chat/completions` SSE stream, the host executes each call against a //! live `EditorState`, and the result rides a follow-up request — looping -//! until the model stops calling tools or the production turn cap -//! (`MAX_TOOL_TURNS = 20`) is reached. +//! until the model stops calling tools or the production design-loop turn +//! cap (`DESIGN_LOOP_MAX_TURNS = 28`, since `build_design_provider` below +//! chains `.with_loop_finalize()`) is reached. //! //! ## What is reused vs. new //! @@ -18,7 +19,7 @@ //! //! - **Loop transport** — `ConfiguredBuiltinProvider::send()` //! (`op_host_services::chat_builtin_http`) which internally builds an -//! `AgentLoopConfig { max_turns: MAX_TOOL_TURNS, … }` and runs +//! `AgentLoopConfig { max_turns: DESIGN_LOOP_MAX_TURNS, … }` and runs //! `run_openai_agent_loop` for the `OpenAiCompat` kind, spawned on the //! crate's global `shared_runtime()`. `send()` returns a *blocking* //! iterator; draining it to exhaustion blocks the caller until the loop @@ -55,7 +56,7 @@ use std::sync::{Arc, Mutex}; use op_ai::chat_provider::{ ChatDelta, ChatProvider, ChatRequest, ChatToolExecutor, ChatToolResult, EffortLevel, - ThinkingMode, + ThinkingMode, UnfilledScreensReport, }; use op_editor_core::{BuiltinAgentConfig, BuiltinAgentKind, BuiltinAgentPresetKey, EditorState}; use op_host_services::chat_builtin_http::ConfiguredBuiltinProvider; @@ -150,14 +151,46 @@ impl ChatToolExecutor for HeadlessExecutor { result } - fn finalize(&self) { + fn finalize(&self) -> UnfilledScreensReport { let mut state = self .state .lock() .expect("EditorState mutex poisoned before finalize"); op_orchestrator::apply_loop_finalize(&mut state); + let unfilled = + op_orchestrator::unfilled_screens::finalize_and_mark_unfilled_screens(&mut state); + let committed = op_orchestrator::unfilled_screens::list_screen_candidates(&state) + .into_iter() + .map(|hit| hit.name) + .collect::>(); if self.dump { eprintln!("[LOOP] finalize: apply_loop_finalize ran (Class-A structural backstop)"); + if !unfilled.is_empty() { + eprintln!("[LOOP] finalize: unfilled screens left unfilled: {unfilled:?}"); + } + } + UnfilledScreensReport { + committed, + unfilled, + } + } + + fn check_unfilled_screens(&self) -> UnfilledScreensReport { + let state = self + .state + .lock() + .expect("EditorState mutex poisoned before check_unfilled_screens"); + let unfilled = op_orchestrator::unfilled_screens::detect_unfilled_screens(&state) + .into_iter() + .map(|hit| hit.name) + .collect(); + let committed = op_orchestrator::unfilled_screens::list_screen_candidates(&state) + .into_iter() + .map(|hit| hit.name) + .collect(); + UnfilledScreensReport { + committed, + unfilled, } } } @@ -293,7 +326,7 @@ pub fn run_loop( eprintln!( "[LOOP] starting agentic design loop (max_turns={})", - op_host_services::chat_canvas_tools::MAX_TOOL_TURNS + op_host_services::chat_builtin_http::DESIGN_LOOP_MAX_TURNS ); // `send()` blocks the iterating thread until the loop ends. Drain every