fix(diagrams): #104 review — a repair is known by what it says, a replaced drawing is not handed back, and a replay keeps the order

Three findings from the Copilot review. All three are right.

agent.js:1041 — "this discards parsed.value and passes the raw string … the
original is degraded and a second card is drawn". The service decides whether a
call is the waiting diagram's repair by its title and its node ids, and it read
them off the arguments as they arrived. Arguments that are loose JSON arrive as
text — no title, no nodes — so a repair written with a trailing comma was a
"different diagram": the first attempt was settled as a degraded picture of its
own and the correction was drawn as a second. The same went for a first attempt
in loose JSON (its repair never found it), for a spec inside a `{"spec": …}`
envelope, and for one that says `name` for `title`.

The identity is now read from the spec as the ladder reads it — normalize(),
the lossless rung — whatever form it came in. agent.js is unchanged: it still
hands over the text, so the loose JSON is still counted as the auto-fix it was.
The placeholder's title comes from the same reading, so a call that arrived as
text has one too. And a second drawing of the same title in one run is
recognised as a redraw when it arrives as text.

service.js:260 — "this emits stubs for records that a later drawing replaced".
A redraw replaces the earlier drawing in the chat and a reopened session leaves
it out, but at compaction the model was handed a stub for each — so it could
see, fetch and edit a diagram the user no longer has. A replaced drawing now
gets no stub, wherever its replacement is in the conversation; it is not among
the known ids; and get_diagram, asked for it by an id the model may still
remember, answers with the id of the drawing the chat shows — the end of the
chain when it was redrawn more than once. A record that names itself, or two
that name each other, is a damaged file and not a reason to hang.

sessionEvents.js:149 — "all text blocks are collapsed and emitted before the
diagram". A message that says something, draws, and says more was replayed as
all of its prose and then the picture. It is now walked block by block: prose,
picture, prose, as it was on screen. Every piece after the first carries
`cont: true`, so a reader of the turns can still tell where a message ends
(the chat page does not need it: it already gives a run of answers one
speaker label). A message with no picture in it comes out exactly as it did,
one turn of all its text — and so does every caller that passes no records.

The Markdown export is built on those turns, and getting its order right
showed two more things wrong with it. A picture with no prose before it was
written under whoever spoke last — after a question, under "You". And once a
message is split, the prose after its picture would have been a turn of its
own. The export now works message by message: one rule and one speaker label
per message that says something, its pictures where they stood; a message that
is only a picture joins the answer above it, as before, unless there is none;
a diagram that was never drawn is dropped only after the pieces are grouped,
so the prose after it is not appended to the message before; and the count in
the header is still the number of messages that said something.

Tests, each failing on the code as reviewed. Through the real agent loop: a
repair in loose JSON, a first attempt in loose JSON, three envelope and
renamed-field shapes, a repair with the title reworded, and a redraw in loose
JSON — one diagram on file each time — and a different diagram in loose JSON,
which must NOT be taken for the same one. Three drawings of one diagram leave
one stub, and none in a stretch that holds only a replaced one; the same after
a reload, and in a file whose records name each other. A message of prose,
picture, another tool's call, prose and picture replays in that order with the
same words, and is exported as expected in five shapes.

Author: ndemianc

extensions/levelcode-ai/diagram/service.js

@@ -35,6 +35,18 @@ const idsOf = (input) => {
 	return nodes.map((n) => norm(n && typeof n === 'object' ? (n.id != null ? n.id : n.label) : n)).filter(Boolean);
 };
 const titleOf = (input) => norm(input && typeof input === 'object' ? input.title : '');
+/**
+ * What a call is a drawing OF: its title and its node ids, as the ladder READS them — not as they
+ * happened to arrive. A spec is the same diagram whether it came as an object, as JSON text with a
+ * trailing comma, inside a `{"spec": …}` envelope or with `name` for `title`; told apart by the raw
+ * arguments, a repair in any of those forms has no title and no nodes, and is taken for a new diagram.
+ * `shown` is the title for the placeholder.
+ */
+function identityOf(input) {
+	let spec = null;
+	try { spec = repair.normalize(input).spec; } catch (e) { spec = null; }
+	return { title: titleOf(spec), ids: idsOf(spec), shown: titleFrom(spec) };
+}
 /** Is this call another go at the same diagram? Same title, or mostly the same nodes. */
 function sameDiagram(a, b) {
 	if (a.title && a.title === b.title) { return true; }
@@ -150,7 +162,7 @@ function createDiagrams(opts) {
 	 */
 	function render(input, meta) {
 		const m = meta || { key: '' };
-		const me = { title: titleOf(input), ids: idsOf(input) };
+		const me = identityOf(input);
 		const post = [];
 		let pending = run.pending;
 		// Something else was sent back for repair and the model has moved on to a different diagram:
@@ -172,7 +184,7 @@ function createDiagrams(opts) {
 			run.pending = { key: String(m.key), input, model: m.model, title: me.title, ids: me.ids };
 			run.bounces++;
 			stat(Object.assign({ type: 'call', model: m.model, format: 'graph', outcome: 'bounced', tokens }, classes));
-			post.push({ type: 'diagramPending', key: String(m.key), state: 'repairing', title: typeof (input && input.title) === 'string' ? repair.cleanText(input.title).slice(0, 120) : '' });
+			post.push({ type: 'diagramPending', key: String(m.key), state: 'repairing', title: me.shown });
 			return { result: tool.result(prepared), post };
 		}
 
@@ -230,30 +242,53 @@ function createDiagrams(opts) {
 	}
 	function reset() { records.clear(); byKey.clear(); fresh = []; seq = 0; beginRun(); }
 
+	/**
+	 * The drawings a later one took the place of: id → the id of the drawing that replaced it. The chat
+	 * shows the later one where the earlier one stood, and a reopened session leaves the earlier one out
+	 * (sessionEvents.toDisplayTurns) — so the model is not handed it back either: no stub at compaction,
+	 * and get_diagram answers with the one that is on screen.
+	 */
+	function replacedBy() {
+		const next = new Map();
+		for (const r of records.values()) { if (r.replaces && String(r.replaces) !== r.id) { next.set(String(r.replaces), r.id); } }
+		return next;
+	}
+	/** The end of the chain of redraws that began with `id` — what the chat shows now. */
+	function shownFor(id, next) {
+		const seen = new Set([id]);
+		let cur = id;
+		while (next.has(cur) && !seen.has(next.get(cur))) { cur = next.get(cur); seen.add(cur); }   // (a damaged file may loop)
+		return cur;
+	}
+
 	/** get_diagram: the spec as the model would write it, or a plain reason there is none. */
 	function fetch(id) {
 		const want = String(id == null ? '' : id).trim();
 		const r = get(want);
+		const gone = replacedBy();
 		if (!r) {
-			const known = list().filter((x) => x.spec).map((x) => x.id);
+			const known = list().filter((x) => x.spec && !gone.has(x.id)).map((x) => x.id);
 			return 'ERROR: no diagram with id "' + want.slice(0, 40) + '" in this session.' + (known.length ? ' Known ids: ' + validator.listIds(known) + '.' : ' None has been drawn yet.');
 		}
+		if (gone.has(r.id)) { const now = shownFor(r.id, gone); return 'ERROR: diagram ' + r.id + ' was replaced by ' + now + ' — the chat shows ' + now + '. Call get_diagram with "' + now + '".'; }
 		if (!r.spec) { return 'ERROR: diagram ' + r.id + ' was never drawn (its spec was invalid). Draw it again with render_diagram.'; }
 		return text.toSource(r.spec);
 	}
 
 	/**
 	 * The one-line stubs for every diagram whose spec is in `messages` — what replaces those specs
-	 * when that stretch of the conversation is compacted. Nothing here runs turn by turn.
+	 * when that stretch of the conversation is compacted. Nothing here runs turn by turn. A drawing
+	 * that a later one replaced gets none: the user no longer sees it, wherever its replacement is.
 	 */
 	function stubsFor(messages) {
 		const out = [];
+		const gone = replacedBy();
 		for (const msg of (Array.isArray(messages) ? messages : [])) {
 			if (!msg || msg.role !== 'assistant' || !Array.isArray(msg.content)) { continue; }
 			for (const b of msg.content) {
 				if (!b || b.type !== 'tool_use' || b.name !== tool.RENDER_DIAGRAM.name) { continue; }
 				const r = byToolUse(b.id);
-				if (r && r.spec) { out.push(text.stub(r)); }
+				if (r && r.spec && !gone.has(r.id)) { out.push(text.stub(r)); }
 			}
 		}
 		return out;

extensions/levelcode-ai/sessionEvents.js

@@ -127,27 +127,42 @@ function messageText(content) {
  * chat did, not like the raw transcript. Pure, so the webview never has to know provider message shapes.
  *
  * With `diagrams` (the session's stored records), each diagram comes back as a turn of its own —
- * `{ role: 'diagram', key, record }` — at the place in the answer where the model drew it. A drawing
- * that a later one replaced is left out, as it was on screen. Without `diagrams` the result is exactly
- * what it always was: prose only.
+ * `{ role: 'diagram', key, record }` — at the place in the answer where the model drew it: a message
+ * that says something, draws, and says more comes back as prose, picture, prose, the way it was on
+ * screen. Every piece of such a message after its first is marked `cont: true` — the same message
+ * going on, not the next one — so a reader of the turns can still tell where a message ends. A
+ * drawing that a later one replaced is left out, as it was on screen. Without `diagrams` the result
+ * is exactly what it always was: prose only, one turn per message.
  */
 function toDisplayTurns(messages, diagrams) {
 	const out = [];
 	const records = Array.isArray(diagrams) ? diagrams : [];
 	const byKey = new Map(records.filter((r) => r && r.key).map((r) => [String(r.key), r]));
-	const replaced = new Set(records.filter((r) => r && r.replaces).map((r) => String(r.replaces)));
+	const replaced = new Set(records.filter((r) => r && r.replaces && String(r.replaces) !== String(r.id)).map((r) => String(r.replaces)));
+	/** The diagram this block drew — if it drew one, and that one is still what the chat shows. */
+	const drawnBy = (b) => {
+		const r = b && b.type === 'tool_use' && b.name === DIAGRAM_TOOL ? byKey.get(String(b.id)) : null;
+		return r && !replaced.has(String(r.id)) ? r : null;
+	};
 	for (const m of (Array.isArray(messages) ? messages : [])) {
 		if (!m || (m.role !== 'user' && m.role !== 'assistant')) { continue; }
 		// a user message whose content is purely tool_result blocks is plumbing, not something the user typed
 		if (m.role === 'user' && Array.isArray(m.content) && m.content.length && m.content.every((b) => b && b.type === 'tool_result')) { continue; }
-		const text = messageText(m.content);
-		if (text.trim()) { out.push({ role: m.role, text }); }   // (an assistant turn that was only tool calls has none)
-		if (m.role === 'assistant' && byKey.size && Array.isArray(m.content)) {
+		if (m.role === 'assistant' && Array.isArray(m.content)) {
+			// An answer, block by block: the prose so far, then a picture, then the rest. With no picture
+			// in it — the usual case — that is one piece: all of its text, as it always was.
+			let prose = [], pieces = 0;
+			const piece = (turn) => { out.push(pieces++ ? Object.assign(turn, { cont: true }) : turn); };
+			const flush = () => { const text = messageText(prose); prose = []; if (text.trim()) { piece({ role: 'assistant', text }); } };
 			for (const b of m.content) {
-				const r = b && b.type === 'tool_use' && b.name === DIAGRAM_TOOL ? byKey.get(String(b.id)) : null;
-				if (r && !replaced.has(String(r.id))) { out.push({ role: 'diagram', key: String(r.key), record: r }); }
+				const r = drawnBy(b);
+				if (r) { flush(); piece({ role: 'diagram', key: String(r.key), record: r }); } else { prose.push(b); }
 			}
+			flush();
+			continue;
 		}
+		const text = messageText(m.content);
+		if (text.trim()) { out.push({ role: m.role, text }); }   // (an assistant turn that was only tool calls has none)
 	}
 	return out;
 }
@@ -190,7 +205,14 @@ function toMarkdown(meta, messages, opts) {
 	const m = meta || {};
 	const o = opts || {};
 	const scrub = typeof o.redact === 'function' ? o.redact : (s) => s;
-	const turns = toDisplayTurns(messages, o.diagrams).filter((t) => t.role !== 'diagram' || (t.record && t.record.spec));
+	// Message by message: the pieces of one message (prose, pictures, more prose) stay together. A
+	// diagram that was never drawn has nothing to export, and is dropped only AFTER the pieces are
+	// grouped — it may be the first piece of its message, and the prose after it is not the tail of
+	// the message before.
+	const answers = [];
+	for (const t of toDisplayTurns(messages, o.diagrams)) { if (t.cont && answers.length) { answers[answers.length - 1].push(t); } else { answers.push([t]); } }
+	const said = (pieces) => pieces.some((t) => t.role !== 'diagram');
+	const shown = answers.map((pieces) => pieces.filter((t) => t.role !== 'diagram' || (t.record && t.record.spec))).filter((pieces) => pieces.length);
 
 	const title = scrub(String(m.title || 'Untitled session')).trim() || 'Untitled session';
 	const when = String(m.updatedAt || m.createdAt || '').slice(0, 10);
@@ -200,17 +222,20 @@ function toMarkdown(meta, messages, opts) {
 	// never named a model or touched a file should read as a transcript, not as a form with blanks.
 	const bits = [];
 	if (when) { bits.push(when); }
-	const spoken = turns.filter((t) => t.role !== 'diagram').length;   // a diagram is part of an answer, not a turn of its own
+	const spoken = shown.filter(said).length;   // a turn is a message that says something: a diagram is part of an answer, not a turn of its own
 	bits.push(spoken + ' turn' + (spoken === 1 ? '' : 's'));
 	if (m.model) { bits.push('`' + scrub(String(m.model)) + '`'); }
 	if (files.length) { bits.push(files.slice(0, 6).map((f) => '`' + f + '`').join(', ')); }
 
 	let md = '# ' + title + '\n\n';
 	md += '_LevelCode session · ' + bits.join(' · ') + '_\n';
 
-	for (const t of turns) {
+	// One rule and one speaker label per message that says something; its pictures stand in it where
+	// they stood. A message that is ONLY a picture joins the answer above it — no rule, no second
+	// label — unless there is none: under the question, it would read as part of the question.
+	let speaker = '';
+	const piece = (t) => {
 		if (t.role === 'diagram') {
-			// Under the answer it belongs to — no rule, no second speaker label.
 			const title = scrub(String((t.record.spec && t.record.spec.title) || 'Diagram')).replace(/\s+/g, ' ').trim();
 			let block = null;
 			try { block = typeof o.diagram === 'function' ? o.diagram(t.record) : null; } catch (e) { block = null; }
@@ -223,13 +248,17 @@ function toMarkdown(meta, messages, opts) {
 			} else {
 				md += '\n_[diagram: ' + title + ']_\n';
 			}
-			continue;
+			return;
 		}
-		md += '\n---\n\n**' + (t.role === 'user' ? 'You' : 'LevelCode') + '**\n\n';
 		// Verbatim aside from redaction — NOT trimmed. Trimming changes Markdown semantics: it de-indents a
-		// leading 4-space (indented code block) and eats trailing "  " (a hard line break). The `**role**\n\n`
-		// above already supplies the blank line an indented block needs after it.
-		md += scrub(String(t.text)) + '\n';
+		// leading 4-space (indented code block) and eats trailing "  " (a hard line break). The blank line
+		// before it — after the `**role**` label, or after a picture — is the one an indented block needs.
+		md += '\n' + scrub(String(t.text)) + '\n';
+	};
+	for (const pieces of shown) {
+		const role = pieces[0].role === 'user' ? 'user' : 'assistant';
+		if (said(pieces) || speaker !== 'assistant') { md += '\n---\n\n**' + (role === 'user' ? 'You' : 'LevelCode') + '**\n'; speaker = role; }
+		pieces.forEach(piece);
 	}
 	// An empty session still exports — a file with a header and no turns is a truthful answer, and
 	// silently producing nothing would read as a broken button.

extensions/levelcode-ai/test/diagramAgent.test.js

@@ -53,6 +53,10 @@ const jev = () => ({
 const broken = () => { const s = jev(); s.edges[1].to = 'billing'; s.nodes[2].accent = true; return s; };
 const call = (id, input) => ({ stop_reason: 'tool_use', content: [{ type: 'text', text: 'Here is the flow.' }, { type: 'tool_use', id, name: 'render_diagram', input }] });
 const say = (text) => ({ stop_reason: 'end_turn', content: [{ type: 'text', text }] });
+/** The same call when its arguments did not parse as JSON: the provider hands over the text it received. */
+const sloppyCall = (id, argsText) => ({ stop_reason: 'tool_use', content: [{ type: 'text', text: 'Here is the flow.' }, { type: 'tool_use', id, name: 'render_diagram', input: {} }], malformed: new Set([id]), raw: new Map([[id, argsText]]) });
+/** A spec as a model writes it on a bad day: every list and the object itself end in a comma. */
+const loose = (spec) => JSON.stringify(spec).replace(/\]/g, ',]').replace(/\}$/, ',}');
 
 /** Run one goal against a script of turns. Returns what was posted, what the model was sent, and the service. */
 async function run(turns, overrides) {
@@ -237,6 +241,60 @@ async function run(turns, overrides) {
 			assert.strictEqual(r.shown[r.shown.length - 1].replacesKey, 'toolu_1');
 		});
 
+		// The call that corrects a diagram is that diagram's repair in whatever FORM it arrives. Told apart
+		// by the raw arguments, a repair written as loose JSON (or wrapped in an envelope) had no title and
+		// no nodes: the first attempt was settled as a picture of its own, and the repair drew a second.
+		await testAsync('LADDER: a repair that arrives as loosely written JSON is still the repair — one picture, in the placeholder', async () => {
+			const r = await run([call('toolu_1', broken()), sloppyCall('toolu_2', loose(jev())), say('Done: drawn.')]);
+			assert.match(r.results[0].content, /^ERROR: the diagram was not drawn/);
+			assert.deepStrictEqual(r.shown.map((m) => m.type + ':' + (m.state || m.record.status)), ['diagramPending:repairing', 'diagram:ok'], 'the first attempt is not drawn on its own');
+			assert.strictEqual(r.shown[1].replacesKey, 'toolu_1', 'the corrected picture lands where the failed attempt was waiting');
+			assert.strictEqual(r.shown[1].record.repaired, true);
+			assert.strictEqual(r.shown[1].record.spec.edges.length, 3, 'and it is the corrected spec, whole');
+			assert.strictEqual(r.diagrams.list().length, 1, 'one diagram on file, so one card on replay too');
+			assert.deepStrictEqual(r.events.filter((e) => e.type === 'call').map((e) => e.outcome), ['bounced', 'repaired']);
+			assert.strictEqual(r.events.filter((e) => e.type === 'call')[1].fixClasses['lenient-json'], 1, 'and the loose JSON is still counted as the auto-fix it was');
+			// with the title reworded too, it is still told by its nodes — read out of the text, like the title
+			const reworded = jev(); reworded.title = 'Routing by confidence';
+			const byNodes = await run([call('toolu_1', broken()), sloppyCall('toolu_2', loose(reworded)), say('Done: drawn.')]);
+			assert.deepStrictEqual(byNodes.shown.map((m) => m.type + ':' + (m.state || m.record.status) + ':' + (m.replacesKey || '-')), ['diagramPending:repairing:-', 'diagram:ok:toolu_1']);
+			assert.strictEqual(byNodes.diagrams.list().length, 1);
+		});
+
+		await testAsync('LADDER: a FIRST attempt in loose JSON is remembered by what it says, so its repair finds it', async () => {
+			const r = await run([sloppyCall('toolu_1', loose(broken())), call('toolu_2', jev()), say('Done: drawn.')]);
+			assert.deepStrictEqual(r.shown.map((m) => m.type + ':' + (m.state || m.record.status)), ['diagramPending:repairing', 'diagram:ok']);
+			assert.strictEqual(r.shown[0].title, 'Jev classifies; your code decides the action', 'the placeholder is titled from the text, too');
+			assert.strictEqual(r.shown[1].replacesKey, 'toolu_1');
+			assert.strictEqual(r.shown[1].record.repaired, true);
+			assert.strictEqual(r.diagrams.list().length, 1);
+			// still broken on the second go: this is the one repair pass, so it degrades — it does not bounce again
+			const twice = await run([sloppyCall('toolu_1', loose(broken())), sloppyCall('toolu_2', loose(broken())), say('Done: as far as it goes.')]);
+			assert.deepStrictEqual(twice.results.map((b) => (/^ERROR/.test(b.content) ? 'bounced' : 'drawn')), ['bounced', 'drawn'], '"no automatic second repair", whatever the JSON looked like');
+			assert.deepStrictEqual(twice.shown.filter((m) => m.type === 'diagram').map((m) => m.record.status + ':' + m.replacesKey), ['degraded:toolu_1']);
+		});
+
+		await testAsync('LADDER: a spec inside an envelope, or with its fields renamed, is told by what the ladder reads in it', async () => {
+			const renamed = () => { const s = jev(); return { name: s.title, nodes: s.nodes.map((x) => ({ key: x.id, text: x.label })), edges: s.edges }; };
+			for (const [first, second] of [[{ diagram: broken() }, { spec: JSON.stringify(jev()) }], [broken(), { graph: jev() }], [{ input: broken() }, renamed()]]) {
+				const r = await run([call('toolu_1', first), call('toolu_2', second), say('Done: drawn.')]);
+				const shape = JSON.stringify([Object.keys(first), Object.keys(second)]);
+				assert.deepStrictEqual(r.shown.map((m) => m.type + ':' + (m.state || m.record.status)), ['diagramPending:repairing', 'diagram:ok'], shape);
+				assert.strictEqual(r.shown[0].title, 'Jev classifies; your code decides the action', shape);
+				assert.strictEqual(r.shown[1].replacesKey, 'toolu_1', shape);
+				assert.strictEqual(r.diagrams.list().length, 1, shape);
+			}
+		});
+
+		await testAsync('LADDER: reading the text does not make every call the same diagram — a different one in loose JSON is still a different one', async () => {
+			const other = { title: 'A second, unrelated picture', nodes: [{ id: 'x', label: 'X' }, { id: 'y', label: 'Y' }], edges: [{ from: 'x', to: 'y' }] };
+			const r = await run([sloppyCall('toolu_1', loose(broken())), sloppyCall('toolu_2', loose(other)), say('Done: two diagrams.')]);
+			const drawn = r.shown.filter((m) => m.type === 'diagram');
+			assert.deepStrictEqual(drawn.map((m) => m.key + ':' + m.record.status), ['toolu_1:degraded', 'toolu_2:ok']);
+			assert.strictEqual(drawn[1].replacesKey, undefined);
+			assert.strictEqual(drawn[1].record.repaired, undefined, 'a first attempt of its own, not somebody else\'s repair');
+		});
+
 		await testAsync('REDRAW: drawing the same diagram again in one run replaces the first instead of stacking a second', async () => {
 			const again = jev(); again.direction = 'down';
 			const r = await run([call('toolu_1', jev()), call('toolu_2', again), say('Done: redrawn top to bottom.')]);
@@ -245,6 +303,10 @@ async function run(turns, overrides) {
 			assert.strictEqual(drawn[1].record.replaces, 'd-1');
 			assert.strictEqual(drawn[1].record.id, 'd-2');
 			assert.strictEqual(drawn[1].record.spec.direction, 'down');
+			// …and it is the same diagram again when the second drawing arrives as loose JSON
+			const text = await run([call('toolu_1', jev()), sloppyCall('toolu_2', loose(again)), say('Done: redrawn.')]);
+			const shown = text.shown.filter((m) => m.type === 'diagram');
+			assert.deepStrictEqual(shown.map((m) => m.record.id + ':' + (m.record.replaces || '-') + ':' + (m.replacesKey || '-')), ['d-1:-:-', 'd-2:d-1:toolu_1']);
 		});
 
 		// ---- extraction: truncated vs merely sloppy -------------------------------------------------------