From ddc82f2743e5fcadc207ca1d30abcd8b023bcb31 Mon Sep 17 00:00:00 2001 From: Fini Date: Tue, 21 Apr 2026 00:23:54 +0800 Subject: [PATCH] feat(ai): N-tool orchestrator integration Phase 1 (flag off by default) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements plan §3.1-§3.5 of the tier-aware embedded-orchestrator integration behind ENABLE_ELEMENT_TOOLS_IN_ORCHESTRATOR env var. With the flag unset (default production state) this change is a no-op — every path added here short-circuits on !needsElementTools(profile). §3.1 model-profiles.ts: - needsElementTools(profile) — returns true iff env flag truthy AND tier in {basic, standard}. Full tier stays OFF per A/B v1 Kimi K2.5 ceiling-effect finding (Δ M1 -12.5pp). - 20 unit tests cover the 2×3 flag × tier matrix + truthy-value allow-list parsing. §3.2 orchestrator-sub-agent.ts: - Pass hasMcpTools: needsElementTools(modelProfile) into resolveSkills('generation', ...) so elements.md auto-loads for gated models, matching the A/B v1 treatment arm. §3.3 orchestrator-sub-agent.ts: - When flag fires, append ELEMENT_TOOL_OUTPUT_FORMAT block to the sub-agent system prompt. Verbatim from scripts/ab-corpus/build-prompt.ts::T_TOOL_CALL_INSTRUCTIONS so production reproduces the measured behavior (PRIMARY element-tool call / FALLBACK batch_design wrapped in op_tool). §3.4 design-parser.ts: - tryParseElementToolOutput(raw) wraps pen-ai-skills parseModelOutput and returns a tagged union {kind:'element-tool'|'batch-design-dsl'} when is detected, or null to route back through the legacy extractJsonFromResponse flow. - 9 unit tests cover happy-path detection, stripping, multi-tag preference (element tool wins over scaffold batch_design), legacy passthrough, and malformed-tag graceful fallback. §3.5 orchestrator-sub-agent.ts: - STUB: when streaming applied zero nodes AND the completed response is element-tool-shape, return a clear error pointing at plan §3.5 as the Phase 2 work item. Apply-path dispatch (server-side pen-mcp handler invocation, live://canvas merge) is deferred to avoid shipping a path that's untested against the live-canvas sync machinery. Tests: 1863/1863 (was 1834; +20 profile tests + 9 parser tests). Format and tsc clean. No behavior change with flag off. --- .../design-parser-element-tools.test.ts | 82 ++++++++++++++ .../model-profiles-element-tools.test.ts | 100 ++++++++++++++++++ apps/web/src/services/ai/design-parser.ts | 63 +++++++++++ apps/web/src/services/ai/model-profiles.ts | 39 +++++++ .../src/services/ai/orchestrator-sub-agent.ts | 92 +++++++++++++++- 5 files changed, 374 insertions(+), 2 deletions(-) create mode 100644 apps/web/src/services/ai/__tests__/design-parser-element-tools.test.ts create mode 100644 apps/web/src/services/ai/__tests__/model-profiles-element-tools.test.ts diff --git a/apps/web/src/services/ai/__tests__/design-parser-element-tools.test.ts b/apps/web/src/services/ai/__tests__/design-parser-element-tools.test.ts new file mode 100644 index 000000000..224d78b85 --- /dev/null +++ b/apps/web/src/services/ai/__tests__/design-parser-element-tools.test.ts @@ -0,0 +1,82 @@ +import { describe, it, expect } from 'vitest'; +import { tryParseElementToolOutput } from '../design-parser'; + +/** + * Element-tool output-shape detection. Sits in front of the legacy + * JSONL parser when the N-tool feature flag is on. Contract mirrors + * the corpus A/B parser so production behavior matches the measured + * results in openpencil-docs + * superpowers/notes/2026-04-20-ab-v1-results.md. + */ +describe('tryParseElementToolOutput — element-tool path', () => { + it('detects a single with an add_*_v0 name', () => { + const raw = + '{"name":"add_card_row_v0","arguments":{"items":[{"title":"Hiit"}]}}'; + const out = tryParseElementToolOutput(raw); + expect(out).not.toBeNull(); + if (!out) throw new Error(); + expect(out.kind).toBe('element-tool'); + if (out.kind !== 'element-tool') throw new Error(); + expect(out.name).toBe('add_card_row_v0'); + expect(out.arguments).toEqual({ items: [{ title: 'Hiit' }] }); + }); + + it('prefers element-tool over batch_design when both appear', () => { + // Observed weak-model pattern (MiniMax M2.7): emits batch_design + // scaffold + add_nav_chip_row_v0 call + another batch_design. + // Element-tool intent must win the dispatch. + const raw = + '{"name":"batch_design","arguments":{"operations":"root=I(null, {})"}}\n' + + '{"name":"add_nav_chip_row_v0","arguments":{"items":[{"label":"All"}]}}'; + const out = tryParseElementToolOutput(raw); + if (!out || out.kind !== 'element-tool') throw new Error(); + expect(out.name).toBe('add_nav_chip_row_v0'); + }); + + it('strips chain-of-thought before detection', () => { + const raw = + 'Let me choose the right element tool...\n' + + '{"name":"add_empty_state_v0","arguments":{"title":"No items"}}'; + const out = tryParseElementToolOutput(raw); + if (!out || out.kind !== 'element-tool') throw new Error(); + expect(out.name).toBe('add_empty_state_v0'); + }); +}); + +describe('tryParseElementToolOutput — batch_design path', () => { + it('unwraps {name:"batch_design"} and returns the DSL string', () => { + const raw = + '{"name":"batch_design","arguments":{"operations":"root=I(null, {\\"type\\":\\"frame\\"})"}}'; + const out = tryParseElementToolOutput(raw); + if (!out || out.kind !== 'batch-design-dsl') throw new Error(); + expect(out.dsl).toContain('root=I(null'); + }); +}); + +describe('tryParseElementToolOutput — legacy passthrough', () => { + it('returns null for JSONL-looking output so the legacy parser runs', () => { + const raw = '{"type":"frame","name":"Root","children":[]}'; + expect(tryParseElementToolOutput(raw)).toBeNull(); + }); + + it('returns null for a bare PenNode tree', () => { + const raw = '[{"type":"frame","name":"Root","children":[]}]'; + expect(tryParseElementToolOutput(raw)).toBeNull(); + }); + + it('returns null for prose-only output', () => { + expect(tryParseElementToolOutput("I'm happy to help!")).toBeNull(); + }); + + it('returns null for empty output', () => { + expect(tryParseElementToolOutput('')).toBeNull(); + expect(tryParseElementToolOutput(' ')).toBeNull(); + }); + + it('returns null for malformed tag (falls through to legacy)', () => { + // Corpus parser returns garbage for these; from the sub-agent's + // perspective, we want the legacy JSONL parser to have a chance + // before giving up. null is the signal "not element-tool shape". + expect(tryParseElementToolOutput('not json')).toBeNull(); + }); +}); diff --git a/apps/web/src/services/ai/__tests__/model-profiles-element-tools.test.ts b/apps/web/src/services/ai/__tests__/model-profiles-element-tools.test.ts new file mode 100644 index 000000000..0a438edf5 --- /dev/null +++ b/apps/web/src/services/ai/__tests__/model-profiles-element-tools.test.ts @@ -0,0 +1,100 @@ +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { needsElementTools } from '../model-profiles'; +import type { ModelProfile } from '../model-profiles'; + +/** + * Feature-flag gate for the N-tool embedded-orchestrator integration. + * + * Contract (per plan §3.1 — openpencil-docs + * superpowers/plans/2026-04-21-element-tools-orchestrator-integration.md): + * needsElementTools(p) === true + * iff ENABLE_ELEMENT_TOOLS_IN_ORCHESTRATOR env var is truthy + * AND p.tier is 'basic' or 'standard' + * + * Why the tier gate: A/B v1 showed ceiling regression on the one + * full-tier weak model sampled (Kimi K2.5 Δ M1 -12.5pp). Until a + * follow-up RCA explains that regression, full-tier models are OFF + * by default even when the global flag is on. + */ + +const FLAG = 'ENABLE_ELEMENT_TOOLS_IN_ORCHESTRATOR'; + +function profile(tier: ModelProfile['tier']): ModelProfile { + return { match: '', tier, label: `Test ${tier}` }; +} + +const originalValue = process.env[FLAG]; + +beforeEach(() => { + delete process.env[FLAG]; +}); + +afterEach(() => { + if (originalValue === undefined) delete process.env[FLAG]; + else process.env[FLAG] = originalValue; +}); + +describe('needsElementTools — flag OFF (default production state)', () => { + it('returns false for every tier when the env var is unset', () => { + expect(needsElementTools(profile('basic'))).toBe(false); + expect(needsElementTools(profile('standard'))).toBe(false); + expect(needsElementTools(profile('full'))).toBe(false); + }); + + it('returns false when env var is the literal string "false"', () => { + process.env[FLAG] = 'false'; + expect(needsElementTools(profile('basic'))).toBe(false); + expect(needsElementTools(profile('standard'))).toBe(false); + }); + + it('returns false when env var is the literal string "0"', () => { + process.env[FLAG] = '0'; + expect(needsElementTools(profile('basic'))).toBe(false); + }); + + it('returns false on empty string (edge: `export FOO=` style)', () => { + process.env[FLAG] = ''; + expect(needsElementTools(profile('basic'))).toBe(false); + }); + + it('returns false on whitespace-only value', () => { + process.env[FLAG] = ' '; + expect(needsElementTools(profile('basic'))).toBe(false); + }); +}); + +describe('needsElementTools — flag ON, tier-gated', () => { + beforeEach(() => { + process.env[FLAG] = '1'; + }); + + it('returns true for basic tier', () => { + expect(needsElementTools(profile('basic'))).toBe(true); + }); + + it('returns true for standard tier', () => { + expect(needsElementTools(profile('standard'))).toBe(true); + }); + + it('returns FALSE for full tier (ceiling-effect guard)', () => { + // Kimi K2.5 regressed -12.5pp in A/B v1; full tier stays off by + // default until RCA produces a finding that changes the policy. + expect(needsElementTools(profile('full'))).toBe(false); + }); +}); + +describe('needsElementTools — truthy-value parsing', () => { + for (const truthy of ['1', 'true', 'TRUE', 'yes', 'YES', 'on', 'ON', ' true ']) { + it(`treats ${JSON.stringify(truthy)} as enabled`, () => { + process.env[FLAG] = truthy; + expect(needsElementTools(profile('basic'))).toBe(true); + }); + } + + for (const falsy of ['2', 'enabled', 'off', 'no']) { + it(`treats ${JSON.stringify(falsy)} as disabled (strict allow-list)`, () => { + process.env[FLAG] = falsy; + expect(needsElementTools(profile('basic'))).toBe(false); + }); + } +}); diff --git a/apps/web/src/services/ai/design-parser.ts b/apps/web/src/services/ai/design-parser.ts index 2e0494b88..d116b480e 100644 --- a/apps/web/src/services/ai/design-parser.ts +++ b/apps/web/src/services/ai/design-parser.ts @@ -1,4 +1,5 @@ import type { PenNode } from '@/types/pen'; +import { parseModelOutput } from '@zseven-w/pen-ai-skills'; // --------------------------------------------------------------------------- // Streaming JSONL parser result @@ -9,6 +10,68 @@ export interface StreamingNodeResult { parentId: string | null; } +// --------------------------------------------------------------------------- +// Element-tool output shape detection +// --------------------------------------------------------------------------- +// +// When `needsElementTools(modelProfile)` is true the sub-agent prompt +// teaches the model to wrap its response in `{...}` +// (see orchestrator-sub-agent.ts::ELEMENT_TOOL_OUTPUT_FORMAT). This +// helper detects that wrapper BEFORE the caller hands the raw output +// to the legacy `extractJsonFromResponse`, so the dispatch layer can +// route element-tool calls through pen-mcp's handler family instead +// of the legacy JSONL path. +// +// Returns `null` when the output isn't ``-wrapped — caller +// should then try `extractJsonFromResponse` as a fallback. Never +// throws; malformed or partial tags degrade to the legacy path. + +export type DesignOutputShape = + | { + kind: 'element-tool'; + /** Element-tool name, e.g. `add_card_row_v0`. Matches pen-mcp's ELEMENT_TOOL_NAMES. */ + name: string; + /** Raw tool arguments from the model. Pen-mcp handler validates shape. */ + arguments: Record; + raw: string; + } + | { + kind: 'batch-design-dsl'; + /** DSL string (single operation per line), ready for handleBatchDesign. */ + dsl: string; + raw: string; + }; + +/** + * Try to detect an ``-wrapped output. Returns a tagged-union + * route hint when detected; null otherwise. + * + * Precedence mirrors the corpus A/B parser: + * 1. If ANY `` tag names an `add_*_v0` element tool, that + * wins (multi-tag outputs where batch_design is used as + * scaffolding still route through the element tool — intent + * matching is the metric of interest) + * 2. Otherwise if an `` names `batch_design`, return the + * extracted `operations` as DSL + * 3. Otherwise the output uses the legacy format — return null so + * the caller falls through to `extractJsonFromResponse` + */ +export function tryParseElementToolOutput(raw: string): DesignOutputShape | null { + const parsed = parseModelOutput(raw); + if (parsed.kind === 'tool_call') { + return { + kind: 'element-tool', + name: parsed.name, + arguments: parsed.arguments, + raw: parsed.raw, + }; + } + if (parsed.kind === 'batch_design') { + return { kind: 'batch-design-dsl', dsl: parsed.dsl, raw: parsed.raw }; + } + return null; +} + // --------------------------------------------------------------------------- // JSON extraction from AI response text // --------------------------------------------------------------------------- diff --git a/apps/web/src/services/ai/model-profiles.ts b/apps/web/src/services/ai/model-profiles.ts index 13334681b..b6141c18f 100644 --- a/apps/web/src/services/ai/model-profiles.ts +++ b/apps/web/src/services/ai/model-profiles.ts @@ -104,6 +104,45 @@ export function needsSimplifiedPrompt(profile: ModelProfile): boolean { return profile.simplifiedPrompt === true; } +/** + * Environment variable that gates N-tool element integration in the + * embedded orchestrator. Default off so production behavior is + * unchanged until the flag is explicitly flipped. Any truthy value + * (`"1"`, `"true"`, `"yes"`, case-insensitive) enables the feature. + * + * Rollout per plan §4 (openpencil-docs + * superpowers/plans/2026-04-21-element-tools-orchestrator-integration.md): + * Phase 1: ship with flag off (scaffolding only, zero behavior change) + * Phase 2: flip on for basic + standard tiers in a canary env + * Phase 3: remove the flag once stable across a monitoring window + */ +const ELEMENT_TOOLS_FLAG_ENV = 'ENABLE_ELEMENT_TOOLS_IN_ORCHESTRATOR'; + +function isElementToolsFlagEnabled(): boolean { + const raw = process.env[ELEMENT_TOOLS_FLAG_ENV]; + if (!raw) return false; + const lower = raw.trim().toLowerCase(); + return lower === '1' || lower === 'true' || lower === 'yes' || lower === 'on'; +} + +/** + * Whether to inject elements.md + teach the `` output format + * for a given model profile. Returns `true` iff the feature flag env + * var is enabled AND the model is in a tier that benefits from the + * element-tool surface. + * + * A/B v1 (openpencil-docs superpowers/notes/2026-04-20-ab-v1-results.md) + * observed consistent wins on the basic + standard tiers (Δ M1 +8 to + * +21pp on MiniMax/GLM5/GLM5.1) and a ceiling-effect regression on + * the one full-tier weak model tested (Kimi K2.5: Δ M1 -12.5pp). We + * therefore default full-tier models OFF; caller-level override is + * anticipated but not yet wired (plan §7.2). + */ +export function needsElementTools(profile: ModelProfile): boolean { + if (!isElementToolsFlagEnabled()) return false; + return profile.tier === 'basic' || profile.tier === 'standard'; +} + /** * Apply profile overrides to a timeout config object (mutates a copy). */ diff --git a/apps/web/src/services/ai/orchestrator-sub-agent.ts b/apps/web/src/services/ai/orchestrator-sub-agent.ts index 4fd6aaf15..e88359001 100644 --- a/apps/web/src/services/ai/orchestrator-sub-agent.ts +++ b/apps/web/src/services/ai/orchestrator-sub-agent.ts @@ -25,7 +25,8 @@ import { buildSubAgentStyleGuideInstruction, compactSubAgentSkills, } from './orchestrator-sub-agent-compact'; -import { resolveModelProfile } from './model-profiles'; +import { tryParseElementToolOutput } from './design-parser'; +import { needsElementTools, resolveModelProfile } from './model-profiles'; import { expandRootFrameHeight, buildVariableContext, @@ -52,6 +53,37 @@ export interface StreamTimeoutConfig { effort?: 'low' | 'medium' | 'high' | 'max'; } +// --------------------------------------------------------------------------- +// Element-tool output-format contract +// --------------------------------------------------------------------------- +// +// When `needsElementTools(modelProfile)` is true we append this block to +// the sub-agent's system prompt so the model emits the same `` +// wrapper measured in the A/B v1 treatment arm (see openpencil-docs +// superpowers/notes/2026-04-20-ab-v1-results.md). Kept verbatim against +// scripts/ab-corpus/build-prompt.ts::T_TOOL_CALL_INSTRUCTIONS so the +// production path reproduces the behavior that cleared the decision gate. +// +// §3.4 of the integration plan adds a parser that detects these tags +// BEFORE the legacy JSONL parser so existing flows stay intact when the +// flag is off or when the model bypasses the wrapper. +const ELEMENT_TOOL_OUTPUT_FORMAT = [ + '', + 'OUTPUT FORMAT — EMIT AS TOOL CALL:', + '', + 'Respond with one `` tag, nothing else. Choose based on intent:', + '', + 'PRIMARY: when your intent matches an add_*_v0 element tool above, emit:', + ' {"name": "add_X_v0", "arguments": {...}}', + '', + 'FALLBACK: when no element tool fits (heterogeneous layout, composite section, post-hoc styling), emit:', + ' {"name": "batch_design", "arguments": {"operations": ""}}', + 'The `operations` value is a single string containing the batch_design DSL.', + '', + 'Do not combine multiple tags. Do not add prose before or after.', + '', +].join('\n'); + // --------------------------------------------------------------------------- // Sub-agent execution (sequential or concurrent) // --------------------------------------------------------------------------- @@ -330,6 +362,16 @@ async function executeSubAgent( } const hasDesignMdContent = designMdContent.length > 0; + // Feature-flagged gate for N-tool element-surface integration. + // Only fires when ENABLE_ELEMENT_TOOLS_IN_ORCHESTRATOR env var is + // truthy AND the model is in the basic/standard tier; full-tier + // models stay on the legacy path per A/B v1 ceiling-effect finding + // (Kimi K2.5 Δ M1 -12.5pp). Default production state is off, so + // this wiring is a no-op until rollout flips the env var. + // Plan: openpencil-docs + // superpowers/plans/2026-04-21-element-tools-orchestrator-integration.md + const elementToolsEnabled = needsElementTools(modelProfile); + const genCtx = resolveSkills('generation', request.prompt, { flags: { hasVariables: !!variables && Object.keys(variables).length > 0, @@ -339,6 +381,13 @@ async function executeSubAgent( // - no pre-built style guide selected // - no usable design.md content (empty raw + empty structured summary) noStyleGuideMatch: !plan.selectedStyleGuideContent && !hasDesignMdContent, + // hasMcpTools gates `elements.md` auto-loading in the skill + // registry. When true, the generation prompt picks up the + // 39-tool decision-tree + invariants reference. §3.3 of the + // plan teaches the matching `` output format so the + // model actually emits tool calls rather than pseudo-call + // syntax the legacy parser would reject. + hasMcpTools: elementToolsEnabled, }, dynamicContent: hasDesignMdContent ? { designMdContent } : undefined, budgetOverride: @@ -382,7 +431,10 @@ async function executeSubAgent( reducedComplexity, ); - const systemPrompt = resolvedSkills.map((s) => s.content).join('\n\n'); + const skillPrompt = resolvedSkills.map((s) => s.content).join('\n\n'); + const systemPrompt = elementToolsEnabled + ? skillPrompt + '\n\n' + ELEMENT_TOOL_OUTPUT_FORMAT + : skillPrompt; if (SUB_AGENT_DEBUG_FLAGS.LOG_PROMPT_SIZE) { const skillNames = resolvedSkills.map((s) => s.meta.name).join(','); @@ -437,6 +489,42 @@ async function executeSubAgent( } } + // Element-tool shape detection (§3.4 of the plan). When the + // feature flag is on and streaming produced no nodes (expected + // because weak models emit `` at the end, not incremental + // JSON), check if the completed response is an element-tool call + // we can dispatch. §3.5 apply-path is a Phase 2 item — for now we + // surface a clear error so dev runs with the flag on see exactly + // what happened. + if ( + elementToolsEnabled && + renderer.getAppliedIds().size === 0 && + rawResponse.trim().length > 0 + ) { + const elementShape = tryParseElementToolOutput(rawResponse); + if (elementShape !== null) { + renderer.finish(); + progressEntry.status = 'error'; + emitProgress(plan, progress, callbacks); + const detectedName = + elementShape.kind === 'element-tool' + ? elementShape.name + : 'batch_design (DSL wrapped in op_tool)'; + return { + subtaskId: subtask.id, + nodes: renderer.getInsertedNodes(), + rawResponse, + error: + `Element-tool output detected (${detectedName}) but the ` + + `apply pipeline is not yet wired into the embedded ` + + `orchestrator. Set ENABLE_ELEMENT_TOOLS_IN_ORCHESTRATOR=0 ` + + `to fall back to the legacy JSONL path, or wait for ` + + `integration plan §3.5 (openpencil-docs ` + + `superpowers/plans/2026-04-21-element-tools-orchestrator-integration.md).`, + }; + } + } + // Fallback batch extraction if (renderer.getAppliedIds().size === 0 && rawResponse.trim()) { const count = renderer.flushRemaining(rawResponse);