From 396f75eb4214bdf80b986000af96e72289956dcc Mon Sep 17 00:00:00 2001 From: Fini Date: Thu, 2 Jul 2026 21:21:43 +0800 Subject: [PATCH] feat(orchestrator): structured generation protocols Add a parent-by-reference program DSL and an executable-script protocol with best-effort per-line parsing, so a truncated or malformed line drops only that op instead of the whole design. Route protocol selection in prompt/subagent and normalize flex keywords in parse. Registers the new modules in lib. --- crates/op-orchestrator/src/lib.rs | 2 + crates/op-orchestrator/src/parse.rs | 23 ++- crates/op-orchestrator/src/program_gen.rs | 70 ++++++- .../op-orchestrator/src/program_gen_tests.rs | 71 ++++++- crates/op-orchestrator/src/prompt.rs | 67 ++++++- crates/op-orchestrator/src/prompt_tests.rs | 10 +- crates/op-orchestrator/src/script_gen.rs | 188 ++++++++++++++++++ .../op-orchestrator/src/script_gen_tests.rs | 188 ++++++++++++++++++ crates/op-orchestrator/src/subagent.rs | 22 +- 9 files changed, 612 insertions(+), 29 deletions(-) create mode 100644 crates/op-orchestrator/src/script_gen.rs create mode 100644 crates/op-orchestrator/src/script_gen_tests.rs diff --git a/crates/op-orchestrator/src/lib.rs b/crates/op-orchestrator/src/lib.rs index 9d0191427..862cd6328 100644 --- a/crates/op-orchestrator/src/lib.rs +++ b/crates/op-orchestrator/src/lib.rs @@ -29,6 +29,7 @@ pub mod plan_normalize; pub mod plan_repair; pub mod program_gen; pub mod retry; +pub mod script_gen; pub mod semantic_palette; pub mod stub_providers; pub mod style_guide_context; @@ -48,6 +49,7 @@ pub mod cleanup; pub(crate) mod cleanup_layout; pub(crate) mod cleanup_typography; pub mod concurrent; +pub mod geometry_validation; pub mod loop_finalize; pub mod prompt; pub mod role_defaults; diff --git a/crates/op-orchestrator/src/parse.rs b/crates/op-orchestrator/src/parse.rs index 5218ff054..721c6ece5 100644 --- a/crates/op-orchestrator/src/parse.rs +++ b/crates/op-orchestrator/src/parse.rs @@ -368,6 +368,11 @@ fn normalize_layout_enum_json(object: &mut serde_json::Map Option<&'static str> { @@ -381,9 +386,9 @@ fn normalize_layout_mode(value: &str) -> Option<&'static str> { fn normalize_justify_content(value: &str) -> Option<&'static str> { match value.trim().to_ascii_lowercase().as_str() { - "start" | "flex-start" | "left" | "top" => Some("start"), + "start" | "flex-start" | "flex_start" | "flexstart" | "left" | "top" => Some("start"), "center" | "middle" => Some("center"), - "end" | "flex-end" | "right" | "bottom" => Some("end"), + "end" | "flex-end" | "flex_end" | "flexend" | "right" | "bottom" => Some("end"), "space_between" | "space-between" | "space between" => Some("space_between"), "space_around" | "space-around" | "space around" => Some("space_around"), "space_evenly" | "space-evenly" | "space evenly" => Some("space_around"), @@ -391,6 +396,20 @@ fn normalize_justify_content(value: &str) -> Option<&'static str> { } } +/// `alignItems` accepts `start`/`center`/`end`/`stretch`. A CSS-fluent model +/// writes `flex-start`/`flex_start` (and `flex-end`/`flex_end`); without this +/// the WHOLE node fails to deserialize and is dropped (the flat-JSONL twin of +/// the `normalize_justify_content` fix). `stretch` is a valid schema value — +/// pass it through unchanged. +fn normalize_align_items(value: &str) -> Option<&'static str> { + match value.trim().to_ascii_lowercase().as_str() { + "flex-start" | "flex_start" | "flexstart" | "left" | "top" => Some("start"), + "center" | "middle" => Some("center"), + "flex-end" | "flex_end" | "flexend" | "right" | "bottom" => Some("end"), + _ => None, + } +} + fn normalize_stroke_json(object: &mut serde_json::Map) { let stroke_width = object .remove("strokeWidth") diff --git a/crates/op-orchestrator/src/program_gen.rs b/crates/op-orchestrator/src/program_gen.rs index 8fcd906fc..9310f04d7 100644 --- a/crates/op-orchestrator/src/program_gen.rs +++ b/crates/op-orchestrator/src/program_gen.rs @@ -12,21 +12,61 @@ //! //! Validated 2026-06-29 across glm-5.2 / minimax-m3 / deepseek on dashboard + //! e-commerce + mobile screens (see openpencil-docs/pencil-generation-way). -//! This is gated OFF by default so it can be A/B'd against the JSONL path on the -//! corpus before becoming the default generation protocol. +//! This is the DEFAULT generation protocol for the open / Chinese reasoning +//! models (see [`program_gen_enabled_for_model`]); the executable-JS script +//! path is now an env-gated opt-in, because its all-or-nothing parse flattened +//! tables whenever a weak model truncated the script mid-op. use std::collections::BTreeMap; use jian_ops_schema::node::PenNode; use op_editor_core::EditorState; -/// Env gate, mirroring [`crate::manifest::manifest_enabled_for_model`]. The -/// `model` is accepted for symmetry / future per-model rollout; today the -/// process-global `OPENPENCIL_PROGRAM_GEN` decides for every model. -pub fn program_gen_enabled_for_model(_model: &str) -> bool { - std::env::var("OPENPENCIL_PROGRAM_GEN") - .map(|v| matches!(v.trim(), "1" | "true" | "TRUE" | "on")) - .unwrap_or(false) +/// Whether the sub-agent should emit a `batch_design` DSL PROGRAM for `model`. +/// +/// Resolution order: +/// 1. `OPENPENCIL_PROGRAM_GEN` set → honor it verbatim (force on/off for A-B). +/// 2. Another protocol explicitly opted into (`OPENPENCIL_SCRIPT_GEN` / +/// `OPENPENCIL_MANIFEST`) → defer to it (program stays off). +/// 3. Otherwise DEFAULT ON for the open / Chinese reasoning models and OFF for +/// Claude / GPT / Gemini / o-series (which emit clean flat JSONL natively). +/// +/// This is the weak-model default because it is both structurally safer and +/// more truncation-resilient than the alternatives — see the module docs and +/// [`crate::script_gen::script_gen_enabled_for_model`]. +pub fn program_gen_enabled_for_model(model: &str) -> bool { + if let Ok(v) = std::env::var("OPENPENCIL_PROGRAM_GEN") { + return matches!(v.trim(), "1" | "true" | "TRUE" | "on"); + } + // A different generation protocol was explicitly requested — don't shadow it. + if std::env::var("OPENPENCIL_SCRIPT_GEN").is_ok() + || std::env::var("OPENPENCIL_MANIFEST").is_ok() + { + return false; + } + default_program_gen_for_model(model) +} + +/// Family default (no env override): ON for the open / Chinese reasoning models, +/// OFF for Claude / GPT / Gemini / o-series (which handle flat JSONL natively) +/// and for an empty/unknown-but-flat-native id. +fn default_program_gen_for_model(model: &str) -> bool { + let normalized = match model.find('/') { + Some(i) => &model[i + 1..], + None => model, + }; + let lower = normalized.to_lowercase(); + if lower.is_empty() { + return false; + } + let flat_jsonl_native = lower.contains("claude") + || lower.contains("gpt-") + || lower.contains("gpt4") + || lower.contains("gemini") + || lower.starts_with("o1") + || lower.starts_with("o3") + || lower.starts_with("o4"); + !flat_jsonl_native } /// Run the emitted `batch_design` program against a FRESH empty document and @@ -40,9 +80,19 @@ pub fn parse_program(text: &str) -> Result, String> { if program.trim().is_empty() { return Err("program is empty after stripping prose/fences".into()); } + run_program_to_forest(&program) +} + +/// Run a `batch_design` DSL PROGRAM string against a FRESH empty document and +/// return the produced section forest. Shared by [`parse_program`] (the model +/// authored the DSL directly) and `script_gen` (a JS engine emitted the DSL by +/// calling the bound `I`/`C`/… functions). The executor collects per-line errors +/// and applies the surviving lines (best-effort); a program that builds nothing +/// is an error. +pub fn run_program_to_forest(program: &str) -> Result, String> { let mut state = EditorState::new(); let mut args: BTreeMap = BTreeMap::new(); - args.insert("operations".to_string(), program); + args.insert("operations".to_string(), program.to_string()); let cmd = { let tool = op_mcp::batch_design_snapshot(&state); diff --git a/crates/op-orchestrator/src/program_gen_tests.rs b/crates/op-orchestrator/src/program_gen_tests.rs index 280087e8c..bc031cbd2 100644 --- a/crates/op-orchestrator/src/program_gen_tests.rs +++ b/crates/op-orchestrator/src/program_gen_tests.rs @@ -42,10 +42,71 @@ fn parse_program_empty_is_error() { } #[test] -fn gate_is_off_by_default() { - // No env set in the test process → off (the production default). - assert!( - !super::program_gen_enabled_for_model("glm-5.2") - || std::env::var("OPENPENCIL_PROGRAM_GEN").is_ok() +fn gate_defaults_on_for_weak_models_off_for_strong() { + // Env override decides everything when set, so this invariant only holds on + // the no-override path. + if std::env::var("OPENPENCIL_PROGRAM_GEN").is_ok() + || std::env::var("OPENPENCIL_SCRIPT_GEN").is_ok() + || std::env::var("OPENPENCIL_MANIFEST").is_ok() + { + return; + } + // Open / Chinese reasoning models default to program-DSL: flat JSONL is + // fragile for them, and executable JS is all-or-nothing on truncation. + for m in [ + "glm-5.2", + "minimax-m3", + "deepseek-v4-pro", + "qwen-max", + "MiniMax-M3", + ] { + assert!( + super::program_gen_enabled_for_model(m), + "{m} should default to program-DSL" + ); + } + // Claude / GPT / Gemini / o-series emit flat JSONL natively → program OFF. + for m in ["claude-opus-4-8", "gpt-4o", "gemini-3-pro", "o3-mini"] { + assert!( + !super::program_gen_enabled_for_model(m), + "{m} should keep flat-JSONL by default" + ); + } +} + +#[test] +fn image_without_src_and_bad_textgrowth_survive() { + // Weak-model typos that used to DROP the whole node: an `image` with no + // `src` (REQUIRED field) and a `text` with `textGrowth:"fit_content"` (not a + // valid variant). The executor's normalize now recovers both so the avatar + // and the label survive instead of vanishing (and collapsing their column). + let program = concat!( + "sec=I(null, {\"type\":\"frame\",\"name\":\"Sec\",\"layout\":\"vertical\",\"width\":\"fill_container\"})\n", + "I(sec, {\"type\":\"image\",\"name\":\"Avatar\",\"width\":40,\"height\":40})\n", + "I(sec, {\"type\":\"text\",\"content\":\"Hi\",\"textGrowth\":\"fit_content\"})" + ); + let nodes = parse_program(program).expect("program builds a forest"); + let sec = &nodes[0]; + let kids = sec.children().expect("sec children"); + assert_eq!( + kids.len(), + 2, + "both the src-less image and the bad-textGrowth text survived" ); } + +#[test] +fn bare_identifier_sizing_value_survives() { + // A weak model wrote `"width":fill_container_str` — a bare (unquoted) leaked + // variable name that fails strict JSON. `parse_json_arg` quotes it and the + // sizing normalize maps `fill_container_str` → `fill_container`, so the node + // lands instead of being dropped (measured: it dropped a table column header). + let program = concat!( + "sec=I(null, {\"type\":\"frame\",\"name\":\"Sec\",\"layout\":\"vertical\",\"width\":\"fill_container\"})\n", + "I(sec, {\"type\":\"text\",\"name\":\"Col Service\",\"content\":\"SERVICE\",\"width\":fill_container_str})" + ); + let nodes = parse_program(program).expect("program builds a forest"); + let sec = &nodes[0]; + let kids = sec.children().expect("sec children"); + assert_eq!(kids.len(), 1, "the bare-identifier-sized text survived"); +} diff --git a/crates/op-orchestrator/src/prompt.rs b/crates/op-orchestrator/src/prompt.rs index 4f7bd8942..b52e465f8 100644 --- a/crates/op-orchestrator/src/prompt.rs +++ b/crates/op-orchestrator/src/prompt.rs @@ -91,6 +91,35 @@ that renders as a full-width band, not a table cell. Emit EVERY data row/card/it with realistic values -- never a header with zero rows. Output ONLY the program lines."#; +/// Script-gen 模式（`OPENPENCIL_SCRIPT_GEN=1`）的输出协议——完全对齐 Pencil: +/// 模型写一段真 JavaScript（循环/数组/变量）调用全局 `I(parent, obj)`。引擎 +/// (rquickjs) 执行、`JSON.stringify` 序列化每个对象（=完美 JSON,无手写括号/引号 +/// 笔误),循环展开重复结构。`I` 返回新节点 id 字符串。 +const SCRIPT_FORMAT: &str = r#" +OUTPUT PROTOCOL: JAVASCRIPT PROGRAM. Write a JavaScript program (no prose, no markdown +fences) that builds this section by calling the global function I(parent, node): + const id = I(parent, { ...node... }); // inserts node, RETURNS its id (a string) +`parent` is null for THIS section's single root frame, otherwise an id returned by an +EARLIER I(...) call. A node is a child of X only if you call I(X, {...}). +I(...) is the ONLY function available — there is no console, and no other builder. Do +not call console.log or any helper; just call I(...). +USE REAL JAVASCRIPT — const/let, arrays of data, and for...of / .forEach loops — to +generate repeated structure (table rows, nav items, cards, list items) by looping over a +data array. PREFER a loop over copy-pasting near-identical I(...) calls. +Each node object starts with type ("frame"/"text"/"rectangle"/"ellipse"/"path"/"icon_font") +and uses camelCase props (cornerRadius, fontSize, fontWeight, justifyContent, alignItems, +clipContent). Do NOT set x/y on children inside layout frames. +Example: + const sec = I(null, {type:"frame", name:"Clients", layout:"vertical", width:"fill_container", gap:0}); + const tbl = I(sec, {type:"frame", layout:"vertical", width:"fill_container"}); + const rows = [{name:"Alice Chen", status:"Active"}, {name:"Bob Ito", status:"VIP"}]; + for (const r of rows) { + const row = I(tbl, {type:"frame", layout:"horizontal", width:"fill_container", padding:[12,16]}); + const c1 = I(row, {type:"frame", width:"fill_container"}); I(c1, {type:"text", content:r.name}); + const c2 = I(row, {type:"frame", width:"fill_container"}); I(c2, {type:"text", content:r.status}); + } +Generate EVERY row/card/item with realistic values. Output ONLY the JavaScript program."#; + /// Rich 模式 system prompt 末尾后缀 —— verbatim,`orchestrator.ts:1382-1383`。 const RICH_SUFFIX: &str = "\n\n---\nCRITICAL OUTPUT FORMAT ENFORCEMENT:\n\ You MUST output ONLY a single JSON object. Start your response with { and end with }.\n\ @@ -722,14 +751,24 @@ pub fn build_subagent_prompt( // to the smaller raw-JSONL prompt, and `parse_manifest` returning `None` // on such output routes parsing back through `parse_nodes`. let model_id = req.model.as_deref().unwrap_or(""); - // Program-DSL protocol (OPENPENCIL_PROGRAM_GEN) takes priority over the - // element manifest and is mutually exclusive with it. Like manifest, it runs - // only on the full first attempt; the reduced/minimal retry rungs fall back - // to raw JSONL (and `subagent::run_subtask` routes parsing accordingly). - let program_on = crate::program_gen::program_gen_enabled_for_model(model_id) + // Protocol selection for the full first attempt. Priority when more than one + // is force-enabled: script (JS, env opt-in) > program (DSL) > manifest (env + // opt-in) > raw JSONL. In production exactly ONE is on: program-DSL is the + // DEFAULT for the open / Chinese reasoning models (parent-by-reference, + // best-effort per-line parse); script-gen + manifest are env-gated + // experiments; strong models (claude / gpt / gemini / o-series) stay on raw + // JSONL. All three run only on the full first attempt — the reduced/minimal + // retry rungs fall back to raw JSONL (and `subagent::run_subtask` routes + // parsing to match). + let script_on = crate::script_gen::script_gen_enabled_for_model(model_id) && !reduced_complexity && !minimal_skills; - let manifest_on = !program_on + let program_on = !script_on + && crate::program_gen::program_gen_enabled_for_model(model_id) + && !reduced_complexity + && !minimal_skills; + let manifest_on = !script_on + && !program_on && crate::manifest::manifest_enabled_for_model(model_id) && !reduced_complexity && !minimal_skills; @@ -742,6 +781,7 @@ pub fn build_subagent_prompt( minimal_skills, manifest_on, program_on, + script_on, components, ) } @@ -759,6 +799,7 @@ fn build_subagent_prompt_with_manifest( minimal_skills: bool, manifest_on: bool, program_on: bool, + script_on: bool, components: &ComponentLibrary, ) -> (CallRequest, SkillLoadReport) { // Resolve the full generation skill set, then apply tier-gated filtering. @@ -931,7 +972,7 @@ fn build_subagent_prompt_with_manifest( // model two contradictory output contracts (e2e showed glm then emitting a // half-program/half-`_parent` blob with unclosed objects). Drop it so the // protocol's own format block (MANIFEST_FORMAT / PROGRAM_FORMAT) governs alone. - if manifest_on || program_on { + if manifest_on || program_on || script_on { filtered.retain(|s| { s.skill_name() != "jsonl-format" && s.skill_name() != "jsonl-format-simplified" }); @@ -965,7 +1006,9 @@ fn build_subagent_prompt_with_manifest( // `jsonl-format` + `jsonl-format-simplified` skills both teach `_parent`, // so this agrees with whichever skill the tier loads (no contradiction). // Manifest mode swaps in the element-manifest contract instead. - system_prompt.push_str(if program_on { + system_prompt.push_str(if script_on { + SCRIPT_FORMAT + } else if program_on { PROGRAM_FORMAT } else if manifest_on { MANIFEST_FORMAT @@ -1050,7 +1093,13 @@ fn build_subagent_prompt_with_manifest( // Three constraints differ by output protocol: the raw-JSONL path has // the model author its own root frame + ids; the manifest path forbids // exactly that (system-assigned ids, system-owned section root). - let (root_rule, nesting_rule, output_rule) = if program_on { + let (root_rule, nesting_rule, output_rule) = if script_on { + ( + format!("Create EXACTLY ONE section root frame first: const sec = I(null, {{type:\"frame\", name:\"{}\", width:\"fill_container\", height:\"fit_content\", layout:\"vertical\"}}); build everything else by calling I(parent, {{...}}) with `sec` or a returned id as parent. NEVER set a fixed pixel height on the root.", subtask.label), + "Nest via the returned id ONLY: const row = I(sec, {...}); const cell = I(row, {...}); I(cell, {type:\"text\",...}). LOOP over a data array to emit repeated rows/cards. A cell inserted into the section/table directly renders as a full-width band, not a table cell.".to_string(), + "Output ONLY the JavaScript program (calls to I(...)) -- no prose, no markdown fences.".to_string(), + ) + } else if program_on { ( format!("Create EXACTLY ONE section root frame as the first line: sec=I(null, {{\"type\":\"frame\",\"name\":\"{}\",\"width\":\"fill_container\",\"height\":\"fit_content\",\"layout\":\"vertical\"}}). Build everything else with bindings into `sec` or its descendants. NEVER set a fixed pixel height on the root.", subtask.label), "Nest via bindings ONLY: row=I(sec, ...); cell=I(row, ...); I(cell, {\"type\":\"text\",...}). A cell or its content inserted into the section/table binding directly renders as a full-width band, not a table cell. Every repeated row/card/item is its own I(...) line.".to_string(), diff --git a/crates/op-orchestrator/src/prompt_tests.rs b/crates/op-orchestrator/src/prompt_tests.rs index 8b7da626e..0482e4af1 100644 --- a/crates/op-orchestrator/src/prompt_tests.rs +++ b/crates/op-orchestrator/src/prompt_tests.rs @@ -1346,13 +1346,21 @@ fn tight_budget_dashboard_force_includes_component_composition() { }; let lib = library_with(5); - let (cr, report) = build_subagent_prompt( + // Drive the env-independent core with all three structured protocols OFF so + // this exercises the FLAT-JSONL tight-budget path the test is about — + // deterministically, regardless of the model's default protocol. (minimax-m3 + // now defaults to script-gen, which drops the jsonl-format skill and frees + // budget; that would un-exhaust the 5200 budget and void the pin scenario.) + let (cr, report) = build_subagent_prompt_with_manifest( &dash_subtask, &dash_plan, &basic_req, AbortFlag::new(), false, false, + false, + false, + false, &lib, ); let sys = &cr.system_prompt; diff --git a/crates/op-orchestrator/src/script_gen.rs b/crates/op-orchestrator/src/script_gen.rs new file mode 100644 index 000000000..b247947d0 --- /dev/null +++ b/crates/op-orchestrator/src/script_gen.rs @@ -0,0 +1,188 @@ +//! Full-Pencil JS-script generation path (`OPENPENCIL_SCRIPT_GEN`). +//! +//! The sub-agent writes a REAL JavaScript program (the Pencil `batch_design` +//! model): `const row = I(parent, {...}); for (const r of data) I(row, {...})`. +//! `I` is a Rust function bound into an embedded QuickJS (rquickjs) engine. It +//! does NOT build nodes in JS — it records one `batch_design` DSL line +//! (`b{n}=I({parent}, {JSON.stringify(obj)})`) and returns a synthetic binding +//! name, so after the script runs we hand the assembled program to the SAME +//! executor `program_gen` uses ([`crate::program_gen::run_program_to_forest`]). +//! +//! Two wins over hand-authored JSON (the program-DSL path): +//! - LOOPS: the JS engine provides `for`/`map`, so a 50-row table is a loop +//! over a data array, not 50 hand-written lines that a weak model truncates. +//! - NO JSON TYPOS: the engine serializes each object to perfect JSON, so the +//! brace/quote/missing-comma typo long tail (which the program-DSL parser has +//! to repair) simply cannot occur. +//! +//! Native-only (op-orchestrator sits behind the `design` feature wasm never +//! enables), so rquickjs's bundled C engine never reaches the web bundle. +//! Sandbox: `I` is the only effectful builder; `console` + Pencil's other +//! `batch_design` ops (`C/U/D/M/R/G`) are no-op stubs so a stray model call can't +//! abort the script. No fs / net / eval / module escape is exposed. + +use std::cell::{Cell, RefCell}; +use std::rc::Rc; + +use jian_ops_schema::node::PenNode; +use rquickjs::{Context, Function, Runtime}; + +/// JS prelude defining the global `I(parent, obj)`. It does `JSON.stringify` in +/// JS-land (engine-native → perfect JSON, no hand-typed-brace risk) and hands a +/// pair of plain STRINGS to the Rust recorder `__record` — so the Rust closure +/// never touches a `Value`/`Ctx` (sidestepping rquickjs's invariant `'js`). +/// +/// Beyond `I`, the prelude STUBS the rest of Pencil's `batch_design` op set +/// (`C/U/D/M/R/G`) and `console`. A weak model is not actually bad at JS — a +/// probe confirmed `Math`/`Date`/template-literals/`.map` all run fine — but it +/// is trained on / inclined toward Pencil's FULL op vocabulary (Pencil's own +/// free backend is pencil-minimax-m3) and habitually calls `console.log`. In a +/// bare sandbox that only defines `I`, the FIRST such call is a `ReferenceError` +/// that aborts the WHOLE script and loses the entire section. The stubs are +/// no-ops returning a fresh synthetic id, so a stray `console.log` / `G(...)` / +/// `C(...)` degrades to "that one op did nothing" instead of nuking everything. +/// (`C/U/D/M/R` operate on pre-existing nodes — meaningless when building a fresh +/// section from scratch — and image-gen `G` isn't wired here; the meaningful op +/// is `I`. The prompt still steers toward `I` only.) +const PRELUDE: &str = r#" +globalThis.I = function (parent, obj) { + return __record(parent == null ? "null" : String(parent), JSON.stringify(obj)); +}; +var __stubSeq = 0; +function __opStub() { __stubSeq += 1; return "stub" + __stubSeq; } +globalThis.C = __opStub; +globalThis.U = __opStub; +globalThis.D = __opStub; +globalThis.M = __opStub; +globalThis.R = __opStub; +globalThis.G = __opStub; +var __noop = function () {}; +globalThis.console = { log: __noop, warn: __noop, error: __noop, info: __noop, debug: __noop }; +"#; + +/// Whether the sub-agent should emit an executable JS script for `model`. +/// +/// Script-gen (real JavaScript with loops, run in QuickJS) is now **opt-in +/// only** — set `OPENPENCIL_SCRIPT_GEN=1` to force it. The default weak-model +/// protocol is program-DSL ([`crate::program_gen`]): the same +/// parent-by-reference nesting, but authored as explicit `I(parent, {...})` +/// ops (no loops — matching Pencil's actual `batch_design` output) and parsed +/// **best-effort per line**. +/// +/// That resilience is why program-DSL replaced script-gen as the default. A +/// weak model that stops or garbles mid-program loses only the trailing op; +/// but a single QuickJS `SyntaxError` (e.g. a script the model truncated +/// mid-token) throws away the ENTIRE section, and the retry then falls back to +/// flat JSONL — which flattens a table's cells into full-width siblings of the +/// row. Keeping script-gen behind an env flag preserves the A-B lever without +/// exposing production to that all-or-nothing cliff. +pub fn script_gen_enabled_for_model(_model: &str) -> bool { + std::env::var("OPENPENCIL_SCRIPT_GEN") + .map(|v| matches!(v.trim(), "1" | "true" | "TRUE" | "on")) + .unwrap_or(false) +} + +/// Run the emitted JS program in QuickJS, collecting the `I(...)` calls into a +/// `batch_design` program, then expand that to a section forest via the shared +/// executor. +pub fn parse_script(text: &str) -> Result, String> { + let script = strip_fences(text); + if script.trim().is_empty() { + return Err("script is empty after stripping fences".into()); + } + let program = run_js_to_program(&script)?; + if program.trim().is_empty() { + return Err("script emitted no I(...) operations".into()); + } + crate::program_gen::run_program_to_forest(&program) +} + +/// Execute `script` in a fresh QuickJS context with the `I(...)` prelude loaded, +/// and return the recorded `batch_design` DSL program (one `b{n}=I(...)` line per +/// `I` call). `__record(parentStr, jsonStr) -> binding` is the only Rust-bound +/// function; the closure captures the line buffer + counter (all `'static` `Rc`, +/// no JS lifetimes). +fn run_js_to_program(script: &str) -> Result { + let rt = Runtime::new().map_err(|e| format!("js runtime: {e}"))?; + let ctx = Context::full(&rt).map_err(|e| format!("js context: {e}"))?; + let lines: Rc>> = Rc::new(RefCell::new(Vec::new())); + let counter: Rc> = Rc::new(Cell::new(0)); + let lines_rec = lines.clone(); + + let outcome: Result<(), String> = ctx.with(|ctx| { + let record = Function::new(ctx.clone(), move |parent: String, json: String| -> String { + let n = counter.get(); + counter.set(n + 1); + let bind = format!("b{n}"); + lines_rec + .borrow_mut() + .push(format!("{bind}=I({parent}, {json})")); + bind + }) + .map_err(|e| format!("bind __record: {e}"))?; + ctx.globals() + .set("__record", record) + .map_err(|e| format!("set __record: {e}"))?; + ctx.eval::<(), _>(PRELUDE) + .map_err(|e| format!("prelude: {e}"))?; + ctx.eval::<(), _>(script) + .map_err(|e| describe_js_error(&ctx, e)) + }); + // Partial-success recovery: `__record` appends to `lines` as each `I(...)` + // runs, so a script that throws PART-WAY (a late `console.log`, a typo'd + // binding reference, an unstubbed call) has already recorded every node + // built before the throw. Salvage those instead of discarding the whole + // section — a stray error on line 50 must not erase 49 good nodes. Only a + // throw that fires before ANY `I(...)` ran (empty buffer) is a hard failure. + let program = lines.borrow().join("\n"); + match outcome { + Ok(()) => Ok(program), + Err(e) if program.trim().is_empty() => Err(e), + Err(e) => { + tracing::warn!( + error = %e, + recorded_ops = lines.borrow().len(), + "script threw mid-run; salvaging the nodes recorded before the throw" + ); + Ok(program) + } + } +} + +/// Turn a rquickjs eval error into a message carrying the ACTUAL JS exception +/// (message + type), not the opaque `Exception generated by QuickJS`. A weak +/// model's emitted script throws real runtime errors (`x is not defined`, +/// `Cannot read properties of undefined`); surfacing them lets the retry ladder +/// feed the cause back and lets us diagnose systematic faults. On a non-exception +/// error (e.g. syntax) fall back to the Display form. +fn describe_js_error(ctx: &rquickjs::Ctx<'_>, err: rquickjs::Error) -> String { + if err.is_exception() { + let caught = ctx.catch(); + if let Some(exc) = caught.as_exception() { + let msg = exc.message().unwrap_or_default(); + if !msg.is_empty() { + return format!("script error: {msg}"); + } + } + if let Some(s) = caught.as_string().and_then(|s| s.to_string().ok()) { + return format!("script error: {s}"); + } + return "script error: uncaught JS exception".to_string(); + } + format!("script error: {err}") +} + +/// Strip an accidental ```fence``` wrapper (a chat model sometimes adds one). +fn strip_fences(text: &str) -> String { + let trimmed = text.trim(); + if let Some(rest) = trimmed.strip_prefix("```") { + let body = rest.split_once('\n').map(|x| x.1).unwrap_or(rest); + let body = body.rsplit_once("```").map(|x| x.0).unwrap_or(body); + return body.trim().to_string(); + } + trimmed.to_string() +} + +#[cfg(test)] +#[path = "script_gen_tests.rs"] +mod tests; diff --git a/crates/op-orchestrator/src/script_gen_tests.rs b/crates/op-orchestrator/src/script_gen_tests.rs new file mode 100644 index 000000000..9dff49149 --- /dev/null +++ b/crates/op-orchestrator/src/script_gen_tests.rs @@ -0,0 +1,188 @@ +//! Tests for the JS-script generation path (`script_gen.rs`). + +use super::parse_script; +use op_editor_core::PenNodeExt; + +#[test] +fn js_loop_builds_repeated_rows_nested_under_the_table() { + // The whole point: a JS `for` loop generates N rows; each row's cells nest + // under the row purely via the binding returned by `I`. + let script = r#" + const sec = I(null, {type:"frame", name:"Sec", layout:"vertical", width:"fill_container"}); + const tbl = I(sec, {type:"frame", name:"Table", layout:"vertical", width:"fill_container"}); + const rows = [{name:"Alice"},{name:"Bob"},{name:"Cara"}]; + for (const r of rows) { + const row = I(tbl, {type:"frame", layout:"horizontal", width:"fill_container"}); + const cell = I(row, {type:"frame", width:"fill_container"}); + I(cell, {type:"text", content:r.name}); + } + "#; + let nodes = parse_script(script).expect("script builds a forest"); + assert_eq!(nodes.len(), 1, "one section root"); + let sec = &nodes[0]; + let tbl = &sec.children().expect("sec children")[0]; + let rows = tbl.children().expect("table children"); + assert_eq!(rows.len(), 3, "loop produced all 3 rows"); + // each row -> cell -> text + let cell = &rows[0].children().expect("row children")[0]; + assert_eq!( + cell.children().expect("cell children").len(), + 1, + "text nested under the cell via the binding" + ); +} + +#[test] +fn script_gate_is_opt_in_only() { + // Script-gen (executable JS) is now env opt-in — OFF by default for EVERY + // model (program-DSL is the weak-model default). The env override decides + // when set, so this invariant only holds on the no-override path. + if std::env::var("OPENPENCIL_SCRIPT_GEN").is_ok() { + return; + } + for m in [ + "glm-5.2", + "minimax-m3", + "deepseek-v4-pro", + "qwen-max", + "MiniMax-M3", + "claude-opus-4-8", + "gpt-4o", + "gemini-3-pro", + "o3-mini", + ] { + assert!( + !super::script_gen_enabled_for_model(m), + "{m} should not use script-gen unless OPENPENCIL_SCRIPT_GEN is set" + ); + } +} + +#[test] +fn empty_script_is_error() { + assert!(parse_script(" ").is_err()); +} + +/// A CSS-fluent model writes `justifyContent:"flex_end"` (snake_case CSS, by +/// analogy to our `space_between`). Such cells must survive end-to-end through +/// script-gen → program → forest. Before the executor normalized the underscore +/// flex_* forms, every such cell failed to deserialize and was SILENTLY dropped +/// — a 5-column table lost the right-aligned amount column + the left-aligned +/// header labels, keeping only the `center` ones. This guards the whole path. +#[test] +fn flex_aligned_cells_survive_the_forest() { + let script = r#" + const tbl = I(null, {type:"frame", name:"Table", layout:"vertical", width:"fill_container"}); + const data = [{name:"Alice", amount:"$1,240"}, {name:"Bob", amount:"$860"}]; + for (const d of data) { + const row = I(tbl, {type:"frame", layout:"horizontal", width:"fill_container"}); + const nameCell = I(row, {type:"frame", width:"fill_container", justifyContent:"flex_start"}); + I(nameCell, {type:"text", content:d.name}); + const amtCell = I(row, {type:"frame", width:110, justifyContent:"flex_end", alignItems:"flex_end"}); + I(amtCell, {type:"text", content:d.amount}); + } + "#; + let nodes = parse_script(script).expect("script builds a forest"); + let json = serde_json::to_string(&nodes).unwrap(); + // The flex_end amount cells must NOT be dropped — both amounts present. + assert!( + json.contains("$1,240"), + "flex_end amount cell must survive: {json}" + ); + assert!( + json.contains("$860"), + "every flex_end amount cell must survive" + ); + // And each row keeps BOTH cells (name + amount), not just the non-flex one. + let tbl = &nodes[0]; + let rows = tbl.children().expect("table rows"); + assert_eq!(rows.len(), 2, "both rows present"); + for row in rows { + assert_eq!( + row.children().map(|c| c.len()).unwrap_or(0), + 2, + "each row keeps the flex_start name cell AND the flex_end amount cell" + ); + } +} + +/// A Pencil-trained model (Pencil's own free backend is pencil-minimax-m3) +/// habitually calls `console.log` and Pencil's other batch_design ops +/// (`C/U/D/M/R/G`). The sandbox stubs them as no-ops so a stray call can't +/// `ReferenceError` and abort the whole section — the section's `I(...)` nodes +/// must still come through. (Pre-fix, each of these aborted the entire script.) +#[test] +fn pencil_ops_and_console_do_not_abort_the_script() { + let base = r#"const s = I(null, {type:"frame", name:"S", layout:"vertical", width:"fill_container"});"#; + let cases: &[(&str, String)] = &[ + ( + "console.log", + format!("console.log('building section');\n{base}"), + ), + ( + "console mid-script", + format!("{base}\nconsole.warn('done');"), + ), + ( + "Pencil G() image-gen", + format!("{base}\nG(s, 'ai', 'a hero photo');"), + ), + ( + "Pencil C() copy", + format!("{base}\nconst c = C(s, s, {{}});"), + ), + ( + "Pencil U/D/M/R", + format!("{base}\nU('s',{{}}); D('x'); M('x','s',0); R('p',{{}});"), + ), + ]; + for (label, script) in cases { + let nodes = parse_script(script) + .unwrap_or_else(|e| panic!("{label} must not abort the script: {e}")); + assert_eq!(nodes.len(), 1, "{label}: the section root must survive"); + } +} + +/// Real JS the models DO write must keep working (full ES via QuickJS). +#[test] +fn standard_es_constructs_work() { + let base = r#"const s = I(null, {type:"frame", name:"S", layout:"vertical", width:"fill_container"});"#; + for (label, extra) in [ + ("Math", "const n = Math.round(3.7);"), + ("Date", "const d = new Date(2024,0,1);"), + ("template literal", "const t = `row ${1+1}`;"), + ( + "map", + "[1,2,3].map(x => I(s, {type:'text', content:String(x)}));", + ), + ] { + let script = format!("{base}\n{extra}"); + assert!( + parse_script(&script).is_ok(), + "{label} is standard ES and must run" + ); + } +} + +/// Partial-success recovery: a throw PART-WAY through the script keeps every +/// node recorded before it. A typo'd binding reference (genuine ReferenceError +/// we can't stub) on a late line must not erase the good nodes that preceded it. +#[test] +fn throw_midway_salvages_nodes_recorded_before_it() { + let script = r#" + const s = I(null, {type:"frame", name:"S", layout:"vertical", width:"fill_container"}); + const a = I(s, {type:"text", content:"kept-1"}); + const b = I(s, {type:"text", content:"kept-2"}); + I(typoBindingNeverDefined, {type:"text", content:"lost"}); + I(s, {type:"text", content:"after-throw"}); + "#; + let nodes = parse_script(script).expect("partial program must be salvaged, not discarded"); + let s = &nodes[0]; + // The two texts recorded before the throw survive; the post-throw line never + // ran. (Section root + 2 kept children.) + assert_eq!( + s.children().map(|c| c.len()).unwrap_or(0), + 2, + "the two nodes built before the throw must be kept" + ); +} diff --git a/crates/op-orchestrator/src/subagent.rs b/crates/op-orchestrator/src/subagent.rs index c2ec37a8d..ca8b7906f 100644 --- a/crates/op-orchestrator/src/subagent.rs +++ b/crates/op-orchestrator/src/subagent.rs @@ -175,10 +175,28 @@ pub(crate) async fn run_subtask_with_reveal_at( // prompt (`program_on`) — the reduced/minimal retry rungs teach raw JSONL, so // parsing must fall back to `parse_nodes` there too. let model_id = req.model.as_deref().unwrap_or(""); - let program_on = crate::program_gen::program_gen_enabled_for_model(model_id) + let script_on = crate::script_gen::script_gen_enabled_for_model(model_id) && !reduced_complexity && !minimal_skills; - let mut nodes = if program_on { + let program_on = !script_on + && crate::program_gen::program_gen_enabled_for_model(model_id) + && !reduced_complexity + && !minimal_skills; + let mut nodes = if script_on { + match crate::script_gen::parse_script(&text) { + Ok(n) => n, + Err(e) => { + tracing::warn!( + subtask = %subtask.id, + text_len = text.len(), + thinking_len, + raw = %text, + "subagent script-gen parse failed" + ); + return fail(e); + } + } + } else if program_on { match crate::program_gen::parse_program(&text) { Ok(n) => n, Err(e) => {