From e388568a792c218ed6e654a57c8c348247b693b7 Mon Sep 17 00:00:00 2001 From: Michael B Reiser Date: Fri, 29 May 2026 00:10:43 -0400 Subject: [PATCH] feat(v3): validation error line numbers + Phase 7 test hardening (v0.12) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - collectBlockingErrors now reports source line numbers for anchor errors (e.g. 'Duplicate anchor name: "&dup" declared 2 times (lines 5, 6)'). It re-parses _doc.toString() with a YAML.LineCounter and maps node.range[0] to a line. Structural validateReferences errors stay line-less (model-based); dangling-alias errors are line-less by nature (a dangling alias makes the doc non-serializable, so the re-parse falls back gracefully and still reports the error). New _linesSuffix helper formats "(line N)" / "(lines N, M)". - Fixed a double-bullet in the confirm-modal list (callers prefix "• ", so the
  • disc marker is now suppressed via list-style: none). - Suite 31 (21 checks): comment preservation at strategic positions (head/section/inline/between), anchor edge cases (two anchors same value, binding isolation), randomized-block semantics (randomize true/false round-trip), and validation line-number coverage. Full suite 467/467. - Footer bumped v0.11 -> v0.12. Browser-verified: the validation modal renders line numbers and a single clean bullet. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/development/v3-editor-handoff-2.md | 26 ++-- experiment_designer_v3.html | 7 +- js/protocol-yaml-v3.js | 69 +++++++++-- tests/test-protocol-roundtrip-v3.js | 150 ++++++++++++++++++++++++ 4 files changed, 230 insertions(+), 22 deletions(-) diff --git a/docs/development/v3-editor-handoff-2.md b/docs/development/v3-editor-handoff-2.md index e854622..7df0327 100644 --- a/docs/development/v3-editor-handoff-2.md +++ b/docs/development/v3-editor-handoff-2.md @@ -1,8 +1,8 @@ # v3 Experiment Designer — Handoff for Next Session (Round 2) -**Last updated:** 2026-05-28 (Phase 6 session) -**Branch:** `phase5/variables-ux` (Phase 5 `de31235` + Phase 6 this session) → PR to `main` -**Editor version:** v3 Experiment Designer **v0.11** (Phase 6 shipped this session) +**Last updated:** 2026-05-29 (Phase 6 + Phase 7 follow-up) +**Branch:** `phase7/test-hardening-and-line-numbers` → PR to `main` (Phase 6 already merged via #77) +**Editor version:** v3 Experiment Designer **v0.12** (validation line numbers + Phase 7 tests) **Pinned upstream:** maDisplayTools `origin/version3` at `649d7ef` This is the second handoff doc for the v3 designer. It supersedes the original @@ -312,7 +312,16 @@ blocked-when-used / works-when-unused). > edits (a stale import silently kills the whole module). A no-cache static > server (`.claude/nocache-server.py`, untracked) on a fresh port avoids it. -**Still deferred:** mapping validation errors to source **line numbers**. +**Follow-up shipped (v0.12):** the validation modal now reports **source line +numbers** for anchor errors — e.g. `Duplicate anchor name: "&dup" declared 2 +times (lines 5, 6)`. `collectBlockingErrors` re-parses `_doc.toString()` with a +`YAML.LineCounter` and maps each node's `range[0]` to a line. Structural +`validateReferences` errors stay line-less (model-based, no node handle). +Dangling-alias errors are also line-less by nature: a dangling alias makes the +doc non-serializable (`toString()` throws "Unresolved alias"), so the +re-parse can't run — the check falls back to a range-less scan and still +reports the error, just without a line. Also fixed a double-bullet in the +modal list (callers prefix `• `, so the `
  • ` disc marker is suppressed). ### Tier 4: D4 — cross-library import (parked — ~5+ days when picked up) @@ -330,9 +339,12 @@ implementation: ### Tier 5: Phase 7-9 — polish (~1 day total) -- Phase 7: comment-preservation tests at strategic positions, anchor edge - cases (numbers, strings, params, two anchors same value), randomized-block - semantics tests, validation error cases. +- Phase 7 ✅ **largely done (v0.12)**: Suite 31 added comment-preservation at + strategic positions (head / section / inline / between), anchor edge cases + (two anchors same value, binding isolation), randomized-block semantics + (`randomize: true`/`false` round-trip), and validation line-number coverage. + Still open if wanted: params-level anchor cases, deeper validation-error + matrices. v3 suite now **467/467**. - Phase 8: write `docs/development/v3-matlab-validation.md` describing the MCP-driven MATLAB validation flow. - Phase 9: `experiment_designer_v3_quickstart.html` step-by-step diff --git a/experiment_designer_v3.html b/experiment_designer_v3.html index f945699..3b94775 100644 --- a/experiment_designer_v3.html +++ b/experiment_designer_v3.html @@ -321,7 +321,10 @@ } .confirm-modal .body ul { margin: 0; - padding-left: 1rem; + /* Callers prefix each item with a literal "• ", so suppress the + default
  • disc marker to avoid a double bullet. */ + list-style: none; + padding-left: 0.25rem; font-size: 0.75rem; color: var(--text-dim); max-height: 240px; @@ -1160,7 +1163,7 @@

    v3 Experiment Designer

    Reiser Lab | - v3 Experiment Designer v0.11 | 2026-05-28 22:55 ET + v3 Experiment Designer v0.12 | 2026-05-29 00:08 ET
    diff --git a/js/protocol-yaml-v3.js b/js/protocol-yaml-v3.js index c4287c2..86e4308 100644 --- a/js/protocol-yaml-v3.js +++ b/js/protocol-yaml-v3.js @@ -453,6 +453,15 @@ function validateReferences(experiment) { * Returns { ok, errors } — the same shape as validateReferences, so callers can * treat the two interchangeably. Never throws (guards on _doc / YAML.visit). */ +// Format a " (line N)" / " (lines N, M)" suffix from a list of line numbers, +// dropping any that couldn't be resolved. Returns '' when none are known. +function _linesSuffix(lines) { + const known = (lines || []).filter((n) => typeof n === 'number'); + if (known.length === 0) return ''; + const uniq = [...new Set(known)].sort((a, b) => a - b); + return uniq.length === 1 ? ' (line ' + uniq[0] + ')' : ' (lines ' + uniq.join(', ') + ')'; +} + function collectBlockingErrors(experiment) { // Fold in all structural reference errors first. const base = validateReferences(experiment); @@ -460,30 +469,61 @@ function collectBlockingErrors(experiment) { // The two anchor checks need the CST; skip gracefully when it's absent. if (experiment && experiment._doc && typeof YAML.visit === 'function') { - // Duplicate anchor names — count every node.anchor across the doc tree. - // The `Node` visitor matches Scalar/Map/Seq (and Alias, which carries - // `.source` not `.anchor`, so it never contributes a count). - const anchorCounts = new Map(); - YAML.visit(experiment._doc, { + // Re-parse the to-be-exported text with a LineCounter so error + // positions map to real line numbers in the export output. Mutated + // _doc nodes can lack source ranges; re-parsing _doc.toString() yields + // uniform, accurate positions against exactly what export will write. + // Falls back to a range-less scan of the live _doc if the yaml build + // lacks LineCounter/parseDocument or anything throws. + let scanDoc = experiment._doc; + let lineOf = () => null; + try { + if (typeof YAML.LineCounter === 'function' && typeof YAML.parseDocument === 'function') { + const lc = new YAML.LineCounter(); + scanDoc = YAML.parseDocument(experiment._doc.toString(), { lineCounter: lc }); + lineOf = (node) => + node && Array.isArray(node.range) && typeof node.range[0] === 'number' + ? lc.linePos(node.range[0]).line + : null; + } + } catch (_e) { + scanDoc = experiment._doc; + lineOf = () => null; + } + + // Duplicate anchor names — collect every node.anchor across the doc + // tree with its line. The `Node` visitor matches Scalar/Map/Seq (and + // Alias, which carries `.source` not `.anchor`, so it never counts). + const anchorLines = new Map(); // name -> [line, ...] + YAML.visit(scanDoc, { Node(_, node) { if (node && node.anchor) { - anchorCounts.set(node.anchor, (anchorCounts.get(node.anchor) || 0) + 1); + const lines = anchorLines.get(node.anchor) || []; + lines.push(lineOf(node)); + anchorLines.set(node.anchor, lines); } } }); - const declaredAnchors = new Set(anchorCounts.keys()); - for (const [name, count] of anchorCounts) { - if (count > 1) { - errors.push('Duplicate anchor name: "&' + name + '" declared ' + count + ' times'); + const declaredAnchors = new Set(anchorLines.keys()); + for (const [name, lines] of anchorLines) { + if (lines.length > 1) { + errors.push( + 'Duplicate anchor name: "&' + + name + + '" declared ' + + lines.length + + ' times' + + _linesSuffix(lines) + ); } } // Dangling aliases — an alias whose source has no anchor declaration // anywhere in the doc (variables:, conditions:, complex nodes — all // count as declared). Deduped so N references to one missing anchor - // produce a single error. + // produce a single error (reporting the first reference's line). const danglingSeen = new Set(); - YAML.visit(experiment._doc, { + YAML.visit(scanDoc, { Alias(_, node) { if ( node && @@ -493,7 +533,10 @@ function collectBlockingErrors(experiment) { ) { danglingSeen.add(node.source); errors.push( - 'Dangling alias: "*' + node.source + '" has no matching anchor declaration' + 'Dangling alias: "*' + + node.source + + '" has no matching anchor declaration' + + _linesSuffix([lineOf(node)]) ); } } diff --git a/tests/test-protocol-roundtrip-v3.js b/tests/test-protocol-roundtrip-v3.js index 94f36cd..df1ad9b 100644 --- a/tests/test-protocol-roundtrip-v3.js +++ b/tests/test-protocol-roundtrip-v3.js @@ -2345,6 +2345,156 @@ console.log('\n--- Suite 30: collectBlockingErrors + library delete ---'); checkTrue('lib-del: round-trip still validates', collectBlockingErrors(reparsed).ok); } +// ─── Test Suite 31: Phase 7 — comments, anchor edge cases, randomize, lines ── +console.log('\n--- Suite 31: comment preservation + anchor edge cases + randomize + line numbers ---'); + +// 31.1 — validation error line numbers: duplicate anchor reports its lines +{ + const yaml = [ + 'version: 3', + 'experiment_info: {name: x}', + 'rig: "/tmp/r.yaml"', + 'variables:', + ' a: &dup 1', + ' b: &dup 2', + 'experiment: [foo]', + 'conditions:', + ' - name: foo', + ' commands: [{type: wait, duration: 1}]' + ].join('\n') + '\n'; + const exp = parseV3Protocol(yaml); + const dupErr = collectBlockingErrors(exp).errors.find((e) => /Duplicate anchor/.test(e)); + checkTrue('lines: duplicate-anchor error carries line numbers', /\(lines \d+, \d+\)/.test(dupErr), dupErr); +} + +// 31.2 — dangling alias: still detected via graceful fallback. A dangling +// alias makes the doc non-serializable (toString throws "Unresolved alias"), +// so the line-number re-parse can't run — detection must still work without a +// line number, and collectBlockingErrors must not throw. +{ + const yaml = [ + 'version: 3', + 'experiment_info: {name: x}', + 'rig: "/tmp/r.yaml"', + 'variables:', + ' d: &d 7', + 'experiment: [foo]', + 'conditions:', + ' - name: foo', + ' commands: [{type: wait, duration: *d}]' + ].join('\n') + '\n'; + const exp = parseV3Protocol(yaml); + const varsNode = exp._doc.get('variables', true); + for (const pair of varsNode.items) { + if (pair.value && pair.value.anchor === 'd') pair.value.anchor = undefined; + } + let report; + let threw = false; + try { + report = collectBlockingErrors(exp); + } catch (_e) { + threw = true; + } + checkTrue('lines: collectBlockingErrors survives unserializable doc', !threw); + checkTrue( + 'lines: dangling alias still detected on fallback', + report && !report.ok && report.errors.some((e) => /Dangling alias.*\bd\b/.test(e)) + ); +} + +// 31.3 — clean doc produces no spurious line-suffixed errors +{ + const exp = parseV3Protocol(readFixture('v3_canonical_a.yaml')); + const report = collectBlockingErrors(exp); + checkTrue('lines: clean doc has no errors at all', report.ok && report.errors.length === 0); +} + +// 31.4 — comment preservation at strategic positions (head / section / inline) +{ + const yaml = [ + '# HEAD comment line', + 'version: 3', + 'experiment_info: {name: x}', + 'rig: "/tmp/r.yaml"', + '# SECTION comment before variables', + 'variables:', + ' speed: &speed 12 # INLINE trailing comment', + 'experiment: [foo, bar]', + 'conditions:', + ' # BETWEEN comment before first condition', + ' - name: foo', + ' commands: [{type: wait, duration: *speed}]', + ' - name: bar', + ' commands: [{type: wait, duration: 1}]' + ].join('\n') + '\n'; + const regen = generateV3Protocol(parseV3Protocol(yaml)); + checkTrue('comments: HEAD comment preserved', regen.includes('# HEAD comment line')); + checkTrue('comments: SECTION comment preserved', regen.includes('# SECTION comment before variables')); + checkTrue('comments: INLINE trailing comment preserved', regen.includes('# INLINE trailing comment')); + checkTrue('comments: BETWEEN comment preserved', regen.includes('# BETWEEN comment before first condition')); + // round-trip stability: comment count stable across a second pass + const c1 = regen.split('\n').filter((l) => l.includes('#')).length; + const c2 = generateV3Protocol(parseV3Protocol(regen)).split('\n').filter((l) => l.includes('#')).length; + check('comments: count stable on second round-trip', c2, c1); +} + +// 31.5 — two distinct anchors with the SAME value both round-trip independently +{ + const yaml = [ + 'version: 3', + 'experiment_info: {name: x}', + 'rig: "/tmp/r.yaml"', + 'variables:', + ' first: &first 5', + ' second: &second 5', + 'experiment: [foo]', + 'conditions:', + ' - name: foo', + ' commands:', + ' - {type: wait, duration: *first}', + ' - {type: wait, duration: *second}' + ].join('\n') + '\n'; + const exp = parseV3Protocol(yaml); + const regen = generateV3Protocol(exp); + checkTrue('anchors-same-value: &first declared', /&first\b/.test(regen)); + checkTrue('anchors-same-value: &second declared', /&second\b/.test(regen)); + checkTrue('anchors-same-value: *first referenced', /\*first\b/.test(regen)); + checkTrue('anchors-same-value: *second referenced', /\*second\b/.test(regen)); + checkTrue('anchors-same-value: no blocking errors (not treated as dup)', collectBlockingErrors(exp).ok); +} + +// 31.6 — binding one field leaves an unrelated anchor's references intact +{ + const exp = parseV3Protocol(readFixture('v3_canonical_a.yaml')); + const shortBefore = findAliasesTo(exp, 'dur_short').length; + // bind some literal to dur_long; dur_short references must be untouched + const longRefs = findAliasesTo(exp, 'dur_long'); + checkTrue('anchor-isolation: precondition dur_long has refs', longRefs.length > 0); + docBindToAnchor(exp, longRefs[0].path.slice(), 'dur_short'); // rebind one to dur_short via path + const shortAfter = findAliasesTo(exp, 'dur_short').length; + check('anchor-isolation: dur_short ref count increased by exactly 1', shortAfter, shortBefore + 1); +} + +// 31.7 — randomize:true round-trips (canonical_a block) +{ + const exp = parseV3Protocol(readFixture('v3_canonical_a.yaml')); + const block = exp.sequence.find((e) => e.kind === 'block'); + checkTrue('randomize: canonical block has randomize true', block && block.randomize === true); + const regen = generateV3Protocol(exp); + checkTrue('randomize: true survives in regen', /randomize:\s*true/.test(regen)); +} + +// 31.8 — randomize:false explicit is NOT dropped on round-trip +{ + const exp = parseV3Protocol(readFixture('v3_no_randomize.yaml')); + const block = exp.sequence.find((e) => e.kind === 'block'); + checkTrue('randomize: no_randomize block parses randomize false', block && block.randomize === false); + const regen = generateV3Protocol(exp); + checkTrue('randomize: explicit false preserved in regen', /randomize:\s*false/.test(regen)); + // repetitions survive too + checkTrue('randomize: repetitions preserved', block.repetitions === 2); +} + // ─── Results ──────────────────────────────────────────────────────────────── console.log('\n=== Results: ' + passedTests + '/' + totalTests + ' passed ==='); if (failedTests.length > 0) {