fix(agent): honor multi-screen commitments across the design loop
One autopsy, four gaps closed. Tab labels now match brand-prefixed screen names by whole tokens (Wander - Trips binds its Trips tab) across the wiring pass, the nav echo, and nav unification. A repair pass lifts content that a fill step mis-nested into a section header row and drops the duplicated title. Screens the skeleton committed but never filled are detected shape-first (screen tags alone over-fire on navbars), retried through a dedicated fill budget that does not consume the general turn cap - the cap guards runaway chatter, never committed work - nudged with the full commitment contract, and honestly reported in the transcript, the run summary, and an (unfilled) canvas label when they still come up empty. The loop's termination surface is now documented: the 28-turn design cap, the previously unchecked model self-stop, and the token/retry/abort exits.
This commit is contained in:
Fini
parent
9fea5557a9
commit
640dc73a43
|
|
@ -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<String>,
|
||||
pub unfilled: Vec<String>,
|
||||
}
|
||||
|
||||
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
|
||||
|
|
|
|||
|
|
@ -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::<serde_json::Value>(&result.content) else {
|
||||
return UnfilledScreensReport::default();
|
||||
};
|
||||
let names = |key: &str| {
|
||||
v.get(key)
|
||||
.cloned()
|
||||
.and_then(|v| serde_json::from_value::<Vec<String>>(v).ok())
|
||||
.unwrap_or_default()
|
||||
};
|
||||
UnfilledScreensReport {
|
||||
committed: names("committed"),
|
||||
unfilled: names("unfilled"),
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -88,6 +88,7 @@ fn design_session_drains_progress_and_command_requests() {
|
|||
subtask: None,
|
||||
}],
|
||||
total_nodes: 2,
|
||||
unfilled_screens: Vec::new(),
|
||||
})))
|
||||
.expect("done");
|
||||
|
||||
|
|
|
|||
|
|
@ -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::<serde_json::Value>(&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::<Vec<_>>()
|
||||
} 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::<Vec<_>>();
|
||||
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;
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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<dyn ChatToolExecutor>, 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<dyn ChatToolExecutor>,
|
||||
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<dyn ChatToolExecutor>,
|
||||
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<String, usize>) -> Vec<String> {
|
||||
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<String, usize>, 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<ChatDelta>, 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<String, usize> = 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<Value> = 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<String, usize> = 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<Value> = 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)]
|
||||
|
|
|
|||
|
|
@ -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<Vec<(String, String)>>,
|
||||
finalize_calls: std::sync::atomic::AtomicUsize,
|
||||
unfilled_check_calls: std::sync::atomic::AtomicUsize,
|
||||
results: Mutex<VecDeque<ChatToolResult>>,
|
||||
/// 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<VecDeque<UnfilledScreensReport>>,
|
||||
/// Scripted `finalize` return values, popped the same way.
|
||||
unfilled_finalizes: Mutex<VecDeque<UnfilledScreensReport>>,
|
||||
}
|
||||
|
||||
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<Self>, committed: &[&str], unfilled: &[&str]) -> Arc<Self> {
|
||||
self.unfilled_checks
|
||||
.lock()
|
||||
.unwrap()
|
||||
.push_back(report_of(committed, unfilled));
|
||||
self
|
||||
}
|
||||
|
||||
/// Queue one scripted `finalize` return value.
|
||||
fn with_unfilled_finalize(self: Arc<Self>, committed: &[&str], unfilled: &[&str]) -> Arc<Self> {
|
||||
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"}"#);
|
||||
|
|
|
|||
|
|
@ -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(", ")
|
||||
)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
||||
|
|
|
|||
|
|
@ -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<String> {
|
|||
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<String> {
|
|||
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;
|
||||
};
|
||||
|
|
|
|||
|
|
@ -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<String> = 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,
|
||||
})
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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!(
|
||||
|
|
|
|||
175
crates/op-orchestrator/src/section_shell_fill_repair.rs
Normal file
175
crates/op-orchestrator/src/section_shell_fill_repair.rs
Normal file
|
|
@ -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<usize>,
|
||||
cmds: &mut Vec<EditorCommand>,
|
||||
) {
|
||||
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<DuplicateTitleFix> {
|
||||
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;
|
||||
230
crates/op-orchestrator/src/section_shell_fill_repair_tests.rs
Normal file
230
crates/op-orchestrator/src/section_shell_fill_repair_tests.rs
Normal file
|
|
@ -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<String> {
|
||||
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<String> = 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}"
|
||||
);
|
||||
}
|
||||
|
|
@ -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<String>,
|
||||
},
|
||||
/// 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<SubtaskOutcome>,
|
||||
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<String>,
|
||||
}
|
||||
|
||||
/// `run()` 的失败。
|
||||
|
|
|
|||
219
crates/op-orchestrator/src/unfilled_screens.rs
Normal file
219
crates/op-orchestrator/src/unfilled_screens.rs
Normal file
|
|
@ -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<UnfilledScreen> {
|
||||
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<UnfilledScreen> {
|
||||
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<String> {
|
||||
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;
|
||||
285
crates/op-orchestrator/src/unfilled_screens_tests.rs
Normal file
285
crates/op-orchestrator/src/unfilled_screens_tests.rs
Normal file
|
|
@ -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>,
|
||||
) -> 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"
|
||||
);
|
||||
}
|
||||
|
|
@ -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<usize> {
|
||||
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))
|
||||
})
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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<String> {
|
||||
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 ──────────────────────────────────────────────────
|
||||
|
|
|
|||
|
|
@ -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":[
|
||||
|
|
|
|||
|
|
@ -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(),
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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::<Vec<_>>();
|
||||
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
|
||||
|
|
|
|||
Loading…
Reference in a new issue