From 2afe78bd84bd27e975f2e9f059f294d3e1a6c804 Mon Sep 17 00:00:00 2001 From: leader Date: Thu, 12 Nov 2026 12:12:10 +0900 Subject: [PATCH 1/2] feat: field focus marks the visual container and field states ease - Generator: field component sets emit their focus variant as _focusWithin on the container and, without a Figma prototype transition, a 160ms ease-out transition on the changed props; transitionProperty is written in CSS kebab-case. - devup_ui_validate: input-focus-* rules and input-state-transition. - devup_runtime_audit: input-focus check (click and Tab) that fails inner outlines, rings missing the container, focus on the container, movement and focus states that snap with no transition. Co-Authored-By: Claude Opus 5.5 --- .../changepack_log_Fq8ZpW2nKe4TbR6yLx3Hd.json | 7 + .../devup-mcp-devup-ui/src/codegen/variant.rs | 1 + .../src/codegen/variant/drawn_when.rs | 31 +- .../src/codegen/variant/field_focus.rs | 48 +++ .../src/codegen/variant/variant_projection.rs | 22 +- .../src/ui_validate/site.rs | 113 +++++- .../src/ui_validate/site/input_focus.rs | 361 ++++++++++++++++++ .../tests/chondrogen_component_sets.rs | 58 +++ .../server/audit_plan/planning_and_capture.rs | 5 +- .../runtime_audit/inputs_and_capture.rs | 11 +- .../src/server/runtime_audit/plan_columns.rs | 2 +- .../runtime_audit/screenshot_integrity.rs | 3 +- .../src/server/runtime_audit/tests.rs | 2 +- .../src/server/runtime_audit/write_plan.rs | 1 + .../runtime_audit-fix_hint-input-focus.md | 1 + crates/devup-mcp/tests/runtime_audit_tool.rs | 2 +- harness/runtime-audit/fixtures/fields.html | 61 +++ harness/runtime-audit/input-focus-checks.mjs | 316 +++++++++++++++ harness/runtime-audit/run.mjs | 18 +- .../runtime-audit/test/input-focus.test.mjs | 163 ++++++++ 20 files changed, 1195 insertions(+), 31 deletions(-) create mode 100644 .changepacks/changepack_log_Fq8ZpW2nKe4TbR6yLx3Hd.json create mode 100644 crates/devup-mcp-devup-ui/src/codegen/variant/field_focus.rs create mode 100644 crates/devup-mcp-devup-ui/src/ui_validate/site/input_focus.rs create mode 100644 crates/devup-mcp/src/server/text/runtime_audit-fix_hint-input-focus.md create mode 100644 harness/runtime-audit/fixtures/fields.html create mode 100644 harness/runtime-audit/input-focus-checks.mjs create mode 100644 harness/runtime-audit/test/input-focus.test.mjs diff --git a/.changepacks/changepack_log_Fq8ZpW2nKe4TbR6yLx3Hd.json b/.changepacks/changepack_log_Fq8ZpW2nKe4TbR6yLx3Hd.json new file mode 100644 index 00000000..2877f11e --- /dev/null +++ b/.changepacks/changepack_log_Fq8ZpW2nKe4TbR6yLx3Hd.json @@ -0,0 +1,7 @@ +{ + "changes": { + "Cargo.toml": "Minor" + }, + "note": "Field focus marks the box the user sees. A Figma field component set (input, text field, textarea, search, select, combobox, dropdown) now generates its focus variant as _focusWithin on the container instead of an unreachable _focus (DEVUP_CODEGEN_FIELD_FOCUS_WITHIN). devup_ui_validate reports input-focus-container, input-focus-duplicate-outline and input-focus-layout-shift as browser-required review items (info) and input-focus-outline-removed and input-focus-moved-to-container as warnings; a container's _focusWithin now counts as the field's focus style. devup_runtime_audit adds the input-focus check (planned by auditPlan): every visible input, textarea and select is focused by click and by Tab and fails an inner default outline inside the container, a ring that does not surround the container, focus landing on the container and any size or position change; disabled fields are listed and skipped. Contract identifier field-focus-on-visual-container. Field states ease instead of snapping: a generated field component without a Figma prototype transition gets transition 160ms ease-out on the props its states change (transitionProperty is now written in CSS kebab-case, e.g. border-color), devup_ui_validate reports input-state-transition (info, browser-required), and the input-focus runtime check fails a focus state no transition covers.", + "date": "2026-11-12T00:00:00.000000000Z" +} diff --git a/crates/devup-mcp-devup-ui/src/codegen/variant.rs b/crates/devup-mcp-devup-ui/src/codegen/variant.rs index 95f8c6e7..1304b5f9 100644 --- a/crates/devup-mcp-devup-ui/src/codegen/variant.rs +++ b/crates/devup-mcp-devup-ui/src/codegen/variant.rs @@ -9,3 +9,4 @@ include!("variant/structural_branches.rs"); include!("variant/instance_booleans.rs"); include!("variant/tree_signature.rs"); +include!("variant/field_focus.rs"); diff --git a/crates/devup-mcp-devup-ui/src/codegen/variant/drawn_when.rs b/crates/devup-mcp-devup-ui/src/codegen/variant/drawn_when.rs index ad2665e3..29fba0f6 100644 --- a/crates/devup-mcp-devup-ui/src/codegen/variant/drawn_when.rs +++ b/crates/devup-mcp-devup-ui/src/codegen/variant/drawn_when.rs @@ -569,19 +569,32 @@ fn transition( .trim_start_matches("EASE_") .to_ascii_lowercase() .replace('_', "-"); - let properties = props + Some(( + format!("{}ms ease-{easing}", layout::format_number(duration)), + css_properties(props), + )) +} + +/// The CSS `transition-property` list for Devup props: `bg` is `background`, +/// and camelCase props are written in kebab-case (`borderColor` -> +/// `border-color`), the only spelling the browser accepts there. +fn css_properties(props: &BTreeSet) -> String { + props .iter() .map(|prop| match prop.as_str() { - "bg" => "background", - "boxShadow" => "box-shadow", - value => value, + "bg" => "background".to_owned(), + value => value.chars().fold(String::new(), |mut out, c| { + if c.is_ascii_uppercase() { + out.push('-'); + out.push(c.to_ascii_lowercase()); + } else { + out.push(c); + } + out + }), }) .collect::>() - .join(","); - Some(( - format!("{}ms ease-{easing}", layout::format_number(duration)), - properties, - )) + .join(",") } fn render_component( diff --git a/crates/devup-mcp-devup-ui/src/codegen/variant/field_focus.rs b/crates/devup-mcp-devup-ui/src/codegen/variant/field_focus.rs new file mode 100644 index 00000000..3a4824a0 --- /dev/null +++ b/crates/devup-mcp-devup-ui/src/codegen/variant/field_focus.rs @@ -0,0 +1,48 @@ +// A field component's focus state belongs on its container. + +/// Whether a component set draws a text field, search box, textarea or +/// select by its Figma name. Its frame is the visual container; the focusable +/// control is the native element the implementation renders inside it. +pub(crate) fn is_field_component(name: &str) -> bool { + let name = name.to_lowercase().replace(['-', '_', ' '], ""); + ["input", "textfield", "textarea", "searchbar", "searchbox", "searchfield", "select", "combobox", "dropdown", "입력", "검색"] + .iter() + .any(|word| name.contains(word)) +} + +/// A field's state change (focus ring, invalid border, disabled fill) eases +/// in instead of snapping. Figma prototype reactions on the field win; this is +/// the default when the design names none: short, ease-out, and only on the +/// props the states change, so layout never animates. +pub(crate) const FIELD_STATE_TRANSITION: &str = "160ms ease-out"; + +fn field_transition( + set_name: &str, + designed: Option<(String, String)>, + props: &BTreeSet, +) -> Option<(String, String)> { + if designed.is_some() || props.is_empty() || !is_field_component(set_name) { + return designed; + } + Some((FIELD_STATE_TRANSITION.to_owned(), css_properties(props))) +} + +/// A field container never receives focus itself, so a captured `focus` +/// state on it would never apply: it moves to `_focusWithin`, which matches +/// while the control inside it is focused. A root already rendered as the +/// native control keeps `_focus`. Returns whether anything moved. +fn field_focus_within( + set_name: &str, + root: &Tree, + selectors: &mut BTreeMap>, +) -> Option<()> { + let native = root.props.get("as").is_some_and(|tag| { + ["input", "textarea", "select"].iter().any(|native| tag.contains(native)) + }); + if native || !is_field_component(set_name) { + return None; + } + let focus = selectors.remove("_focus")?; + selectors.entry("_focusWithin".to_owned()).or_default().extend(focus); + Some(()) +} diff --git a/crates/devup-mcp-devup-ui/src/codegen/variant/variant_projection.rs b/crates/devup-mcp-devup-ui/src/codegen/variant/variant_projection.rs index 6a8d3ec0..f3db1469 100644 --- a/crates/devup-mcp-devup-ui/src/codegen/variant/variant_projection.rs +++ b/crates/devup-mcp-devup-ui/src/codegen/variant/variant_projection.rs @@ -248,9 +248,26 @@ pub(super) fn generate_variant_component_set( } } } - let transition = transition(snapshot, &default.node_id, &transition_props); + let set_name = set.typed_view().name().unwrap_or("Component"); + let field_focus = field_focus_within(set_name, &root, &mut selectors).map(|_| Diagnostic { + code: "DEVUP_CODEGEN_FIELD_FOCUS_WITHIN".to_owned(), + message: "The focus variant of this field component is applied with _focusWithin on its container, because the container is not what receives focus: the , +
+ + +
+
after
+
+
+
+
+
+
+ + + diff --git a/harness/runtime-audit/input-focus-checks.mjs b/harness/runtime-audit/input-focus-checks.mjs new file mode 100644 index 00000000..9186d6cb --- /dev/null +++ b/harness/runtime-audit/input-focus-checks.mjs @@ -0,0 +1,316 @@ +// input-focus: a field's focus must mark the box the user sees. +// +// A text field is often an with no border of its own inside a +// bordered container that also holds an icon, prefix or button. Focus lands +// on the , so unless the container reacts to :focus-within, the only +// thing that shows is the browser's default outline inside the box - or +// nothing. Each visible input, textarea and select is focused twice, by mouse +// click and by keyboard Tab, and judged on what the browser actually paints: +// focus stays on the field, an indicator surrounds the visual container, no +// inner outline shows inside that container, nothing moves or resizes, and +// the box eases into its focus state (a CSS transition covers what changes) +// instead of snapping. + +/** Text-entry and choice fields; buttons, checkboxes and the like are not fields. */ +export const FIELD_SELECTOR = [ + 'textarea', 'select', + 'input:not([type="hidden"]):not([type="checkbox"]):not([type="radio"]):not([type="range"]):not([type="color"])' + + ':not([type="file"]):not([type="submit"]):not([type="button"]):not([type="reset"]):not([type="image"])', +].join(', '); + +const MAX_FIELDS = 20; +const SETTLE_MS = 450; +/** Sub-pixel rounding is not movement. */ +const SHIFT_PX = 0.5; +const PROBE = 'data-devup-focus-probe'; + +/** Paint that marks focus: border, ring, outline, background. */ +const INDICATOR = [ + 'borderTopColor', 'borderRightColor', 'borderBottomColor', 'borderLeftColor', + 'borderTopWidth', 'borderRightWidth', 'borderBottomWidth', 'borderLeftWidth', + 'boxShadow', 'outlineStyle', 'outlineWidth', 'outlineColor', 'backgroundColor', +]; + +const RECT = ['x', 'y', 'width', 'height']; + +/** + * Marks every visible field in scope and finds the box each one is drawn in. + * The container is the field itself when it paints a border, background or + * shadow; otherwise the nearest of three ancestors that does and is at most a + * row taller than the field (an icon or button beside it, not a whole form). + */ +function markFields(root, { selector, probe, max }) { + const transparent = (color) => !color || color === 'transparent' || /rgba\([^)]*,\s*0\)$/.test(color); + const paints = (element) => { + const style = getComputedStyle(element); + const border = ['Top', 'Right', 'Bottom', 'Left'].some((side) => parseFloat(style[`border${side}Width`]) > 0 + && style[`border${side}Style`] !== 'none' && !transparent(style[`border${side}Color`])); + return border || !transparent(style.backgroundColor) || style.boxShadow !== 'none'; + }; + const visible = (element) => { + if (element.checkVisibility && !element.checkVisibility({ visibilityProperty: true, opacityProperty: true })) return false; + const rect = element.getBoundingClientRect(); + return rect.width > 0 && rect.height > 0; + }; + for (const old of document.querySelectorAll(`[${probe}]`)) old.removeAttribute(probe); + const fields = []; + for (const field of [...(root.matches?.(selector) ? [root] : []), ...root.querySelectorAll(selector)]) { + if (fields.length >= max || !visible(field)) continue; + const rect = field.getBoundingClientRect(); + let container = field; + if (!paints(field)) { + let node = field.parentElement; + for (let depth = 0; node && depth < 3 && node !== document.body; depth += 1, node = node.parentElement) { + const box = node.getBoundingClientRect(); + if (box.height > Math.max(rect.height * 2, rect.height + 48)) break; + if (paints(node)) { container = node; break; } + } + } + const index = fields.length; + field.setAttribute(probe, `field-${index}`); + if (container !== field) container.setAttribute(probe, `box-${index}`); + const label = field.labels?.[0]?.textContent?.trim() + || field.getAttribute('aria-label') + || (field.getAttribute('aria-labelledby') && document.getElementById(field.getAttribute('aria-labelledby'))?.textContent?.trim()) + || field.getAttribute('placeholder') || field.name || ''; + fields.push({ + index, + tag: field.tagName.toLowerCase(), + type: field.type ?? '', + name: label.slice(0, 60), + labelled: Boolean(label), + disabled: field.disabled || field.matches(':disabled'), + invalid: field.getAttribute('aria-invalid') === 'true' || field.matches(':invalid'), + separateContainer: container !== field, + containerTag: container.tagName.toLowerCase(), + radius: getComputedStyle(container).borderRadius, + }); + } + return fields; +} + +/** Computed paint and geometry of a field, its container and the next box in flow. */ +function snapshotField({ probe, index, indicator, rect: keys }) { + const field = document.querySelector(`[${probe}="field-${index}"]`); + const container = document.querySelector(`[${probe}="box-${index}"]`) ?? field; + const paint = (element) => { + const style = getComputedStyle(element); + return Object.fromEntries(indicator.map((property) => [property, style[property]])); + }; + const rect = (element) => { + if (!element) return null; + const box = element.getBoundingClientRect(); + return Object.fromEntries(keys.map((key) => [key, box[key] + (key === 'y' ? scrollY : key === 'x' ? scrollX : 0)])); + }; + let neighbour = container.nextElementSibling; + while (neighbour && !neighbour.getClientRects().length) neighbour = neighbour.nextElementSibling; + const active = document.activeElement; + const motion = (element) => { + const style = getComputedStyle(element); + return { property: style.transitionProperty, duration: style.transitionDuration }; + }; + return { + field: { paint: paint(field), rect: rect(field) }, + container: { paint: paint(container), rect: rect(container), motion: motion(container) }, + neighbour: rect(neighbour), + focus: active === field ? 'field' : active === container && container !== field ? 'container' + : active && active !== document.body ? (active.tagName.toLowerCase() + (active.id ? `#${active.id}` : '')) : 'none', + }; +} + +/** Tabbable elements in document order, as Tab walks them (no positive tabindex reordering). */ +function focusBefore({ probe, index }) { + const field = document.querySelector(`[${probe}="field-${index}"]`); + const tabbable = [...document.querySelectorAll('a[href], button, input, select, textarea, iframe, summary, [tabindex], [contenteditable="true"]')] + .filter((element) => element.tabIndex >= 0 && !element.matches(':disabled') && element.getClientRects().length + && !(element.tagName === 'INPUT' && element.type === 'hidden')); + const at = tabbable.indexOf(field); + document.activeElement?.blur?.(); + if (at > 0) { + const previous = tabbable[at - 1]; + previous.focus({ preventScroll: true }); + // A tab stop that wraps the field is its container taking focus. + if (previous.contains(field)) return 'container'; + return document.activeElement === previous ? 'previous' : 'none'; + } + if (at !== 0) return 'unreachable'; + // The first tab stop: Tab from a blurred element continues after it, so + // navigation restarts from a fixed, focusable-by-script marker at the top. + const start = document.createElement('span'); + start.tabIndex = -1; + start.setAttribute(probe, 'start'); + start.style.cssText = 'position:fixed;top:0;left:0;width:0;height:0;overflow:hidden'; + document.body.prepend(start); + start.focus({ preventScroll: true }); + return 'start'; +} + +/** + * Focuses each field by click and by Tab and records paint and geometry + * before and after. Disabled fields are listed, never focused. + * + * @param {import('playwright').Page} page + * @param {import('playwright').Locator} scope + */ +export async function observeInputFocus(page, scope, { timeout = 10_000 } = {}) { + const fields = await scope.evaluate(markFields, { selector: FIELD_SELECTOR, probe: PROBE, max: MAX_FIELDS }); + const snap = (index) => page.evaluate(snapshotField, { probe: PROBE, index, indicator: INDICATOR, rect: RECT }); + const results = []; + for (const field of fields) { + if (field.disabled) { + results.push({ ...field, modes: [] }); + continue; + } + const locator = page.locator(`[${PROBE}="field-${field.index}"]`); + const modes = []; + // Mouse: a click inside the field, where a user would put the caret. + try { + await page.evaluate(() => document.activeElement?.blur?.()); + await locator.scrollIntoViewIfNeeded({ timeout }); + await page.mouse.move(0, 0); + // An eased state needs time to return to rest before it is measured. + await page.waitForTimeout(SETTLE_MS); + const before = await snap(field.index); + await locator.click({ timeout, position: { x: 4, y: 4 }, noWaitAfter: true }); + // The pointer leaves so a hover style is not mistaken for focus. + await page.mouse.move(0, 0); + await page.waitForTimeout(SETTLE_MS); + const after = await snap(field.index); + if (field.tag === 'select') await page.keyboard.press('Escape'); + modes.push({ mode: 'click', before, after }); + } catch (error) { + modes.push({ mode: 'click', error: String(error?.message ?? error).split('\n')[0] }); + } + // Keyboard: focus the tabbable element before the field, then Tab. + try { + await page.mouse.move(0, 0); + const start = await page.evaluate(focusBefore, { probe: PROBE, index: field.index }); + if (start === 'unreachable') { + modes.push({ mode: 'tab', unreachable: true }); + } else { + await page.waitForTimeout(SETTLE_MS); + const before = await snap(field.index); + await page.keyboard.press('Tab'); + await page.waitForTimeout(SETTLE_MS); + const after = await snap(field.index); + await page.evaluate((probe) => document.querySelector(`[${probe}="start"]`)?.remove(), PROBE); + modes.push({ mode: 'tab', before, after, ...(start === 'container' ? { containerTabStop: true } : {}) }); + } + } catch (error) { + modes.push({ mode: 'tab', error: String(error?.message ?? error).split('\n')[0] }); + } + await page.evaluate(() => document.activeElement?.blur?.()); + results.push({ ...field, modes }); + } + await page.evaluate((probe) => { for (const element of document.querySelectorAll(`[${probe}]`)) element.removeAttribute(probe); }, PROBE) + .catch(() => {}); + return { fields: results }; +} + +const visibleOutline = (paint) => paint.outlineStyle !== 'none' && parseFloat(paint.outlineWidth) > 0; +const changed = (before, after) => INDICATOR.filter((property) => before[property] !== after[property]); +const moved = (before, after) => Boolean(before && after) && RECT.some((key) => Math.abs(before[key] - after[key]) > SHIFT_PX); + +/** `borderTopColor` -> `border-top-color`. */ +const kebab = (property) => property.replace(/[A-Z]/g, (letter) => `-${letter.toLowerCase()}`); + +/** + * The changed paint properties no CSS transition eases: a property is eased + * when `transition-property` names it, its shorthand (`border-color`, + * `border`, `outline`) or `all`, with a duration above zero. `outline-style` + * is discrete and never needs one. Missing motion data (an older snapshot) + * eases everything, so it never fails on its own. + */ +export function snappedProperties(changedProperties, motion) { + if (!motion) return []; + const names = String(motion.property ?? '').split(',').map((name) => name.trim()); + const durations = String(motion.duration ?? '').split(',').map((value) => parseFloat(value) || 0); + const duration = (at) => durations[at % Math.max(durations.length, 1)] ?? 0; + return changedProperties.filter((property) => property !== 'outlineStyle').filter((property) => { + const css = kebab(property); + const sideless = css.replace(/-(top|right|bottom|left)(?=-|$)/, ''); + return !names.some((name, at) => duration(at) > 0 && (name === 'all' || name === css || name === sideless + || css.startsWith(`${name}-`) || sideless.startsWith(`${name}-`))); + }); +} + +/** Problems with one focus of one field; empty when it passes. */ +export function judgeFocus(field, mode) { + const { before, after } = mode; + if (after.focus === 'container') { + return ['focus moved to the container instead of the field']; + } + if (after.focus !== 'field') { + return [`focus did not reach the field (active: ${after.focus})`]; + } + const problems = mode.containerTabStop ? ['Tab stops on a container wrapping the field before the field itself'] : []; + const boxChange = changed(before.container.paint, after.container.paint); + const innerOutline = field.separateContainer && visibleOutline(after.field.paint) + && (changed(before.field.paint, after.field.paint).length > 0); + const innerRing = field.separateContainer && before.field.paint.boxShadow !== after.field.paint.boxShadow; + if (!field.separateContainer) { + if (boxChange.length === 0) problems.push('no focus indicator: nothing on the field changes'); + } else if (boxChange.length === 0) { + problems.push(innerOutline || innerRing + ? `focus marks only the inner ${field.tag}, not the <${field.containerTag}> box around it` + : `no focus indicator: neither the ${field.tag} nor its <${field.containerTag}> box changes`); + } else if (innerOutline || innerRing) { + problems.push(`a second ${innerOutline ? 'outline' : 'ring'} shows on the inner ${field.tag} inside the <${field.containerTag}> box's focus style`); + } + const snapped = boxChange.length ? snappedProperties(boxChange, after.container.motion) : []; + if (snapped.length) { + problems.push(`the focus state snaps with no transition on ${[...new Set(snapped.map(kebab).map((name) => name.replace(/-(top|right|bottom|left)(?=-|$)/, '')))].join(', ')}`); + } + if (moved(before.container.rect, after.container.rect) || moved(before.field.rect, after.field.rect)) { + problems.push('the field or its box changes size or position when focused'); + } else if (moved(before.neighbour, after.neighbour)) { + problems.push('the element after the field moves when it is focused'); + } + return problems; +} + +/** Fails any field whose focus, by click or by Tab, does not mark its box cleanly. */ +export function evaluateInputFocus(observed) { + const fields = observed.fields ?? []; + const enabled = fields.filter((field) => !field.disabled); + if (fields.length === 0) return { status: 'pass', reason: 'no input, textarea or select to check', measured: { checked: 0, fields: [] } }; + const rows = []; + const failures = []; + const unjudged = []; + for (const field of enabled) { + for (const mode of field.modes) { + const where = `<${field.tag}> "${field.name || field.index}" by ${mode.mode}`; + if (mode.error || mode.unreachable) { + unjudged.push(`${where}: ${mode.error ?? 'not reachable with Tab'}`); + rows.push({ index: field.index, mode: mode.mode, status: 'inconclusive', reason: mode.error ?? 'not reachable with Tab' }); + continue; + } + const problems = judgeFocus(field, mode); + if (problems.length) failures.push(`${where}: ${problems.join(', ')}`); + rows.push({ + index: field.index, mode: mode.mode, status: problems.length ? 'fail' : 'pass', problems, + changed: changed(mode.before.container.paint, mode.after.container.paint), + }); + } + } + const measured = { + checked: enabled.length, + disabled: fields.length - enabled.length, + fields: fields.map(({ modes, ...field }) => ({ + ...field, + modes: rows.filter((row) => row.index === field.index).map(({ index, ...row }) => row), + })), + }; + if (failures.length) { + return { status: 'fail', reason: `${failures.length} focus problem(s): ${failures.slice(0, 4).join('; ')}`, measured }; + } + if (unjudged.length) { + return { status: 'inconclusive', reason: `could not focus: ${unjudged.slice(0, 4).join('; ')}`, measured }; + } + return { + status: 'pass', + reason: `focus by click and Tab marks the box of all ${enabled.length} field(s), eased, with no inner outline or movement` + + (measured.disabled ? `; ${measured.disabled} disabled field(s) not focused` : ''), + measured, + }; +} diff --git a/harness/runtime-audit/run.mjs b/harness/runtime-audit/run.mjs index c7deb8a7..2efca212 100644 --- a/harness/runtime-audit/run.mjs +++ b/harness/runtime-audit/run.mjs @@ -21,6 +21,7 @@ import { assertSelection } from './selection-checks.mjs'; import { installMotionProbe, runMotionCheck } from './motion-checks.mjs'; import { runDataBindingCheck } from './data-checks.mjs'; import { sourceDigest } from './source-digest.mjs'; +import { evaluateInputFocus, observeInputFocus } from './input-focus-checks.mjs'; import { ERROR_PAGE_TEXT, STATE_PROPERTIES, evaluateBounds, evaluateHidden, evaluateCheck, evaluateClippedText, evaluateDimensions, evaluateInteraction, evaluatePageHealth, evaluateTextAlignment, evaluateSequenceOrder, evaluateSectionLayout, evaluateStylesApplied, evaluateNoLowContrastText, evaluateRenderedCompleteness, evaluateViewTransition, summarize, @@ -1056,7 +1057,7 @@ async function runViewport(browser, plan, viewport, out, deadline, references, r record.checks.push({ ...item, ...evaluateClippedText(await clippedText(page, check.subject)) }); continue; } - if (check.kind === 'interaction') continue; + if (check.kind === 'interaction' || check.kind === 'input-focus') continue; if (check.kind === 'capture') { const target = locate(page, check.subject); const count = await target.count(); @@ -1111,6 +1112,21 @@ async function runViewport(browser, plan, viewport, out, deadline, references, r ? { status: 'inconclusive', reason: `the subject could not be ${check.state === 'focus' ? 'focused' : 'hovered'}: ${observed.error.split('\n')[0]}`, measured: {} } : evaluateInteraction(check, observed)) }); } + // Fields focused by click and by Tab, after the interaction checks so + // their hover and focus have been left. + for (const [index, check] of screen.checks.entries()) { + if (check.kind !== 'input-focus' || (check.widths?.length && !check.widths.includes(viewport.width))) continue; + const item = { index, kind: check.kind, label: check.label }; + const scope = check.subject ? locate(page, check.subject) : page.locator('body'); + if ((await scope.count()) !== 1) { + record.checks.push({ ...item, status: 'inconclusive', reason: 'subject must match exactly one element', measured: {} }); + continue; + } + const observed = await observeInputFocus(page, scope, { timeout: stepTimeout }).catch((error) => ({ error: String(error?.message ?? error) })); + record.checks.push({ ...item, ...(observed.error + ? { status: 'inconclusive', reason: `fields could not be focused: ${observed.error.split('\n')[0]}`, measured: {} } + : evaluateInputFocus(observed)) }); + } // Links on this page, so a dynamic route ([slug]) can be audited at the // URLs the site itself links to. const anchors = await page.evaluate(() => [...document.querySelectorAll('a[href]')].flatMap((anchor) => { diff --git a/harness/runtime-audit/test/input-focus.test.mjs b/harness/runtime-audit/test/input-focus.test.mjs new file mode 100644 index 00000000..cf17eaa9 --- /dev/null +++ b/harness/runtime-audit/test/input-focus.test.mjs @@ -0,0 +1,163 @@ +// input-focus: a field's focus must mark the box the user sees, by click and +// by Tab, with no inner default outline, no movement and focus left on the +// field. Unit cases pin the judgement; the browser case runs fixtures/fields.html. + +import assert from 'node:assert/strict'; +import { mkdtemp, readFile, rm, writeFile } from 'node:fs/promises'; +import { createServer } from 'node:http'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import test from 'node:test'; +import { evaluateInputFocus, judgeFocus, observeInputFocus, snappedProperties } from '../input-focus-checks.mjs'; +import { runPlan } from '../run.mjs'; + +const FIXTURES = new URL('../fixtures/', import.meta.url); + +const paint = (overrides = {}) => ({ + borderTopColor: 'rgb(196, 196, 196)', borderRightColor: 'rgb(196, 196, 196)', borderBottomColor: 'rgb(196, 196, 196)', borderLeftColor: 'rgb(196, 196, 196)', + borderTopWidth: '1px', borderRightWidth: '1px', borderBottomWidth: '1px', borderLeftWidth: '1px', + boxShadow: 'none', outlineStyle: 'none', outlineWidth: '0px', outlineColor: 'rgb(0, 0, 0)', backgroundColor: 'rgb(255, 255, 255)', + ...overrides, +}); +const rect = (x = 0, y = 0, width = 320, height = 40) => ({ x, y, width, height }); +const state = ({ field = {}, container = {}, focus = 'field', fieldRect = rect(28, 0, 280, 40), boxRect = rect(), neighbour = rect(0, 64) } = {}) => ({ + field: { paint: paint({ borderTopWidth: '0px', ...field }), rect: fieldRect }, + container: { paint: paint(container), rect: boxRect }, + neighbour, + focus, +}); +const ring = { borderTopColor: 'rgb(0, 87, 187)', boxShadow: 'rgb(0, 87, 187) 0px 0px 0px 2px' }; +const outline = { outlineStyle: 'auto', outlineWidth: '1px', outlineColor: 'rgb(16, 16, 16)' }; +const wrapped = { tag: 'input', separateContainer: true, containerTag: 'div' }; +const own = { tag: 'input', separateContainer: false, containerTag: 'input' }; + +test('a field in a box passes only when the box shows focus and the field adds no outline', () => { + const before = state(); + assert.deepEqual(judgeFocus(wrapped, { before, after: state({ container: ring }) }), []); + assert.match(judgeFocus(wrapped, { before, after: state({ field: outline }) })[0], /marks only the inner input, not the
box/); + assert.match(judgeFocus(wrapped, { before, after: state({ container: ring, field: outline }) })[0], /second outline shows on the inner input/); + assert.match(judgeFocus(wrapped, { before, after: state() })[0], /no focus indicator: neither the input nor its
box changes/); +}); + +test('a field that is its own box passes on its own ring; focus elsewhere or movement fails', () => { + const before = state({ container: {} }); + assert.deepEqual(judgeFocus(own, { before, after: state({ container: ring }) }), []); + assert.match(judgeFocus(own, { before, after: state() })[0], /nothing on the field changes/); + assert.match(judgeFocus(wrapped, { before, after: state({ container: ring, focus: 'container' }) })[0], /moved to the container/); + assert.match(judgeFocus(wrapped, { before, after: state({ container: ring, focus: 'button' }) })[0], /did not reach the field/); + assert.match(judgeFocus(own, { before, after: state({ container: { ...ring, borderTopWidth: '3px' }, boxRect: rect(0, 0, 324, 44) }) })[0], /changes size or position/); + assert.match(judgeFocus(own, { before, after: state({ container: ring, neighbour: rect(0, 66) }) })[0], /element after the field moves/); + assert.match(judgeFocus(wrapped, { before, after: state({ container: ring }), containerTabStop: true })[0], /Tab stops on a container/); +}); + +test('a focus state that changes paint no transition covers snaps', () => { + const changedProps = ['borderTopColor', 'borderLeftColor', 'boxShadow', 'outlineStyle']; + assert.deepEqual(snappedProperties(changedProps, { property: 'all', duration: '0s' }), ['borderTopColor', 'borderLeftColor', 'boxShadow']); + assert.deepEqual(snappedProperties(changedProps, { property: 'all', duration: '0.16s' }), []); + assert.deepEqual(snappedProperties(changedProps, { property: 'border-color, box-shadow', duration: '0.16s' }), []); + assert.deepEqual(snappedProperties(changedProps, { property: 'border', duration: '0.2s' }), ['boxShadow']); + assert.deepEqual(snappedProperties(changedProps, { property: 'box-shadow, border-color', duration: '0.2s, 0s' }), ['borderTopColor', 'borderLeftColor']); + assert.deepEqual(snappedProperties(changedProps, undefined), []); + const snaps = { mode: 'click', before: state(), after: state({ container: ring }) }; + snaps.after.container.motion = { property: 'all', duration: '0s' }; + assert.match(judgeFocus(wrapped, snaps).join(), /snaps with no transition on border-color, box-shadow/); +}); + +test('the evaluation fails on any field and mode, counts disabled fields, and is inconclusive when nothing could be judged', () => { + const pass = { mode: 'click', before: state(), after: state({ container: ring }) }; + const fail = { mode: 'tab', before: state(), after: state({ field: outline }) }; + const fields = [ + { ...wrapped, index: 0, name: 'Search', modes: [pass, { ...pass, mode: 'tab' }] }, + { ...own, index: 1, name: 'Locked', disabled: true, modes: [] }, + ]; + const ok = evaluateInputFocus({ fields }); + assert.equal(ok.status, 'pass', ok.reason); + assert.match(ok.reason, /all 1 field\(s\).*1 disabled field/); + assert.equal(ok.measured.disabled, 1); + const bad = evaluateInputFocus({ fields: [{ ...fields[0], modes: [pass, fail] }] }); + assert.equal(bad.status, 'fail'); + assert.match(bad.reason, / "Search" by tab: focus marks only the inner input/); + assert.equal(evaluateInputFocus({ fields: [{ ...fields[0], modes: [{ mode: 'tab', unreachable: true }] }] }).status, 'inconclusive'); + assert.equal(evaluateInputFocus({ fields: [] }).status, 'pass'); +}); + +function serve() { + const server = createServer(async (request, response) => { + const name = new URL(request.url, 'http://local').pathname.slice(1); + try { + response.writeHead(200, { 'content-type': 'text/html; charset=utf-8' }); + response.end(await readFile(new URL(name, FIXTURES))); + } catch { + response.writeHead(404).end(); + } + }); + return new Promise((resolve) => server.listen(0, '127.0.0.1', () => resolve(server))); +} + +test('in a real browser, click and Tab focus are judged per field layout', async () => { + const server = await serve(); + const { chromium } = await import('playwright'); + const browser = await chromium.launch(); + try { + const page = await browser.newPage({ viewport: { width: 800, height: 900 } }); + await page.goto(`http://127.0.0.1:${server.address().port}/fields.html`); + const judge = async (id) => evaluateInputFocus(await observeInputFocus(page, page.locator(`#${id}`))); + const results = {}; + for (const id of ['own', 'wrapped-good', 'wrapped-bad', 'snap', 'wrapped-dup', 'textarea', 'states', 'shift', 'moved', 'select']) { + results[id] = await judge(id); + } + const summary = Object.fromEntries(Object.entries(results).map(([id, result]) => [id, result.status])); + assert.deepEqual(summary, { + own: 'pass', 'wrapped-good': 'pass', 'wrapped-bad': 'fail', snap: 'fail', 'wrapped-dup': 'fail', textarea: 'pass', + states: 'pass', shift: 'fail', moved: 'fail', select: 'pass', + }, JSON.stringify(results, null, 2)); + // Both modes ran on every enabled field. + for (const [id, result] of Object.entries(results)) { + for (const field of result.measured.fields.filter((field) => !field.disabled)) { + assert.deepEqual(field.modes.map((mode) => mode.mode), ['click', 'tab'], id); + } + } + assert.equal(results['wrapped-good'].measured.fields[0].containerTag, 'div'); + assert.match(results['wrapped-bad'].reason, /by click: focus marks only the inner input, not the
box/); + assert.match(results['wrapped-bad'].reason, /by tab: focus marks only the inner input/); + assert.match(results['wrapped-dup'].reason, /second outline shows on the inner input/); + assert.match(results.snap.reason, /by click: the focus state snaps with no transition on border-color, box-shadow/); + assert.doesNotMatch(results['wrapped-good'].reason, /snaps/); + assert.match(results.shift.reason, /changes size or position/); + assert.match(results.moved.reason, /Tab stops on a container/); + assert.equal(results.states.measured.disabled, 1); + assert.ok(results.states.measured.fields.some((field) => field.invalid && !field.disabled)); + // The probe leaves no marker behind and the page keeps its values. + assert.equal(await page.locator('[data-devup-focus-probe]').count(), 0); + assert.equal(await page.locator('#states input').first().inputValue(), 'not-an-email'); + } finally { + await browser.close(); + server.close(); + } +}); + +test('the executor runs an input-focus check from a plan at its widths', async () => { + const server = await serve(); + const baseUrl = `http://127.0.0.1:${server.address().port}`; + const out = await mkdtemp(join(tmpdir(), 'devup-input-focus-')); + try { + const plan = join(out, 'plan.json'); + await writeFile(plan, JSON.stringify({ schemaVersion: 1, kind: 'devup-runtime-audit-plan', planId: 'input-focus', plan: { + baseUrl, allowedOrigins: [baseUrl], viewports: [{ width: 800, height: 900 }, { width: 400, height: 900 }], + screens: [{ id: 'fields', routePath: '/fields.html', checks: [ + { kind: 'input-focus', label: 'good box', subject: { css: '#wrapped-good' }, widths: [800] }, + { kind: 'input-focus', label: 'bad box', subject: { css: '#wrapped-bad' }, widths: [800] }, + { kind: 'input-focus', label: 'missing', subject: { css: '#nothing' }, widths: [800] }, + ] }], + } })); + const report = await runPlan({ plan, out, maxMinutes: 2 }); + const [wide, narrow] = report.runs; + assert.deepEqual(wide.screens[0].checks.map((check) => [check.label, check.status]), [ + ['good box', 'pass'], ['bad box', 'fail'], ['missing', 'inconclusive'], + ], JSON.stringify(wide.screens[0].checks, null, 2)); + assert.equal(narrow.screens[0].checks.filter((check) => check.kind === 'input-focus').length, 0); + } finally { + server.close(); + await rm(out, { recursive: true, force: true }); + } +}); From c70d8fc6ef85050aff34bfad613d9d04cb6879eb Mon Sep 17 00:00:00 2001 From: leader Date: Thu, 12 Nov 2026 12:12:10 +0900 Subject: [PATCH 2/2] feat: enforce the plan procedure with user approval and a procedure block - Screen and project plan confirm require the user's approval receipt (via, verbatim answer, approved URLs); page writes are refused until a confirmed plan carries it. - Every devup_screen_plan reply and refused export carries a procedure block: step of 5, the one next call, waitForUser and forbiddenNow. - site:motion recommends predicting motion from the design; status adds motion and field-state-motion completion items. - Skills, instructions, delivery-quality contract (v6) and manifest updated. Co-Authored-By: Claude Opus 5.5 --- .../changepack_log_Uv5MsA9cJr2GhN7wQe4Kp.json | 7 + .../src/server/commands/opencode-devup.md | 8 +- .../src/server/development_contract.rs | 8 +- .../src/server/handlers/devup_figma_export.rs | 5 +- .../src/server/handlers/devup_project_plan.rs | 12 ++ .../src/server/handlers/devup_screen_plan.rs | 8 +- .../devup-mcp/src/server/project_plan/mod.rs | 14 +- crates/devup-mcp/src/server/screen_plan.rs | 2 + .../src/server/screen_plan/actions.rs | 72 ++++++- .../src/server/screen_plan/app_select.rs | 33 +++- .../devup-mcp/src/server/screen_plan/draft.rs | 8 +- .../src/server/screen_plan/procedure.rs | 62 ++++++ .../devup-mcp/src/server/screen_plan/state.rs | 6 + .../src/server/screen_plan/status.rs | 12 +- .../src/server/screen_plan/user_approval.rs | 181 ++++++++++++++++++ .../devup-mcp/src/server/screen_plan_tests.rs | 2 + ...oup_answer_can_name_one_route_per_frame.rs | 2 +- .../screen_plan_tests/fixtures_and_cases.rs | 1 + .../screen_plan_tests/motion_completeness.rs | 28 +++ .../server/screen_plan_tests/user_approval.rs | 148 ++++++++++++++ .../references/delivery-quality.md | 26 +++ .../skills/devup-implementation-plan/SKILL.md | 10 +- .../src/server/skills/devup/SKILL.md | 28 ++- .../devup-mcp/src/server/skills/manifest.json | 12 +- .../src/server/text/devup-screen-plan.md | 2 +- .../text/guide-INSTRUCTIONS-49bd7ec6.md | 13 +- .../devup-mcp/tests/export_project_theme.rs | 2 +- crates/devup-mcp/tests/screen_plan_tool.rs | 54 +++++- 28 files changed, 723 insertions(+), 43 deletions(-) create mode 100644 .changepacks/changepack_log_Uv5MsA9cJr2GhN7wQe4Kp.json create mode 100644 crates/devup-mcp/src/server/screen_plan/procedure.rs create mode 100644 crates/devup-mcp/src/server/screen_plan/user_approval.rs create mode 100644 crates/devup-mcp/src/server/screen_plan_tests/motion_completeness.rs create mode 100644 crates/devup-mcp/src/server/screen_plan_tests/user_approval.rs diff --git a/.changepacks/changepack_log_Uv5MsA9cJr2GhN7wQe4Kp.json b/.changepacks/changepack_log_Uv5MsA9cJr2GhN7wQe4Kp.json new file mode 100644 index 00000000..d3332629 --- /dev/null +++ b/.changepacks/changepack_log_Uv5MsA9cJr2GhN7wQe4Kp.json @@ -0,0 +1,7 @@ +{ + "changes": { + "Cargo.toml": "Minor" + }, + "note": "The plan gates can no longer be self-confirmed. devup_screen_plan confirm through the tool requires userApproval {via: question-tool | user-message | delegated, userAnswer: the user's verbatim answer, approvedUrls: every URL of the table shown}; a missing or placeholder receipt, or URLs that differ from what confirm builds, confirms nothing and returns the table and expectedUrls to show. devup_project_plan confirm requires the same receipt (without URLs). The receipt is written to the plan and printed in confirm, status and the finalReport (delegated or missing receipts are reported as not reviewed by the user). Draft nextAction carries mustAskUser and the receipt template; the server instructions, the devup and devup-implementation-plan skills and the OpenCode command state the hard stop. Contract identifier user-approved-plan-before-implementation (developmentContract v6). A confirmed plan without the user's receipt writes no page: devup_figma_export page writes return status approval_required until confirm records the approval. Every devup_screen_plan reply and every refused export carry a procedure block (step of 5, the one next call, waitForUser, forbiddenNow) computed from the plan on disk. The site:motion question now recommends Predict from the design (motion source predicted, Devup UI static keyframes), and status adds motion and field-state-motion completion items that stay missing until the sources carry them.", + "date": "2026-11-12T00:00:00.000000000Z" +} diff --git a/crates/devup-mcp/src/server/commands/opencode-devup.md b/crates/devup-mcp/src/server/commands/opencode-devup.md index ad1cb63f..e3fcf8b7 100644 --- a/crates/devup-mcp/src/server/commands/opencode-devup.md +++ b/crates/devup-mcp/src/server/commands/opencode-devup.md @@ -110,7 +110,13 @@ For Figma work, continue with the following branch after project intake: 5. Ask every question now with the `question` tool: the flat `questions` list in one call. Never in prose. New user-owned decisions after confirm still use the question tool; workers escalate to the supervisor. -6. `devup_screen_plan` confirm with every answer that applies. + Stop there and wait for the answer; never answer for the user. +6. `devup_screen_plan` confirm with every answer that applies and + `userApproval: {via: "question-tool", userAnswer: "", approvedUrls: []}`. Without it, + or with URLs that differ from what confirm builds, nothing is confirmed: + show the returned table and ask again. `devup_project_plan` confirm takes + the same receipt (without approvedUrls). 7. Latest first, without asking: at the repository root run `bunx @dependency-check-updates/cli -d -u --rm` - it updates every manifest in the repository, every bun workspace and Cargo.toml included, diff --git a/crates/devup-mcp/src/server/development_contract.rs b/crates/devup-mcp/src/server/development_contract.rs index e30dd750..49ee326b 100644 --- a/crates/devup-mcp/src/server/development_contract.rs +++ b/crates/devup-mcp/src/server/development_contract.rs @@ -22,6 +22,7 @@ pub(super) const REQUIREMENTS: &[&str] = &[ "preserve-instance-sizing-and-paint", "paired-e2e-and-native-figma-visuals", "semantic-native-controls-and-complete-pseudo-states", + "field-focus-on-visual-container", "variant-palette-consistency", "single-source-url-content-selection", "viewport-footer-shell", @@ -31,6 +32,7 @@ pub(super) const REQUIREMENTS: &[&str] = &[ "one-shot-reveals-without-ssr-flash", "canonical-semantic-assets", "landing-detail-api-db-single-source", + "user-approved-plan-before-implementation", "runtime-evidence-before-completion", "offer-devup-retro-in-final-report", ]; @@ -49,7 +51,7 @@ pub(super) const COMPLETION_EVIDENCE: &[&str] = &[ pub(super) fn attach(result: &mut Value) { if !result.is_object() { return; } result["developmentContract"] = json!({ - "version": 5, + "version": 6, "guide": "devup://guide/usage#behavior-and-data-contracts", "alwaysApplies": true, "requirements": REQUIREMENTS, @@ -67,7 +69,7 @@ mod tests { attach(&mut result); assert_eq!(result["developmentContract"]["alwaysApplies"], true); assert_eq!(result["developmentContract"]["staticExportProvesApplicationBehavior"], false); - assert_eq!(result["developmentContract"]["version"], 5); + assert_eq!(result["developmentContract"]["version"], 6); let requirements = result["developmentContract"]["requirements"].as_array().unwrap(); for required in ["route-owned-page-body", "devup-api-only", "stop-affected-api-gaps", "paired-e2e-and-native-figma-visuals"] { assert!(requirements.contains(&json!(required))); @@ -142,6 +144,6 @@ yourself", "carries no offer", super::super::skills::RETRO_OFFER_FIELD] { unique.sort(); unique.dedup(); assert_eq!(unique.len(), REQUIREMENTS.len() + COMPLETION_EVIDENCE.len()); - assert_eq!((REQUIREMENTS.len(), COMPLETION_EVIDENCE.len()), (21, 8)); + assert_eq!((REQUIREMENTS.len(), COMPLETION_EVIDENCE.len()), (23, 8)); } } diff --git a/crates/devup-mcp/src/server/handlers/devup_figma_export.rs b/crates/devup-mcp/src/server/handlers/devup_figma_export.rs index 5cd634b8..5eba5d85 100644 --- a/crates/devup-mcp/src/server/handlers/devup_figma_export.rs +++ b/crates/devup-mcp/src/server/handlers/devup_figma_export.rs @@ -86,7 +86,10 @@ impl DevupServer { }; match screen_plan::export_gate(&request).map_err(to_mcp_error)? { screen_plan::Gate::Open => {} - screen_plan::Gate::Blocked(response) => return Ok(tool_result(response)), + screen_plan::Gate::Blocked(mut response) => { + response["procedure"] = screen_plan::procedure(&project_root); + return Ok(tool_result(response)); + } screen_plan::Gate::Route(route) => { if let Some(scaffold) = input.page_scaffold.as_mut() { scaffold.route = route; diff --git a/crates/devup-mcp/src/server/handlers/devup_project_plan.rs b/crates/devup-mcp/src/server/handlers/devup_project_plan.rs index fc39d7bf..7e4edc66 100644 --- a/crates/devup-mcp/src/server/handlers/devup_project_plan.rs +++ b/crates/devup-mcp/src/server/handlers/devup_project_plan.rs @@ -10,6 +10,18 @@ impl DevupServer { &self, Parameters(input): Parameters, ) -> Result { + // The tool confirms only with the user's approval; nothing is read or + // written before that is shown. + if input.action == "confirm" { + screen_plan::approval_receipt( + input.user_approval.as_ref(), + true, + "project plan (scope, ERD and delivery answers)", + &json!("Show the project plan status (evidence table, ERD Mermaid and every recommended answer) first."), + &[], + ) + .map_err(to_mcp_error)?; + } let result = project_plan::run(&input, &self.output_policy) .await .map_err(to_mcp_error)?; diff --git a/crates/devup-mcp/src/server/handlers/devup_screen_plan.rs b/crates/devup-mcp/src/server/handlers/devup_screen_plan.rs index c6495291..cc215459 100644 --- a/crates/devup-mcp/src/server/handlers/devup_screen_plan.rs +++ b/crates/devup-mcp/src/server/handlers/devup_screen_plan.rs @@ -25,7 +25,7 @@ impl DevupServer { screen_plan::draft_with(&input, &screens, &assets, source, &self.output_policy) .await } - "confirm" => screen_plan::confirm(&input, &self.output_policy), + "confirm" => screen_plan::confirm_approved(&input, &self.output_policy), "assume" => screen_plan::assume(&input, &self.output_policy), "status" => screen_plan::status(&input, &self.output_policy, &self.artifacts).await, "auditPlan" => { @@ -47,6 +47,12 @@ impl DevupServer { { prefer_bridge_reference_calls(&mut result); } + // Every reply says where the workflow stands and the one call to make next. + if let Some(root) = input.project_root.as_deref().filter(|root| !root.trim().is_empty()) + && result.is_object() + { + result["procedure"] = screen_plan::procedure(root); + } // Status is done only after application completion was re-evaluated. let complete = skills::marks_completion("devup_screen_plan", &input.action, &result); self.offer_retro(&mut result, input.project_root.as_deref(), complete); diff --git a/crates/devup-mcp/src/server/project_plan/mod.rs b/crates/devup-mcp/src/server/project_plan/mod.rs index 57b41408..ecc93039 100644 --- a/crates/devup-mcp/src/server/project_plan/mod.rs +++ b/crates/devup-mcp/src/server/project_plan/mod.rs @@ -45,6 +45,10 @@ pub struct ProjectPlanInput { /// missing from `answers` takes its recommended option. #[serde(default)] pub accept_recommended: bool, + /// confirm (required): the user's approval of the plan - how they + /// answered and their own words. Without it nothing is confirmed. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub user_approval: Option, #[serde(default)] pub features: Vec, /// Explicit ERD models in vespertide wire format; confirm may refine the draft. @@ -325,8 +329,9 @@ fn forward_screen_answers( } let forward = json!({"tool":"devup_screen_plan","arguments":{ "action":"confirm","planId":screen["planId"],"projectRoot":root, - "answers":screen_answers,"acceptRecommended":accept_recommended}, - "how":"These answers belong to the screen plan: send them with this call. The user was asked once, in the merged proposal."}); + "answers":screen_answers,"acceptRecommended":accept_recommended, + "userApproval":{"via":"question-tool","userAnswer":"","approvedUrls":[""]}}, + "how":"These answers belong to the screen plan: send them with this call, with userApproval from the user's answer to the merged proposal (it showed every screen and URL).","mustAskUser":true}); (own, forward) } fn without_screens(plan: &Value) -> Vec { @@ -659,7 +664,9 @@ pub(super) async fn run( .map(|q| q["id"].clone()).collect::>(), "nextAction":{"tool":"devup_project_plan","arguments":{ "action":"confirm","projectRoot":input.project_root, - "planId":plan["planId"],"answers":{},"acceptRecommended":true}} + "planId":plan["planId"],"answers":{},"acceptRecommended":true, + "userApproval":{"via":"question-tool","userAnswer":""}}, + "how":screen_plan::APPROVAL_INSTRUCTION} }); error })?; @@ -691,6 +698,7 @@ pub(super) async fn run( ))); } wiring::draft(&mut plan); + plan["userApproval"] = screen_plan::approval_record(input.user_approval.as_ref()); erd::write_models(&mut tx, policy, &input.project_root, &plan)?; plan["writtenTo"] = json!(if erd::api(&plan) { array(&plan["entities"]).iter() diff --git a/crates/devup-mcp/src/server/screen_plan.rs b/crates/devup-mcp/src/server/screen_plan.rs index a00f960d..e6e87d52 100644 --- a/crates/devup-mcp/src/server/screen_plan.rs +++ b/crates/devup-mcp/src/server/screen_plan.rs @@ -43,3 +43,5 @@ include!("screen_plan/lookup.rs"); include!("screen_plan/component_width.rs"); include!("screen_plan/responsive_groups.rs"); include!("screen_plan/responsive_contract.rs"); +include!("screen_plan/user_approval.rs"); +include!("screen_plan/procedure.rs"); diff --git a/crates/devup-mcp/src/server/screen_plan/actions.rs b/crates/devup-mcp/src/server/screen_plan/actions.rs index e55d3db5..1f4e7bd7 100644 --- a/crates/devup-mcp/src/server/screen_plan/actions.rs +++ b/crates/devup-mcp/src/server/screen_plan/actions.rs @@ -1,6 +1,29 @@ /// `confirm`: every question that applies answered, in one call, or nothing -/// changes. +/// changes. A `userApproval` sent here is checked and recorded, but not +/// required; the tool requires it ([`confirm_approved`]). Tests drive the +/// plan logic through this one. +/// A plan confirmed here without a receipt is recorded as approved by a test +/// user, so the export gate's other rules stay testable; the gate itself is +/// tested by removing the receipt. +#[cfg(test)] pub(super) fn confirm(input: &ScreenPlanInput, policy: &OutputPolicy) -> Result { + let result = confirm_with(input, policy, false)?; + if input.user_approval.is_none() && result["alreadyConfirmed"] != true { + let (plan_root, mut plan) = audit_plan_location(input)?; + plan["userApproval"] = json!({"via": "question-tool", "userAnswer": "Accept (test user)", + "approvedUrls": plan_urls(&plan), "reviewedByUser": true, "recordedAt": now()}); + write_plan(policy, plan_root.to_str().unwrap_or_default(), &plan)?; + } + Ok(result) +} + +/// `confirm` as the tool runs it: refused unless `userApproval` shows the +/// user approved exactly the URLs that are built. +pub(super) fn confirm_approved(input: &ScreenPlanInput, policy: &OutputPolicy) -> Result { + confirm_with(input, policy, true) +} + +fn confirm_with(input: &ScreenPlanInput, policy: &OutputPolicy, approval_required: bool) -> Result { let (plan_root, mut plan) = audit_plan_location(input)?; let root = plan_root.to_str().ok_or_else(|| invalid("The plan root is not UTF-8."))?; let plan_id = input @@ -16,15 +39,35 @@ pub(super) fn confirm(input: &ScreenPlanInput, policy: &OutputPolicy) -> Result< )); } if plan["status"] == "confirmed" && input.answers.is_empty() { + // A plan confirmed before receipts existed (or without one) takes the + // user's approval of its URLs now; pages stay blocked until it has. + let table = json!(route_markdown(array(&plan["routes"]))); + let built = plan_urls(&plan); + let mut written = Value::Null; + if let Some(approval) = approval_receipt(input.user_approval.as_ref(), false, "screens and their URLs", &table, &built)? { + check_approved_urls(approval, &built, &table)?; + plan["userApproval"] = approval_record(Some(approval)); + written = json!(write_plan(policy, root, &plan)?); + } + let approved = has_user_approval(&plan); return Ok(json!({ "status": "confirmed", "alreadyConfirmed": true, "planId": plan["planId"], "confirmedAt": plan["confirmedAt"], "routes": plan["routes"], - "markdownTable": route_markdown(array(&plan["routes"])), - "nextAction": {"tool": "devup_screen_plan", "arguments": {"action": "status", "projectRoot": root, "planId": plan_id}, - "how": "Nothing changed. The plan was confirmed before; continue implementation from its status."}, + "markdownTable": table, + "userApproval": plan["userApproval"], + "approval": approval_line(&plan["userApproval"]), + "writtenTo": written, + "nextAction": if approved { + json!({"tool": "devup_screen_plan", "arguments": {"action": "status", "projectRoot": root, "planId": plan_id}, + "how": "The plan was confirmed before and carries the user's approval; continue implementation from its status."}) + } else { + json!({"tool": "devup_screen_plan", "arguments": {"action": "confirm", "projectRoot": root, "planId": plan_id, "answers": {}, + "userApproval": {"via": "question-tool", "userAnswer": "", "approvedUrls": built}}, + "how": APPROVAL_INSTRUCTION, "mustAskUser": true}) + }, })); } let mut questions = array(&plan["questions"]).to_vec(); @@ -386,6 +429,25 @@ pub(super) fn confirm(input: &ScreenPlanInput, policy: &OutputPolicy) -> Result< .find(|frame| frame["pageId"] == page).map(|frame| frame["pageName"].clone()); plan["confirmedPage"] = json!({"id":page,"name":name,"basis":"base-page answer"}); } + // The user approves the URLs that are built, not a table they never saw. + let table = json!(route_markdown(&routes)); + let built = routes + .iter() + .filter_map(|route| route["url"].as_str().map(str::to_owned)) + .collect::>() + .into_iter() + .collect::>(); + let approval = approval_receipt( + input.user_approval.as_ref(), + approval_required, + "screens and their URLs", + &table, + &built, + )?; + if let Some(approval) = approval { + check_approved_urls(approval, &built, &table)?; + } + plan["userApproval"] = approval_record(approval); plan["status"] = json!("confirmed"); plan["confirmedAt"] = json!(now()); refresh_inventory_checks(&mut plan); @@ -421,6 +483,8 @@ pub(super) fn confirm(input: &ScreenPlanInput, policy: &OutputPolicy) -> Result< "confirmedPage": plan["confirmedPage"], "routes": routes, "markdownTable": route_markdown(&routes), + "userApproval": plan["userApproval"], + "approval": approval_line(&plan["userApproval"]), "frameInventory": plan["frameInventory"], "linkLabelCheck": plan["linkLabelCheck"], "excluded": excluded, diff --git a/crates/devup-mcp/src/server/screen_plan/app_select.rs b/crates/devup-mcp/src/server/screen_plan/app_select.rs index cc7b0d14..d914da0c 100644 --- a/crates/devup-mcp/src/server/screen_plan/app_select.rs +++ b/crates/devup-mcp/src/server/screen_plan/app_select.rs @@ -698,9 +698,10 @@ fn site_questions( "motion", "site:motion", "Motion beyond hover", - "Beyond hover, focus and page transitions, should sections reveal on scroll? Nothing in the plan shows designed motion.".into(), + "Beyond hover, focus and page transitions, how should the screens move? The recommended answer reads the motion the design implies.".into(), vec![ - choice("None", "Recommended unless the design shows motion; hover and focus transitions are still added.", json!({"motion": {"source": "none"}})), + choice("Predict from the design", "Read each frame's prototype reactions and layout (hero, card grids, lists, counters, logo rows, tabs) and apply the motion they imply - entrance reveals, count-up, marquee, eased hover/focus/field states - from behaviorPlan motion candidates, as Devup UI static keyframes and transitions (no animation library), off under prefers-reduced-motion; the runtime audit motion check proves each one.", json!({"motion": {"source": "predicted", "implementation": "devup-ui-static-keyframes"}})), + choice("None", "Only hover, focus and field-state transitions; nothing reveals or loops.", json!({"motion": {"source": "none"}})), choice("Subtle reveal on scroll", "Sections fade and slide in once as they enter, as Devup UI static keyframes on a view timeline (CSS only, no animation library); off under prefers-reduced-motion.", json!({"motion": {"source": "reveal", "implementation": "devup-ui-static-keyframes"}})), ], )); @@ -763,6 +764,8 @@ fn site_completeness(plan: &Value, evidence: &ProjectEvidence) -> Vec { let mut motion = false; let mut reduced = false; let mut transitions = false; + let mut keyframes = false; + let mut snapping_fields = Vec::new(); for relative in &evidence.sources { let Some(source) = read_small(&join_relative(&evidence.root, relative)) else { continue; @@ -776,6 +779,13 @@ fn site_completeness(plan: &Value, evidence: &ProjectEvidence) -> Vec { || source.contains("transform") || source.contains("animation"); reduced |= source.contains("_motionReduce") || source.contains("prefers-reduced-motion"); + keyframes |= source.contains("keyframes") || source.contains("animationName"); + let fields = ["as=\"input\"", "as=\"textarea\"", "as=\"select\"", " Vec { items.push(json!({"item": "reduced-motion", "state": if reduced { "filled" } else if motion { "missing" } else { "not-applicable" }, "detail": if reduced { "prefers-reduced-motion is handled" } else if motion { "transitions or transforms with no prefers-reduced-motion handling" } else { "no motion in the sources" }})); + // The motion the user chose is in the sources: a predicted or reveal + // answer needs keyframes, and every file with a field eases its states. + let chosen = site["motion"]["answer"]["motion"]["source"] + .as_str() + .or_else(|| site["motion"]["answer"]["source"].as_str()) + .or_else(|| site["motion"]["value"].as_str()); + items.push(match chosen { + Some("none") => json!({"item": "motion", "state": "not-applicable", "detail": "the user chose no motion beyond hover, focus and field states"}), + Some(_) if keyframes => json!({"item": "motion", "state": "filled", "detail": "keyframes are in the sources; the runtime audit motion check proves each candidate"}), + Some(_) => json!({"item": "motion", "state": "missing", "detail": "motion was chosen (predicted from the design or reveal) but no keyframes are in the sources yet: implement the behaviorPlan motion candidates"}), + None if decided("motion") || site["motion"].is_null() => json!({"item": "motion", "state": "not-applicable", "detail": "no motion decision in this plan"}), + None => json!({"item": "motion", "state": "missing", "detail": "the site:motion question is unanswered"}), + }); + items.push(if snapping_fields.is_empty() { + json!({"item": "field-state-motion", "state": "filled", "detail": "every source with a field declares a transition, or there is no field"}) + } else { + json!({"item": "field-state-motion", "state": "missing", + "detail": format!("fields snap between rest, focus, invalid and disabled with no transition in: {} (transition=\"160ms ease-out\" with transitionProperty on the box that changes)", snapping_fields.join(", "))}) + }); // Decided again from what is installed now: the plan was drafted before // the dependency update, and the update is what makes it possible. let planned = view_transition_default(evidence); diff --git a/crates/devup-mcp/src/server/screen_plan/draft.rs b/crates/devup-mcp/src/server/screen_plan/draft.rs index 2fc76c95..a3918667 100644 --- a/crates/devup-mcp/src/server/screen_plan/draft.rs +++ b/crates/devup-mcp/src/server/screen_plan/draft.rs @@ -569,11 +569,11 @@ pub(super) async fn draft_with( "writtenTo": written, "replacedConfirmedPlan": replaced, "nextAction": if questions.is_empty() { - json!({"tool": "devup_screen_plan", "arguments": {"action": "confirm", "projectRoot": root, "planId": plan["planId"], "answers": {}}, - "how": "Nothing is open. Show the user the screens table, then confirm with empty answers."}) + json!({"tool": "devup_screen_plan", "arguments": {"action": "confirm", "projectRoot": root, "planId": plan["planId"], "answers": {}, "userApproval": {"via": "question-tool", "userAnswer": "", "approvedUrls": [""]}}, + "how": "Nothing is open, but the user still approves the URLs. Show the screens table (every screen and its URL) and ask with the question tool; confirm only after the user answers, with userApproval carrying their answer and every URL in the table.", "mustAskUser": true}) } else { - json!({"tool": "devup_screen_plan", "arguments": {"action": "confirm", "projectRoot": root, "planId": plan["planId"], "acceptRecommended": true, "answers": " chosen label or custom text>"}, - "how": ASK_INSTRUCTION}) + json!({"tool": "devup_screen_plan", "arguments": {"action": "confirm", "projectRoot": root, "planId": plan["planId"], "acceptRecommended": true, "answers": " chosen label or custom text>", "userApproval": {"via": "question-tool", "userAnswer": "", "approvedUrls": [""]}}, + "how": ASK_INSTRUCTION, "mustAskUser": true, "approvalRule": APPROVAL_INSTRUCTION}) }, })) } diff --git a/crates/devup-mcp/src/server/screen_plan/procedure.rs b/crates/devup-mcp/src/server/screen_plan/procedure.rs new file mode 100644 index 00000000..3aec31f8 --- /dev/null +++ b/crates/devup-mcp/src/server/screen_plan/procedure.rs @@ -0,0 +1,62 @@ +// Where the workflow stands, said the same way in every plan reply. +// +// A weaker model does not hold a long skill in mind; it follows the reply it +// just read. So every devup_screen_plan and devup_project_plan reply, and +// every export the gate refuses, carries one `procedure` block computed from +// the plan on disk: the step the project is at, the one call to make next, +// whether it must stop and wait for the user, and what is not allowed now. +// The block is derived, never stored, so it cannot drift from the plan. + +/// The procedure block for a project, from its screen plan on disk. +pub(crate) fn procedure(project_root: &str) -> Value { + let plan = read_plan(project_root).ok().flatten(); + let step = |index: usize, id: &str, next: Value, wait: bool, forbidden: &[&str]| { + json!({ + "step": format!("{index}/5 {id}"), + "steps": ["1 draft", "2 ask-user", "3 confirm-with-approval", "4 implement", "5 verify"], + "next": next, + "waitForUser": wait, + "forbiddenNow": forbidden, + "rule": "Follow `next` exactly. Do not skip a step, do not answer for the user, do not claim completion before status says completion.reportAllowed.", + }) + }; + let Some(plan) = plan else { + return step(1, "draft", + json!({"tool": "devup_screen_plan", "arguments": {"action": "draft", "projectRoot": project_root}, + "how": "Draft the screen plan from the Figma link (or the Bridge selection) before anything else."}), + false, + &["writing or exporting pages", "hand-writing TSX", "confirming anything"]); + }; + let plan_id = plan["planId"].clone(); + if plan["status"] != "confirmed" { + return step(2, "ask-user", + json!({"tool": "question-tool", + "how": "Show proposalMarkdown (every screen, its URL and each open decision with the recommended answer) in the user's language and ask ONE question with the runtime question tool. Then STOP and wait for the answer. Only after it: devup_screen_plan confirm with userApproval.", + "then": {"tool": "devup_screen_plan", "arguments": {"action": "confirm", "projectRoot": project_root, "planId": plan_id, + "acceptRecommended": true, "answers": "", + "userApproval": {"via": "question-tool", "userAnswer": "", "approvedUrls": ""}}}}), + true, + &["confirm in the same turn as the question", "confirm without the user's answer", "exporting screens or writing pages", "hand-writing TSX"]); + } + if !has_user_approval(&plan) { + return step(3, "confirm-with-approval", + json!({"tool": "devup_screen_plan", "arguments": {"action": "confirm", "projectRoot": project_root, "planId": plan_id, "answers": {}, + "userApproval": {"via": "question-tool", "userAnswer": "", "approvedUrls": plan_urls(&plan)}}, + "how": "The plan is confirmed but the user never approved its URLs. Show markdownTable, ask with the question tool, wait, then send this call with the user's answer."}), + true, + &["exporting screens or writing pages", "hand-writing TSX"]); + } + let audited = plan["audit"].is_object(); + if !audited { + return step(4, "implement", + json!({"tool": "devup_screen_plan", "arguments": {"action": "status", "projectRoot": project_root, "planId": plan_id}, + "how": "Build every URL in workQueue with devup_figma_export (components first, then sections), validate with devup_ui_validate, then auditPlan and the runtime audit. New decisions go to defer/assume, never to the user."}), + false, + &["asking the user (use defer or assume)", "hand-writing TSX from Figma nodes", "claiming completion"]); + } + step(5, "verify", + json!({"tool": "devup_screen_plan", "arguments": {"action": "status", "projectRoot": project_root, "planId": plan_id}, + "how": "Run the runtime audit for every URL, pass auditPlanPath/auditReportPath to status, fix findings in workQueue order and re-run. Report completion only when completion.reportAllowed is true."}), + false, + &["claiming completion before completion.reportAllowed", "asking the user (use defer or assume)"]) +} diff --git a/crates/devup-mcp/src/server/screen_plan/state.rs b/crates/devup-mcp/src/server/screen_plan/state.rs index eaa12dd9..43f02067 100644 --- a/crates/devup-mcp/src/server/screen_plan/state.rs +++ b/crates/devup-mcp/src/server/screen_plan/state.rs @@ -108,6 +108,12 @@ pub struct ScreenPlanInput { /// the rows the user changed need to be sent. #[serde(default)] pub accept_recommended: bool, + /// confirm (required through the tool): the user's approval of the + /// screens and URLs table - how they answered, their own words, and every + /// URL exactly as shown. Without it, or with URLs that differ from what + /// confirm builds, nothing is confirmed and the table to show is returned. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub user_approval: Option, /// status: the runtime audit plan JSON, together with auditReportPath. #[serde(default)] pub audit_plan_path: Option, diff --git a/crates/devup-mcp/src/server/screen_plan/status.rs b/crates/devup-mcp/src/server/screen_plan/status.rs index a2b8b6fa..ea87094b 100644 --- a/crates/devup-mcp/src/server/screen_plan/status.rs +++ b/crates/devup-mcp/src/server/screen_plan/status.rs @@ -362,7 +362,11 @@ pub(super) fn status_report( if links_fail { for item in &mut queue { item["done"] = json!(false); } } - let final_report = super::audit_plan::final_report(plan, &rows, &trailing) + &link_markdown; + // Who approved the URLs is part of what was delivered. + let approval = approval_line(&plan["userApproval"]); + let final_report = super::audit_plan::final_report(plan, &rows, &trailing) + + &link_markdown + + &format!("\n\nURL plan: {approval}.\n"); let blocking_urls = rows.iter().filter(|row| { row["implemented"] != "yes" || array(&row["widths"]).iter().any(|width| width["result"] != "pass") || row["generatedExport"]["issue"].is_string() @@ -382,6 +386,7 @@ pub(super) fn status_report( "blockingUrls": blocking_urls, "completion": {"done": done, "blockingUrls": blocking_urls, "reportAllowed": done}, "pendingDecisions": plan["pendingDecisions"], + "userApproval": {"line": approval, "record": plan["userApproval"]}, "finalReport": final_report, "linkTable": link_table, "mobileNavigation": plan["mobileNavigation"], @@ -599,6 +604,11 @@ pub(super) fn export_gate(request: &ExportRequest<'_>) -> Result return Ok(Gate::Open), }; + // The user approves the URLs before any page is written: a confirmed plan + // without the user's receipt is approved first, not built. + if gated && !has_user_approval(&plan) { + return Ok(Gate::Blocked(approval_gate(&plan, request.project_root))); + } if gated && let Some(erd) = super::project_plan::erd_required(request.project_root) { return Ok(Gate::Blocked(json!({ "status": "erd_required", diff --git a/crates/devup-mcp/src/server/screen_plan/user_approval.rs b/crates/devup-mcp/src/server/screen_plan/user_approval.rs new file mode 100644 index 00000000..08d76a79 --- /dev/null +++ b/crates/devup-mcp/src/server/screen_plan/user_approval.rs @@ -0,0 +1,181 @@ +// The user's approval, as a receipt the confirm call must carry. +// +// `confirm` used to take `acceptRecommended: true` on its own. An agent that +// skipped showing the URL table - or never asked at all - confirmed the plan +// itself, and pages were built on URLs nobody had seen. Prose in the skill +// did not stop that: a weaker model read "confirm" in nextAction and called +// it. So the confirm that a client sends through the tool now needs +// `userApproval`: how the user answered, the user's own words, and (for the +// screen plan) every URL of the table exactly as the user saw it. A receipt +// whose URLs differ from what confirm would build is refused with the table +// to show, so the user always approves the URLs that are built. The receipt +// is written into the plan and reported by status and the final report. + +/// The user's approval of a plan, sent with `confirm`. +#[derive(Debug, Clone, Default, Serialize, Deserialize, JsonSchema)] +#[serde(rename_all = "camelCase", deny_unknown_fields)] +#[schemars(extend("additionalProperties" = json!({"not": {}})))] +pub struct UserApprovalInput { + /// How the user approved. `question-tool`: the user chose an answer in the + /// runtime's question tool after the table was shown. `user-message`: the + /// user typed the approval in chat after the table was shown. `delegated`: + /// earlier in this session the user explicitly told you to decide without + /// asking - quote those words in userAnswer; the plan is then reported as + /// not reviewed by the user. + #[schemars(extend("enum" = ["question-tool", "user-message", "delegated"]))] + pub via: String, + /// The user's own answer, verbatim: the option label they chose or the + /// text they typed. Never your summary, never a recommendation they did + /// not pick. + pub user_answer: String, + /// devup_screen_plan: every URL in the table the user approved, exactly + /// as shown (e.g. ["/", "/about"]). It must equal the URLs this confirm + /// builds; when it does not, confirm refuses and returns the table to + /// show the user. + #[serde(default)] + pub approved_urls: Vec, +} + +const APPROVAL_VIA: [&str; 3] = ["question-tool", "user-message", "delegated"]; + +/// What a client must do when confirm has no valid approval. +pub(super) const APPROVAL_INSTRUCTION: &str = "Stop. Do not confirm on your own. Show the user the table (markdownTable: every screen and its URL) in the language they write in, ask with the runtime question tool (accept, or name the rows to change), wait for the answer, then call confirm again with userApproval: {via: \"question-tool\", userAnswer: \"\", approvedUrls: []}. Only when the user already told you in this session to decide without asking, use via \"delegated\" and quote their words in userAnswer."; + +fn approval_error(message: String, details: Value) -> DevupError { + let mut details = details; + details["approvalRequired"] = json!(true); + details["how"] = json!(APPROVAL_INSTRUCTION); + DevupError::with_details(ErrorCode::DevupInvalidInput, message, false, details) +} + +/// The receipt, checked for shape: a known `via` and the user's own words. +/// `required` is set for a confirm sent through the tool; `None` then fails. +pub(crate) fn approval_receipt<'a>( + approval: Option<&'a UserApprovalInput>, + required: bool, + what: &str, + table: &Value, + expected_urls: &[String], +) -> Result, DevupError> { + let Some(approval) = approval else { + if required { + return Err(approval_error( + format!("confirm needs userApproval: the user has to approve the {what} before it is confirmed, and this call carries no approval."), + json!({"missing": "userApproval", "markdownTable": table, "expectedUrls": expected_urls}), + )); + } + return Ok(None); + }; + if !APPROVAL_VIA.contains(&approval.via.as_str()) { + return Err(approval_error( + format!("userApproval.via must be one of {}, not {:?}.", APPROVAL_VIA.join(", "), approval.via), + json!({"markdownTable": table}), + )); + } + let answer = approval.user_answer.trim(); + if answer.is_empty() || (answer.contains('<') && answer.contains('>')) { + return Err(approval_error( + "userApproval.userAnswer must be the user's own answer, verbatim, not empty and not a placeholder.".into(), + json!({"markdownTable": table}), + )); + } + Ok(Some(approval)) +} + +/// The approved URLs against the URLs confirm builds; refused with both +/// lists and the table when they differ. +pub(crate) fn check_approved_urls( + approval: &UserApprovalInput, + built: &[String], + table: &Value, +) -> Result<(), DevupError> { + let normalize = |url: &str| { + let url = url.trim(); + if url.len() > 1 { url.trim_end_matches('/').to_owned() } else { url.to_owned() } + }; + let approved = approval.approved_urls.iter().map(|url| normalize(url)).collect::>(); + let expected = built.iter().map(|url| normalize(url)).collect::>(); + if approved == expected { + return Ok(()); + } + let missing = expected.difference(&approved).cloned().collect::>(); + let unexpected = approved.difference(&expected).cloned().collect::>(); + Err(approval_error( + format!( + "userApproval.approvedUrls does not match the URLs this confirm would build. Not approved: {}. Approved but not in the plan: {}. Nothing was confirmed: show this table to the user and ask again.", + if missing.is_empty() { "none".to_owned() } else { missing.join(", ") }, + if unexpected.is_empty() { "none".to_owned() } else { unexpected.join(", ") }, + ), + json!({"expectedUrls": expected, "notApproved": missing, "notInPlan": unexpected, "markdownTable": table}), + )) +} + +/// The receipt as the plan records it. +pub(crate) fn approval_record(approval: Option<&UserApprovalInput>) -> Value { + match approval { + Some(approval) => json!({ + "via": approval.via, + "userAnswer": approval.user_answer.trim(), + "approvedUrls": approval.approved_urls, + "reviewedByUser": approval.via != "delegated", + "recordedAt": now(), + }), + None => json!({"via": "none", "reviewedByUser": false, + "note": "Confirmed without a user approval receipt (not through the devup_screen_plan tool)."}), + } +} + +/// One line for status and the final report. +pub(crate) fn approval_line(record: &Value) -> String { + match record["via"].as_str() { + Some("question-tool") | Some("user-message") => format!( + "approved by the user ({}): \"{}\"", + text(&record["via"]), + text(&record["userAnswer"]) + ), + Some("delegated") => format!( + "NOT reviewed by the user: decided by the agent on the user's instruction \"{}\"", + text(&record["userAnswer"]) + ), + _ => "NOT reviewed by the user: no approval receipt was recorded".to_owned(), + } +} + +/// Every URL a plan builds, sorted and unique. +pub(crate) fn plan_urls(plan: &Value) -> Vec { + array(&plan["routes"]) + .iter() + .filter_map(|route| route["url"].as_str().map(str::to_owned)) + .collect::>() + .into_iter() + .collect() +} + +/// Whether the plan records the user's approval: their own answer, or their +/// explicit instruction to decide alone. No receipt, or one recorded without +/// the user, is not approval. +pub(crate) fn has_user_approval(plan: &Value) -> bool { + matches!( + plan["userApproval"]["via"].as_str(), + Some("question-tool" | "user-message" | "delegated") + ) +} + +/// The export gate's refusal for a confirmed plan the user never approved. +pub(super) fn approval_gate(plan: &Value, project_root: &str) -> Value { + let urls = plan_urls(plan); + json!({ + "status": "approval_required", + "written": false, + "approvalRequired": { + "reason": "The screen plan is confirmed, but no user approval of its URLs is recorded. No page is written until the user has seen the screens and URLs table and approved it.", + "planId": plan["planId"], + "markdownTable": route_markdown(array(&plan["routes"])), + "expectedUrls": urls, + }, + "nextAction": {"tool": "devup_screen_plan", + "arguments": {"action": "confirm", "projectRoot": project_root, "planId": plan["planId"], "answers": {}, + "userApproval": {"via": "question-tool", "userAnswer": "", "approvedUrls": urls}}, + "how": APPROVAL_INSTRUCTION, "mustAskUser": true}, + }) +} diff --git a/crates/devup-mcp/src/server/screen_plan_tests.rs b/crates/devup-mcp/src/server/screen_plan_tests.rs index a73f1053..fe783a6c 100644 --- a/crates/devup-mcp/src/server/screen_plan_tests.rs +++ b/crates/devup-mcp/src/server/screen_plan_tests.rs @@ -12,3 +12,5 @@ include!("screen_plan_tests/feedback_remaining.rs"); include!("screen_plan_tests/component_width.rs"); include!("screen_plan_tests/responsive_contract.rs"); include!("screen_plan_tests/responsive_groups.rs"); +include!("screen_plan_tests/user_approval.rs"); +include!("screen_plan_tests/motion_completeness.rs"); diff --git a/crates/devup-mcp/src/server/screen_plan_tests/a_custom_group_answer_can_name_one_route_per_frame.rs b/crates/devup-mcp/src/server/screen_plan_tests/a_custom_group_answer_can_name_one_route_per_frame.rs index 59a55b30..b6990cc5 100644 --- a/crates/devup-mcp/src/server/screen_plan_tests/a_custom_group_answer_can_name_one_route_per_frame.rs +++ b/crates/devup-mcp/src/server/screen_plan_tests/a_custom_group_answer_can_name_one_route_per_frame.rs @@ -395,7 +395,7 @@ fn site_files_are_asked_with_evidence_backed_candidates() { labels(find(&plan, "site:name"))[0], "prime-web (Recommended)" ); - assert_eq!(labels(find(&plan, "site:motion"))[0], "None (Recommended)"); + assert_eq!(labels(find(&plan, "site:motion"))[0], "Predict from the design (Recommended)"); assert!(!question_ids(&plan).contains(&"site:analytics".to_owned())); let defaults = array(&plan["defaults"]) .iter() diff --git a/crates/devup-mcp/src/server/screen_plan_tests/fixtures_and_cases.rs b/crates/devup-mcp/src/server/screen_plan_tests/fixtures_and_cases.rs index fe290a84..c7cf7ab8 100644 --- a/crates/devup-mcp/src/server/screen_plan_tests/fixtures_and_cases.rs +++ b/crates/devup-mcp/src/server/screen_plan_tests/fixtures_and_cases.rs @@ -610,6 +610,7 @@ fn confirmed_plan() -> Value { json!({ "kind": PLAN_KIND, "schemaVersion": SCHEMA_VERSION, "planId": "sp-test", "status": "confirmed", "source": {"nodeId": "10:0"}, + "userApproval": {"via": "question-tool", "userAnswer": "Accept (test user)", "approvedUrls": ["/", "/about"], "reviewedByUser": true}, "routes": [ {"url": "/", "screen": "10:1", "name": "Main", "nodeIds": ["10:1", "10:2"], "drawnWidths": [1440, 390], "implementation": "page", "file": "src/app/page.tsx"}, {"url": "/about", "screen": "10:3", "name": "About", "nodeIds": ["10:3"], "drawnWidths": [1440], "implementation": "page", "file": "src/app/about/page.tsx"}, diff --git a/crates/devup-mcp/src/server/screen_plan_tests/motion_completeness.rs b/crates/devup-mcp/src/server/screen_plan_tests/motion_completeness.rs new file mode 100644 index 00000000..13bdbfaf --- /dev/null +++ b/crates/devup-mcp/src/server/screen_plan_tests/motion_completeness.rs @@ -0,0 +1,28 @@ +// The motion the user chose, and eased field states, are completion items: +// status is not done while the sources lack them. + +#[test] +fn chosen_motion_and_field_state_easing_must_be_in_the_sources() { + let fixture = Fixture::new(); + fixture.write("src/components/Search.tsx", r#"export const S = () => ;"#); + let mut project = evidence(); + project.root = fixture.0.clone(); + project.sources = vec!["src/components/Search.tsx".into()]; + let item = |plan: &Value, name: &str| { + site_completeness(plan, &project).into_iter().find(|item| item["item"] == name).unwrap() + }; + let predicted = json!({"routes": [], "site": {"motion": {"state": "answered", "answer": {"motion": {"source": "predicted"}}}}}); + assert_eq!(item(&predicted, "motion")["state"], "missing"); + let fields = item(&predicted, "field-state-motion"); + assert_eq!(fields["state"], "missing"); + assert!(text(&fields["detail"]).contains("src/components/Search.tsx")); + let none = json!({"routes": [], "site": {"motion": {"state": "answered", "answer": {"motion": {"source": "none"}}}}}); + assert_eq!(item(&none, "motion")["state"], "not-applicable"); + let unanswered = json!({"routes": [], "site": {"motion": {"state": "asked", "question": "site:motion"}}}); + assert_eq!(item(&unanswered, "motion")["state"], "missing"); + + fixture.write("src/components/Search.tsx", r#"const fadeUp = keyframes({ from: { opacity: 0 }, to: { opacity: 1 } }); +export const S = () => ;"#); + assert_eq!(item(&predicted, "motion")["state"], "filled"); + assert_eq!(item(&predicted, "field-state-motion")["state"], "filled"); +} diff --git a/crates/devup-mcp/src/server/screen_plan_tests/user_approval.rs b/crates/devup-mcp/src/server/screen_plan_tests/user_approval.rs new file mode 100644 index 00000000..452660ec --- /dev/null +++ b/crates/devup-mcp/src/server/screen_plan_tests/user_approval.rs @@ -0,0 +1,148 @@ +// The tool confirms a screen plan only with the user's receipt, and only for +// the URLs the user saw. + +fn receipt(via: &str, answer: &str, urls: &[&str]) -> UserApprovalInput { + UserApprovalInput { + via: via.into(), + user_answer: answer.into(), + approved_urls: urls.iter().map(|url| (*url).to_owned()).collect(), + } +} + +#[tokio::test] +async fn the_tool_confirm_refuses_without_a_user_receipt_and_writes_nothing() { + let fixture = Fixture::new(); + let mut input = drafted(&fixture, &main_widths()).await; + input.accept_recommended = true; + let before = std::fs::read_to_string(fixture.0.join(PLAN_FILE)).unwrap(); + let error = confirm_approved(&input, &fixture.policy()).unwrap_err(); + assert!(error.message.contains("needs userApproval"), "{}", error.message); + assert_eq!(error.details["approvalRequired"], true); + assert!(error.details["markdownTable"].as_str().is_some_and(|table| table.contains('/'))); + assert!(text(&error.details["how"]).contains("question tool")); + assert_eq!(std::fs::read_to_string(fixture.0.join(PLAN_FILE)).unwrap(), before); + // A placeholder or an unknown channel is not an answer either. + for bad in [receipt("question-tool", "", &["/"]), receipt("assistant", "Accept", &["/"]), receipt("question-tool", " ", &["/"])] { + input.user_approval = Some(bad); + assert_eq!(confirm_approved(&input, &fixture.policy()).unwrap_err().details["approvalRequired"], true); + } + assert_eq!(read_plan(fixture.root()).unwrap().unwrap()["status"], "draft"); +} + +#[tokio::test] +async fn approved_urls_must_be_exactly_the_urls_confirm_builds() { + let fixture = Fixture::new(); + let mut input = drafted(&fixture, &main_widths()).await; + input.accept_recommended = true; + input.user_approval = Some(receipt("question-tool", "Accept the table (Recommended)", &["/", "/pricing"])); + let error = confirm_approved(&input, &fixture.policy()).unwrap_err(); + assert!(error.message.contains("does not match"), "{}", error.message); + assert_eq!(error.details["notInPlan"], json!(["/pricing"])); + assert_eq!(error.details["expectedUrls"], json!(["/"])); + assert_eq!(read_plan(fixture.root()).unwrap().unwrap()["status"], "draft"); + + // The URLs the user saw: confirmed, and the receipt is recorded. + input.user_approval = Some(receipt("question-tool", "Accept the table (Recommended)", &["/"])); + let confirmed = confirm_approved(&input, &fixture.policy()).unwrap(); + assert_eq!(confirmed["status"], "confirmed"); + assert_eq!(confirmed["userApproval"]["reviewedByUser"], true); + assert!(text(&confirmed["approval"]).starts_with("approved by the user (question-tool)")); + let saved = read_plan(fixture.root()).unwrap().unwrap(); + assert_eq!(saved["userApproval"]["userAnswer"], "Accept the table (Recommended)"); + assert_eq!(saved["userApproval"]["approvedUrls"], json!(["/"])); +} + +#[tokio::test] +async fn a_delegated_confirm_is_reported_as_not_reviewed_by_the_user() { + let fixture = Fixture::new(); + let mut input = drafted(&fixture, &main_widths()).await; + input.accept_recommended = true; + input.user_approval = Some(receipt("delegated", "알아서 진행해 줘", &["/"])); + confirm_approved(&input, &fixture.policy()).unwrap(); + let saved = read_plan(fixture.root()).unwrap().unwrap(); + assert_eq!(saved["userApproval"]["reviewedByUser"], false); + assert!(approval_line(&saved["userApproval"]).starts_with("NOT reviewed by the user")); + // A plan confirmed outside the tool carries no receipt, and says so. + assert!(approval_line(&approval_record(None)).contains("no approval receipt")); +} + +#[tokio::test] +async fn the_draft_tells_the_agent_to_wait_for_the_user_before_confirm() { + let fixture = Fixture::new(); + let drafted = draft(&input(&fixture, "draft"), &main_widths(), json!({}), &fixture.policy()) + .await + .unwrap(); + let next = &drafted["nextAction"]; + assert_eq!(next["mustAskUser"], true); + assert_eq!(next["arguments"]["userApproval"]["via"], "question-tool"); + assert!(next["arguments"]["userApproval"]["approvedUrls"].is_array()); +} + +#[test] +fn a_confirmed_plan_without_the_users_approval_writes_no_page_until_it_is_approved() { + let fixture = Fixture::new(); + let mut plan = confirmed_plan(); + plan["userApproval"] = Value::Null; + write_plan_file(&fixture, &plan); + let request = ExportRequest { + project_root: fixture.root(), + url: None, + frames: vec!["10:2".into()], + all_screens: false, + scaffold_write: true, + scaffold_route: Some(""), + output_paths: Vec::new(), + }; + let Gate::Blocked(response) = export_gate(&request).unwrap() else { + panic!("a plan the user never approved must not write a page"); + }; + assert_eq!(response["status"], "approval_required"); + assert_eq!(response["written"], false); + assert_eq!(response["approvalRequired"]["expectedUrls"], json!(["/", "/about"])); + assert_eq!(response["nextAction"]["mustAskUser"], true); + // A preview that writes nothing is never blocked. + let preview = ExportRequest { scaffold_write: false, scaffold_route: None, ..request }; + assert!(matches!(export_gate(&preview).unwrap(), Gate::Open)); + + // The user approves the existing plan; the receipt is recorded and pages open. + let mut approve = input(&fixture, "confirm"); + approve.plan_id = Some("sp-test".into()); + approve.user_approval = Some(receipt("question-tool", "승인", &["/about"])); + assert!(confirm_approved(&approve, &fixture.policy()).unwrap_err().message.contains("does not match")); + approve.user_approval = Some(receipt("question-tool", "승인", &["/", "/about"])); + let approved = confirm_approved(&approve, &fixture.policy()).unwrap(); + assert_eq!(approved["alreadyConfirmed"], true); + assert_eq!(approved["userApproval"]["userAnswer"], "승인"); + let request = ExportRequest { scaffold_write: true, scaffold_route: Some(""), ..preview }; + assert!(matches!(export_gate(&request).unwrap(), Gate::Route(_))); +} + +#[tokio::test] +async fn the_procedure_names_one_step_and_waits_for_the_user_at_the_question() { + let fixture = Fixture::new(); + let none = procedure(fixture.root()); + assert_eq!(none["step"], "1/5 draft"); + assert_eq!(none["next"]["arguments"]["action"], "draft"); + assert_eq!(none["waitForUser"], false); + + let mut input = drafted(&fixture, &main_widths()).await; + let asking = procedure(fixture.root()); + assert_eq!(asking["step"], "2/5 ask-user"); + assert_eq!(asking["waitForUser"], true); + assert_eq!(asking["next"]["tool"], "question-tool"); + assert!(array(&asking["forbiddenNow"]).iter().any(|rule| text(rule).contains("same turn"))); + + input.accept_recommended = true; + input.user_approval = Some(receipt("question-tool", "Accept", &["/"])); + confirm_approved(&input, &fixture.policy()).unwrap(); + let building = procedure(fixture.root()); + assert_eq!(building["step"], "4/5 implement"); + assert_eq!(building["waitForUser"], false); + + let mut plan = read_plan(fixture.root()).unwrap().unwrap(); + plan["userApproval"] = Value::Null; + write_plan_file(&fixture, &plan); + let approve = procedure(fixture.root()); + assert_eq!(approve["step"], "3/5 confirm-with-approval"); + assert_eq!(approve["next"]["arguments"]["userApproval"]["approvedUrls"], json!(["/"])); +} diff --git a/crates/devup-mcp/src/server/skills/devfive-frontend/references/delivery-quality.md b/crates/devup-mcp/src/server/skills/devfive-frontend/references/delivery-quality.md index fc716a51..9a1f803f 100644 --- a/crates/devup-mcp/src/server/skills/devfive-frontend/references/delivery-quality.md +++ b/crates/devup-mcp/src/server/skills/devfive-frontend/references/delivery-quality.md @@ -43,6 +43,17 @@ binary ships this document in its usage resource and skill installation. pending and captured state styles. Capture the whole COMPONENT_SET before projecting states; a static icon or screenshot is not a working select. Verify select open/choose/close, arrows, Enter/Escape, ARIA and focus restoration. +- A field's focus marks the box the user sees. When the input/textarea/select + is itself the bordered box, its `_focusVisible` draws the ring. When it sits + in a container with an icon, prefix or button, the container takes + `_focusWithin` (border colour plus a ring) and the field drops its own + outline only then, so no second inner outline appears. Keyboard focus stays + on the field: never `tabIndex` on the container. Keep the border width equal + at rest and focused (draw extra width with `boxShadow`/`outline`) so the + field and its neighbours never move. Labels, aria-invalid error styles and + disabled stay. The runtime `input-focus` check focuses each field by click + and by Tab and fails an inner outline, a ring that misses the container, + focus landing on the container and any movement. - A colour variant must use its own captured palette for background, text, border, icon, hover, press, focus and disabled states. Do not inherit primary blue into a secondary variant or infer absent tokens by suffix number. @@ -91,6 +102,19 @@ Do this without being asked, on every page built or changed; the installed `devfive-frontend/references/animation.md` has the full rules. A design the user stated (no animation, a given timing, a replaying effect) always wins. +- Predict motion from the design: the screen plan's `site:motion` question + recommends "Predict from the design" - read each frame's prototype + reactions and layout (hero, card grids, lists, counters, logo rows, tabs) + and implement the behaviorPlan motion candidates they imply. Status lists a + `motion` completion item that stays missing until keyframes are in the + sources, and a `field-state-motion` item that stays missing while a file + with a field has no transition. +- Field states ease: the box that changes on focus, invalid or disabled has + `transition="160ms ease-out"` and a `transitionProperty` naming only the + changed paint (`border-color,box-shadow,background`), unless the prototype + gives its own. Generated field components carry it; `devup_ui_validate` + reports `input-state-transition` and the runtime `input-focus` check fails a + focus state that snaps. - Survey sections and repeated content, then propose motion by role and implement it: one restrained entrance per below-fold content group, a short stagger for card groups only where it helps, transitions for state changes. @@ -339,6 +363,7 @@ before claiming completion. The installed copy of this document is | `preserve-instance-sizing-and-paint` | Captured instance overrides, sizes and paints are kept | | `paired-e2e-and-native-figma-visuals` | Functional E2E and native Figma visual comparisons are run together | | `semantic-native-controls-and-complete-pseudo-states` | Native elements with accessible names and every pseudo-state painted | +| `field-focus-on-visual-container` | A field's focus ring surrounds the box the user sees - the field itself, or its container via `_focusWithin` with the inner outline removed - focus stays on the field and nothing moves; proven by the runtime `input-focus` check by click and by Tab | | `variant-palette-consistency` | Every variant uses the captured palette, including selected and disabled | | `single-source-url-content-selection` | The URL is the single source of which content is selected (`aria-current`) | | `viewport-footer-shell` | A short page keeps its footer at the viewport bottom | @@ -348,6 +373,7 @@ before claiming completion. The installed copy of this document is | `one-shot-reveals-without-ssr-flash` | A reveal completes when its content is visible and never replays, re-hides or flashes on hydration, reload, back/forward or client navigation | | `canonical-semantic-assets` | One asset registry, semantic file names, no duplicates or hash names | | `landing-detail-api-db-single-source` | Landing and detail values come from the same API and DB records | +| `user-approved-plan-before-implementation` | The screen and project plans are confirmed only with `userApproval`: the user's own answer and, for screens, every approved URL; an agent never confirms on the user's behalf | | `runtime-evidence-before-completion` | Completion is claimed from a runtime audit, not from the export or a screenshot | | `offer-devup-retro-in-final-report` | The final completion report ends by offering `/devup-retro` once, in the user's language; it is never run unasked | diff --git a/crates/devup-mcp/src/server/skills/devup-implementation-plan/SKILL.md b/crates/devup-mcp/src/server/skills/devup-implementation-plan/SKILL.md index 834c2c35..1259b45c 100644 --- a/crates/devup-mcp/src/server/skills/devup-implementation-plan/SKILL.md +++ b/crates/devup-mcp/src/server/skills/devup-implementation-plan/SKILL.md @@ -178,6 +178,14 @@ So the order is fixed, and none of it is optional. the rows to change. Then confirm with `acceptRecommended: true` and only the changed rows in `answers`. Use `questions` / `questionRounds` only to review the rows the user asked to change. +- **The user approves the URLs, not you.** Confirm (screen plan and project + plan) requires `userApproval`: `via` (`question-tool`, `user-message`, or + `delegated` only when the user told you to decide alone), `userAnswer` (the + user's own words, verbatim) and, for the screen plan, `approvedUrls` - every + URL of the table exactly as the user saw it. A missing receipt or a URL list + that differs from what confirm builds confirms nothing and returns the table + to show. Ask, wait for the answer, then confirm; never in the same turn as + the question, never with an answer the user did not give. - **Ask in the user's language.** Translate the table, the question and the option labels into the language the user first wrote to you in, for display only. Answers sent to confirm stay keyed by question id and use the original @@ -273,7 +281,7 @@ and the final report repeats them: | Card that links | 2-4px lift + shadow token; inner image zooms 1.02-1.04 (overflow hidden) | lift removed | ring on the card | - | | Nav item / tab | colour | - | ring | current route: `aria-current="page"` and the Figma active style (e.g. the underline the header draws) | | Icon button | background circle or tint | press | ring | `aria-label` required | -| Input / select / textarea | border | - | border + ring | invalid state with its message; disabled | +| Input / select / textarea | border | - | border + ring on the box the user sees: the field itself (`_focusVisible`), or - when the field sits in a container with an icon, prefix or button - the container with `_focusWithin`, and the field's own outline removed (`outline: "none"`) only then; focus stays on the field (no `tabIndex` on the container); same border width at rest and focused (ring by `boxShadow`/`outline`) | invalid state with its message; disabled; verified by the runtime `input-focus` check (click and Tab) | | Accordion / disclosure / dropdown | - | - | ring | `aria-expanded`; the chevron rotates 180deg | | Mobile menu / drawer | - | - | - | an open/close transition, when the design has one | diff --git a/crates/devup-mcp/src/server/skills/devup/SKILL.md b/crates/devup-mcp/src/server/skills/devup/SKILL.md index 248bb937..e2745116 100644 --- a/crates/devup-mcp/src/server/skills/devup/SKILL.md +++ b/crates/devup-mcp/src/server/skills/devup/SKILL.md @@ -155,6 +155,14 @@ This is the devup-implementation-plan workflow, started explicitly. Follow it in this order and do not skip a step. The devup-implementation-plan skill has the full rules; these are the steps. +Every `devup_screen_plan` reply, and every export the plan gate refuses, +carries a `procedure` block computed from the plan on disk: `step` (where the +work is, of 5), `next` (the one call to make now), `waitForUser` and +`forbiddenNow`. Do exactly `next`. When `waitForUser` is true, end the turn +after asking and do nothing else until the user answers. Never do anything in +`forbiddenNow`. If what you planned differs from `procedure.next`, the +procedure wins. + 1. **Connection.** `devup_figma_auth` with `action: "status"`. With a Bridge plugin attached nothing needs a login; do not ask for one. For disconnected, out-of-view or mismatched-host states, load `figma-bridge-setup` and perform @@ -180,10 +188,22 @@ the full rules; these are the steps. (every open decision with its recommended answer) in the language the user first wrote in, and ask ONE question with your question tool: accept, or name the rows to change. Collect known decisions in one question call. -6. **Confirm.** `devup_screen_plan` with `action: "confirm"`, the `planId`, - `acceptRecommended: true` and only the changed rows in `answers`. If - confirm reports two screens on one URL, retry with its - `details.retryAnswers` merged into your answers. + **This is a hard stop.** Do not call confirm in the same turn as the + question. Wait for the user's answer. Never pick an answer for the user, + never treat silence or your own recommendation as approval. +6. **Confirm with the user's receipt.** `devup_screen_plan` with + `action: "confirm"`, the `planId`, `acceptRecommended: true`, only the + changed rows in `answers`, and `userApproval`: + `{ via: "question-tool", userAnswer: "", + approvedUrls: [] }`. + Confirm refuses without it, and refuses when `approvedUrls` differs from + the URLs it would build - then it returns `details.markdownTable`: show + that table, ask again, and confirm with the URLs the user approved. Use + `via: "delegated"` only when the user said in this session to decide + without asking, quoting those words; the final report then says the URLs + were not reviewed. If confirm reports two screens on one URL, retry with + its `details.retryAnswers` merged into your answers - and the new URL goes + back to the user the same way. 7. **Latest first, before implementing, without asking.** a. At the repository root, run `bunx @dependency-check-updates/cli -d -u --rm`. It updates every manifest in the repository to the latest diff --git a/crates/devup-mcp/src/server/skills/manifest.json b/crates/devup-mcp/src/server/skills/manifest.json index 3a5213b2..7554892e 100644 --- a/crates/devup-mcp/src/server/skills/manifest.json +++ b/crates/devup-mcp/src/server/skills/manifest.json @@ -76,8 +76,8 @@ }, { "path": "references/delivery-quality.md", - "bytes": 26870, - "sha256": "ee1dd0b19c1a4bff42709af86e19e15496ccc2f22fa99b60ffa9490db2574362" + "bytes": 29079, + "sha256": "51c4c81fb36a969f708696ab9becf15de5cc458ac50975ea496a5aa1546b80e8" }, { "path": "references/animation.md", @@ -99,8 +99,8 @@ "documents": [ { "path": "SKILL.md", - "bytes": 45897, - "sha256": "ddbfcb3588c4c61c1e867abae24d062f4a88ed838fd47e7917d98edcfd498e2c" + "bytes": 46928, + "sha256": "eeef6ee8744deccda24908be0d94b0016e4d4364e172903adf1745422b6ca511" } ], "sourceUrl": "https://github.com/dev-five-git/devup-mcp/blob/HEAD/crates/devup-mcp/src/server/skills/devup-implementation-plan", @@ -117,8 +117,8 @@ "documents": [ { "path": "SKILL.md", - "bytes": 25151, - "sha256": "52c926b03727b9b05bd09a2a8875f1dbbb71c253ef60cb4054434f4d600b5fdc" + "bytes": 26465, + "sha256": "ada6d39398c2d19f22e7b5eb036da6d37940c76ca9c9f550948d9c30125a8aee" } ], "sourceUrl": "https://github.com/dev-five-git/devup-mcp/blob/HEAD/crates/devup-mcp/src/server/skills/devup", diff --git a/crates/devup-mcp/src/server/text/devup-screen-plan.md b/crates/devup-mcp/src/server/text/devup-screen-plan.md index b53e83cc..595fbbc7 100644 --- a/crates/devup-mcp/src/server/text/devup-screen-plan.md +++ b/crates/devup-mcp/src/server/text/devup-screen-plan.md @@ -1,4 +1,4 @@ -Plan which Figma screen becomes which URL before any page is written, confirm it up front, then report it URL by width (action: draft | confirm | status | assume | auditPlan | defer). draft reads the linked Section or frame (or the Bridge selection, or `screens` from an earlier devup_figma_explore) and proposes a route only from evidence: an existing page built from the frame, a nav link label bound to an existing href, a navigationTargets entry, or .devup/screen-plan.config.json (modalKeywords, routeAliases, widthGrouping). A route inferred from a layer name is routeEvidence name-match and is put to the user; with no evidence it is UNVERIFIED and asked. Only frames under the link are candidates, never the rest of the file; frames in trash, archive, draft, copy or hidden areas are excluded and listed (excludeKeywords/includeKeywords in the config). Frames sharing a name in one section are asked about per group; the same name in different sections, a screen split across sections, a frame outside any section and duplicates each get an explicit question with a "the Figma structure is wrong" option whose frames are reported, never built. It writes .devup/screen-plan.json and .md and returns `proposal` (and `proposalMarkdown`): every open decision as one table row with the recommended answer and the alternatives. Do not interview the user one decision at a time: show the table and ask ONE question with your question tool - accept it, or name the rows to change - in the language the user first wrote in (translate for display only; answers stay keyed by question id with the original labels or custom text). confirm with `acceptRecommended: true` fills every unanswered applicable question with its recommended option, so send only the changed rows in `answers`; without it, confirm refuses and names the missing ids. `questions`/`questionRounds` remain for reviewing individual rows. A confirm that puts two screens on one URL writes nothing and returns `details.conflict` (both screens and the questions placing them) and `details.retryAnswers` with a distinct suggested URL to retry with. After confirm, never ask the user: take the recommended or most conservative option and record it with assume. status compares the confirmed plan with the routes on disk and an optional devup_runtime_audit plan/report: per URL, implemented yes/no and per-width pass, fail with reason, or not-run - a width without an audit item is never pass - plus every assumption. draft also lists the stack defaults - Next App Router on vinext, a new project from dev-five-git/service-template, and the dependency update `bunx @dependency-check-updates/cli -d -u --rm` before implementing - and asks once whether a next-CLI app migrates to vinext; with several workspace apps it picks the app by meaning (the public app unless the frames clearly say admin, per screen in a mixed file) and asks only when no app looks like the public site; then the site defaults it fills in without asking (an interaction recipe per element role, restrained; one shared transition; reduced motion; view transitions always on (React's ViewTransition; skipped only when the project config opts out), verified by a runtime-audit view-transition check; metadata, lang, alt text, not-found) and asks only what needs a file or a fact: favicon and share image (from existing files or Figma logo/og nodes, never generated), site name and URL when the project does not say, analytics when the project shows it, and scroll reveal; `extraQuestions` adds the agent's own questions to the same rounds so nothing is asked after confirm; .devup/screen-plan.config.json may pre-answer them (favicon, ogImage, siteName, siteUrl, motion). status adds site completeness: favicon, metadata per URL, share image, hover/focus coverage and reduced motion as filled, asked-and-answered or missing. auditPlan writes one devup_runtime_audit plan per confirmed URL from the export artifacts: page health, overflow and clipped text at every width; the page, each section and each asset against its Figma render (devup_visual_compare calls returned by status) and the size and position of each section, card and asset at the drawn widths, after recording CSS viewport, zoom, device scale, fonts and scroll; half a CSS pixel of rounding applies only to calibrated drawn-width geometry; merged pages mapped through structured export sidecars and explicit semantic runtime targets, with missing mappings left not-run; dynamic routes at concrete URLs from generateStaticParams, the site's links or the detail frame; hover and focus; a view-transition check placed last; clean TSX in development and production without source-node attributes or strip plugins; missing referencePngs and renders are listed with the export call that makes them. status then classifies every failure (runtime-error, overflow, code-drift or converter-bug for geometry - compared with the export record each export writes to .devup/exports, a converter bug written as a repro to .devup/converter-bugs and never retried - layout-position, layout-size, asset-mismatch, section-visual, page-visual, interaction, view-transition) with its Figma node, code file and fix hint, folds visualResults in, marks a finding unresolved with its evidence after maxFixAttempts (config, default 3), and returns a workQueue per URL and the finalReport. defer records a decision met after confirm as pending with its recommended option so the work never waits; assume takes it at the end, and a destructive one stays unbuilt. Done = zero fails, or listed as unresolved with evidence. The devup-implementation-plan skill (devup_skills) carries the whole workflow; /devup starts it. +Plan which Figma screen becomes which URL before any page is written, confirm it up front, then report it URL by width (action: draft | confirm | status | assume | auditPlan | defer). draft reads the linked Section or frame (or the Bridge selection, or `screens` from an earlier devup_figma_explore) and proposes a route only from evidence: an existing page built from the frame, a nav link label bound to an existing href, a navigationTargets entry, or .devup/screen-plan.config.json (modalKeywords, routeAliases, widthGrouping). A route inferred from a layer name is routeEvidence name-match and is put to the user; with no evidence it is UNVERIFIED and asked. Only frames under the link are candidates, never the rest of the file; frames in trash, archive, draft, copy or hidden areas are excluded and listed (excludeKeywords/includeKeywords in the config). Frames sharing a name in one section are asked about per group; the same name in different sections, a screen split across sections, a frame outside any section and duplicates each get an explicit question with a "the Figma structure is wrong" option whose frames are reported, never built. It writes .devup/screen-plan.json and .md and returns `proposal` (and `proposalMarkdown`): every open decision as one table row with the recommended answer and the alternatives. Do not interview the user one decision at a time: show the table and ask ONE question with your question tool - accept it, or name the rows to change - in the language the user first wrote in (translate for display only; answers stay keyed by question id with the original labels or custom text). confirm with `acceptRecommended: true` fills every unanswered applicable question with its recommended option, so send only the changed rows in `answers`; without it, confirm refuses and names the missing ids. Every reply carries `procedure` (step of 5, the one `next` call, `waitForUser`, `forbiddenNow`): follow `next` exactly and stop when `waitForUser` is true. confirm also requires `userApproval` - `via` (question-tool, user-message, or delegated when the user told you to decide alone), `userAnswer` (the user's own words, verbatim) and `approvedUrls` (every URL of the table exactly as the user saw it); a missing receipt, or URLs that differ from what confirm builds, confirms nothing and returns the table to show. Never confirm on the user's behalf. The receipt is recorded and printed in status and the finalReport. `questions`/`questionRounds` remain for reviewing individual rows. A confirm that puts two screens on one URL writes nothing and returns `details.conflict` (both screens and the questions placing them) and `details.retryAnswers` with a distinct suggested URL to retry with. After confirm, never ask the user: take the recommended or most conservative option and record it with assume. status compares the confirmed plan with the routes on disk and an optional devup_runtime_audit plan/report: per URL, implemented yes/no and per-width pass, fail with reason, or not-run - a width without an audit item is never pass - plus every assumption. draft also lists the stack defaults - Next App Router on vinext, a new project from dev-five-git/service-template, and the dependency update `bunx @dependency-check-updates/cli -d -u --rm` before implementing - and asks once whether a next-CLI app migrates to vinext; with several workspace apps it picks the app by meaning (the public app unless the frames clearly say admin, per screen in a mixed file) and asks only when no app looks like the public site; then the site defaults it fills in without asking (an interaction recipe per element role, restrained; one shared transition; reduced motion; view transitions always on (React's ViewTransition; skipped only when the project config opts out), verified by a runtime-audit view-transition check; metadata, lang, alt text, not-found) and asks only what needs a file or a fact: favicon and share image (from existing files or Figma logo/og nodes, never generated), site name and URL when the project does not say, analytics when the project shows it, and scroll reveal; `extraQuestions` adds the agent's own questions to the same rounds so nothing is asked after confirm; .devup/screen-plan.config.json may pre-answer them (favicon, ogImage, siteName, siteUrl, motion). status adds site completeness: favicon, metadata per URL, share image, hover/focus coverage and reduced motion as filled, asked-and-answered or missing. auditPlan writes one devup_runtime_audit plan per confirmed URL from the export artifacts: page health, overflow and clipped text at every width; the page, each section and each asset against its Figma render (devup_visual_compare calls returned by status) and the size and position of each section, card and asset at the drawn widths, after recording CSS viewport, zoom, device scale, fonts and scroll; half a CSS pixel of rounding applies only to calibrated drawn-width geometry; merged pages mapped through structured export sidecars and explicit semantic runtime targets, with missing mappings left not-run; dynamic routes at concrete URLs from generateStaticParams, the site's links or the detail frame; hover and focus; a view-transition check placed last; clean TSX in development and production without source-node attributes or strip plugins; missing referencePngs and renders are listed with the export call that makes them. status then classifies every failure (runtime-error, overflow, code-drift or converter-bug for geometry - compared with the export record each export writes to .devup/exports, a converter bug written as a repro to .devup/converter-bugs and never retried - layout-position, layout-size, asset-mismatch, section-visual, page-visual, interaction, view-transition) with its Figma node, code file and fix hint, folds visualResults in, marks a finding unresolved with its evidence after maxFixAttempts (config, default 3), and returns a workQueue per URL and the finalReport. defer records a decision met after confirm as pending with its recommended option so the work never waits; assume takes it at the end, and a destructive one stays unbuilt. Done = zero fails, or listed as unresolved with evidence. The devup-implementation-plan skill (devup_skills) carries the whole workflow; /devup starts it. Audit build policy: do not change app build config for auditing or register a strip plugin, including any legacy auditPlan suggestion to do so. DevupUI strips audit attributes by default; DEVUP_AUDIT=1 keeps them. Figma node attributes are off by default; figmaNodeIds true is opt-in only where supported by the export schema. Missing anchors leave coverage unverified. Before reporting done, run the runtime audit for every confirmed URL at every required width (default 1440/1100/858/390), read the report, fix failures and rerun until every cell passes. Missing cells are not-run and block completion. At drawn widths require rendered completeness and page, section and asset visual comparisons. Check sequence-order and section-layout at every width. For every multi-width URL, status and its work queue provide the responsiveTsx export call with all sibling frameIds in width order. responsiveTsx is the deliverable: never start from single-width tsx, and never change generated layout values. Only wire links/events/data. generatedExport reports recorded responsive coverage, flags single-width exports used for multi-width URLs, and includes heuristic generatedDrift with confidence and source file paths; absent mappings remain unverified. Ask known site, structure and URL decisions up front in one runtime question call. Keep table approval separate from individual decisions. New user-owned decisions after confirm still use the question tool: headers up to 30 characters, two to four options, recommended first and labelled "(Recommended)". Never ask for numbers in chat. Record the chosen answer, decision id, affected model/endpoint/file and rationale through existing defer/assume actions; do not invent an ask action. Re-confirm source model changes before dependent work. Without a question tool, report the limitation and leave the decision unresolved. Workers escalate to the supervisor; continue independent work while answers are pending. diff --git a/crates/devup-mcp/src/server/text/guide-INSTRUCTIONS-49bd7ec6.md b/crates/devup-mcp/src/server/text/guide-INSTRUCTIONS-49bd7ec6.md index 4d3e9873..6b3b27d2 100644 --- a/crates/devup-mcp/src/server/text/guide-INSTRUCTIONS-49bd7ec6.md +++ b/crates/devup-mcp/src/server/text/guide-INSTRUCTIONS-49bd7ec6.md @@ -1,8 +1,9 @@ Build identity: use server.commit/buildId, not version; reconnect if stale. server.updateAvailable unknown never blocks. -1. devup-mcp is the primary Figma-to-code source. Connection: bridge -> direct; call devup_figma_auth status first, one Bridge needs no login or url. -2a. Another running project is not a desktop lock; use OS split-screen. +0. GATES, never skipped: show the screen/URL table, ask with the question tool, wait; confirm only with userApproval (user's verbatim answer + every approved URL). No page write before confirm. +1. devup-mcp is the primary Figma-to-code source. Connection: bridge -> direct; devup_figma_auth status first; one Bridge needs no login. +2a. Another running project is no desktop lock; use split-screen. 2b. /devup: devup_project_plan intake; read Jira/spec/Figma and devup_project_context, ask all rounds up front, draft ERD, confirm, split. devup_screen_plan handles Figma. Load devup_skills and devup-implementation-plan; after confirm defer, continue, assume at the end. -3. For Figma implementation use devup_figma_export tsx, the deliverable. TSX links/events need wiring: resolve interactiveCandidates, devup_ui_validate, report wired/tested counts. deliverable.isFinal describes export readiness. Audit every width and exact layout. Backend completion needs devup_stack_diff zero drift. -4. Request only outputs you read. Before your second call read devup://guide/usage for sizing, verification, batching, assets and delivery. -5. After confirm, extract COMPONENT_SET/COMPONENT first with componentTsx into components/common|layout; record mainComponent id -> file in .devup/component-map.json. Assemble generated sections (frame-root children); audit via file mappings, not DOM Figma attributes. Never hand-write TSX from Figma nodes. Report unsupported exports. -6. Before completion apply developmentContract, behaviorPlan, default keyframes motion. +3. Implement Figma with devup_figma_export tsx. Wire links/events (interactiveCandidates), devup_ui_validate, report wired/tested counts. deliverable.isFinal is export readiness. Audit every width, exact layout. Backend: devup_stack_diff zero drift. +4. Request only outputs you read. Before your second call read devup://guide/usage. +5. After confirm, extract COMPONENT_SET/COMPONENT first (componentTsx) into components/common|layout; map mainComponent id -> file in .devup/component-map.json. Assemble generated sections; audit via file mappings. Never hand-write TSX from Figma nodes. +6. Before completion apply developmentContract, behaviorPlan, keyframes motion; done only from status reportAllowed. diff --git a/crates/devup-mcp/tests/export_project_theme.rs b/crates/devup-mcp/tests/export_project_theme.rs index a18d4428..14f044cb 100644 --- a/crates/devup-mcp/tests/export_project_theme.rs +++ b/crates/devup-mcp/tests/export_project_theme.rs @@ -202,7 +202,7 @@ async fn export_context_summarizes_a_large_project_under_fifteen_kib() -> anyhow let contract = &result["developmentContract"]; assert_eq!( contract["requirements"].as_array().map(Vec::len), - Some(21), + Some(23), "{contract}" ); assert_eq!( diff --git a/crates/devup-mcp/tests/screen_plan_tool.rs b/crates/devup-mcp/tests/screen_plan_tool.rs index 84d4e283..6f441ba8 100644 --- a/crates/devup-mcp/tests/screen_plan_tool.rs +++ b/crates/devup-mcp/tests/screen_plan_tool.rs @@ -44,6 +44,32 @@ impl FigmaUpstream for NoFigma { } } +/// The first value under `key` anywhere in `value`. +fn find<'v>(value: &'v Value, key: &str) -> Option<&'v Value> { + match value { + Value::Object(map) => map + .get(key) + .or_else(|| map.values().find_map(|child| find(child, key))), + Value::Array(items) => items.iter().find_map(|child| find(child, key)), + _ => None, + } +} + +/// A confirm through the tool: refused without the user's receipt, then sent +/// again with the URLs the refusal says the user has to approve. +async fn confirm_as_user(root: &Path, mut arguments: Value) -> anyhow::Result<(bool, Value)> { + let (failed, refused) = call(root, "devup_screen_plan", arguments.clone()).await?; + if !failed { + return Ok((failed, refused)); + } + let Some(urls) = find(&refused, "expectedUrls").cloned() else { + return Ok((failed, refused)); + }; + assert!(refused.to_string().contains("userApproval"), "{refused}"); + arguments["userApproval"] = json!({"via": "question-tool", "userAnswer": "Accept the table (Recommended)", "approvedUrls": urls}); + call(root, "devup_screen_plan", arguments).await +} + async fn call(root: &Path, tool: &'static str, arguments: Value) -> anyhow::Result<(bool, Value)> { let server = DevupServer::with_output_roots( Services::new(Arc::new(Auth), Arc::new(NoFigma)), @@ -110,7 +136,7 @@ async fn recommended_contact_routes_confirm_through_the_tool_surface() -> anyhow ) .await?; assert!(!failed, "{draft}"); - let (failed, confirmed) = call(&root, "devup_screen_plan", json!({ + let (failed, confirmed) = confirm_as_user(&root, json!({ "action":"confirm", "projectRoot":root, "planId":draft["planId"], "acceptRecommended":true })).await?; assert!(!failed, "{confirmed}"); @@ -204,9 +230,8 @@ async fn draft_asks_everything_at_once_and_confirm_needs_every_answer() -> anyho assert!(!failed, "{gated}"); assert_eq!(gated["status"], "plan_required", "{gated}"); - let (failed, confirmed) = call( + let (failed, confirmed) = confirm_as_user( &root, - "devup_screen_plan", json!({"action": "confirm", "projectRoot": root_text, "planId": plan_id, "answers": {"route:1:5": "/pricing (Recommended)", "group:1:2": "One responsive page (Recommended)", @@ -310,3 +335,26 @@ async fn validate_warns_on_bare_controls_missing_metadata_and_no_icon() -> anyho std::fs::remove_dir_all(root).ok(); Ok(()) } + +/// The project plan, too, is confirmed only with the user's receipt; the +/// refusal comes before anything is read or written. +#[tokio::test] +async fn a_project_plan_confirm_without_the_users_receipt_is_refused() -> anyhow::Result<()> { + let root = project("project-approval"); + let root_text = root.to_str().unwrap(); + let (failed, refused) = call( + &root, + "devup_project_plan", + json!({"action": "confirm", "projectRoot": root_text, "planId": "any", "acceptRecommended": true}), + ) + .await?; + assert!(failed, "{refused}"); + let text = refused.to_string(); + assert!( + text.contains("userApproval") && text.contains("question tool"), + "{text}" + ); + assert!(!root.join(".devup/project-plan.json").exists()); + std::fs::remove_dir_all(root)?; + Ok(()) +}