From 7b9c92eb9811797362f4b4cd21177ba222b87e31 Mon Sep 17 00:00:00 2001 From: MechaCat02 Date: Wed, 9 Sep 2026 21:52:02 +0200 Subject: [PATCH] fix(tutor): raw ::task markup rendered as prose above the exercise Reported from the app: "::task translate" and its five sentences appeared as text in the message, directly above the exercise those same lines had been rendered into. It is lib/blocks.js, not the model. parse() removes ::words by truncating the body at its index, then removes ::task by substring: if (w) body = body.slice(0, body.indexOf("::words")); if (t) body = body.replace(t[0], ""); RE.task's terminator (?:\n::|$) is part of the match, so t[0] ends with the "\n::" belonging to the ::words that follows -- the two colons the line above just truncated away. The substring no longer occurs, replace() is a no-op, and the whole task block stays in the body. Order is the whole trigger. The stub tutor emits ::words before ::task and is therefore fine; the local model emitted ::task first. Nothing in the prompt requires either order, so this was always reachable -- Claude would hit it too. It survived every test until a real model chose the other way. lib/ ships unchanged, so the fix is at the call site, next to the ::gloss workaround that is there for the same reason: parseMessage() drops the body from the first surviving directive line on. Safe precisely because parse() has already removed the blocks it handled correctly, so a "::" still in the body is by definition one that leaked. That also subsumes the streaming filter added earlier, which is now one rule instead of two. A turn can also be nothing but blocks -- this model writes no prose around an exercise at all -- which left an empty bubble above it. The bubble is skipped when there is nothing to put in it, and the typing dots stay up while the reply so far is only markup, since there is genuinely nothing to read yet. test/domain/block-leak.test.ts pins the lib behaviour as it is, and the workaround against a verbatim capture of the model output that produced the report. Co-Authored-By: Claude Opus 5 --- app/src/domain/gloss.ts | 48 ++++++++++++++++-- app/src/ui/tutor/TutorTab.tsx | 37 ++++++++------ test/domain/block-leak.test.ts | 92 ++++++++++++++++++++++++++++++++++ 3 files changed, 158 insertions(+), 19 deletions(-) create mode 100644 test/domain/block-leak.test.ts diff --git a/app/src/domain/gloss.ts b/app/src/domain/gloss.ts index 6e8d9a8..1c59d7b 100644 --- a/app/src/domain/gloss.ts +++ b/app/src/domain/gloss.ts @@ -1,4 +1,29 @@ -/* Multi-sentence ::gloss blocks. +/* Two workarounds for lib/blocks.js, which ships unchanged. + + ── 1. Directive markup leaking into the message body ────────────────── + + parse() builds `body` by removing each block it recognised, but it does + the removals in an order that defeats one of them: + + if (w) body = body.slice(0, body.indexOf("::words")); // truncate + if (t) body = body.replace(t[0], ""); // substring + + RE.task's terminator `(?:\n::|$)` is INSIDE the match, so t[0] ends with + the "\n::" of whatever block follows. When that block is ::words, the + truncation on the line above has already cut those two colons off, the + substring no longer occurs, replace() finds nothing, and the entire task + block stays in the body — rendered as prose above the exercise it was + supposed to become. + + It only bites when ::task precedes ::words. The stub tutor emits them the + other way round, which is why this survived every test until a real model + chose the other order. Nothing in the prompt requires one. + + The fix here: drop anything from the first surviving directive line on. + Safe precisely because parse() has already removed the blocks it handled + correctly, so a `::` left in the body is by definition one that leaked. + + ── 2. Multi-sentence ::gloss blocks ─────────────────────────────────── The system prompt tells the tutor it may put several sentences in one gloss block, each closed by its own "=" line. lib/blocks.js parse() sets @@ -43,15 +68,28 @@ function splitSentences(body: string): string[] { * parse(), with multi-sentence gloss blocks split correctly. * Use this everywhere instead of calling parse() directly. */ +/** + * Cut the body at the first directive line that survived parse(). + * + * Exported for the test that pins the lib bug this exists for. + */ +export function stripLeakedBlocks(body: string): string { + const lines = body.split("\n"); + const at = lines.findIndex((l) => l.trimStart().startsWith("::")); + return at === -1 ? body : lines.slice(0, at).join("\n").trimEnd(); +} + export function parseMessage(text: string): ParsedMessage { const parsed = parse(text); - if (!parsed.gloss) return parsed; + const body = stripLeakedBlocks(parsed.body); + + if (!parsed.gloss) return { ...parsed, body }; const match = text.match(GLOSS_BLOCK); - if (!match?.[1]) return parsed; + if (!match?.[1]) return { ...parsed, body }; const sentences = splitSentences(match[1]); - if (sentences.length < 2) return parsed; // the common case; nothing to fix + if (sentences.length < 2) return { ...parsed, body }; // common case const blocks: GlossBlock[] = []; for (const s of sentences) { @@ -59,7 +97,7 @@ export function parseMessage(text: string): ParsedMessage { if (one.gloss) blocks.push(...one.gloss); } - return blocks.length ? { ...parsed, gloss: blocks } : parsed; + return blocks.length ? { ...parsed, body, gloss: blocks } : { ...parsed, body }; } export type { GlossBlock, ParsedMessage }; diff --git a/app/src/ui/tutor/TutorTab.tsx b/app/src/ui/tutor/TutorTab.tsx index fd32d04..ee29643 100644 --- a/app/src/ui/tutor/TutorTab.tsx +++ b/app/src/ui/tutor/TutorTab.tsx @@ -368,15 +368,12 @@ export function TutorTab() { on every unrelated re-render of this tab. */ const streamingBody = useMemo(() => { if (streaming === null) return ""; - /* A directive line arrives before the block it opens is complete, and - until then parse() has no reason to treat it as anything but prose — - so "::task match" and "::words" rendered as visible text for exactly - one frame and then vanished. That blink was the flicker. Blocks are - not rendered during streaming anyway, so drop the markers outright - and the preview only ever grows. */ - const lines = parseMessage(streaming) - .body.split("\n") - .filter((l) => !l.trimStart().startsWith("::")); + /* parseMessage() drops directive markup from the body, which during a + stream also covers the half-arrived kind: a directive line shows up + before the block it opens is complete, and until then parse() has no + reason to treat it as anything but prose. That is what made + "::task match" appear for one frame and vanish. */ + const lines = parseMessage(streaming).body.split("\n"); /* MessageBody renders a blank line as a 9px gap. Dropping a directive leaves the blank line that preceded it at the end of the preview, so a gap opened and closed on every block boundary. */ @@ -478,10 +475,19 @@ export function TutorTab() { return (
{you ? "나" : "선생님"} -
- - {parsed?.gloss && } -
+ {/* A turn can be nothing but blocks — some models write no + prose around an exercise at all. Rendering the bubble + anyway left an empty box above it. */} + {(() => { + const body = parsed ? parsed.body : t.body; + if (!body.trim() && !parsed?.gloss) return null; + return ( +
+ + {parsed?.gloss && } +
+ ); + })()} {/* An answered exercise stays rendered, read-only. It used to collapse to a single line, which threw away what the @@ -511,7 +517,10 @@ export function TutorTab() { {busy && (
선생님 - {streaming === null ? ( + {/* Keep the dots up while the reply so far is only block + markup: there is genuinely nothing to read yet, and an + empty bubble reads as a failure rather than as waiting. */} + {streamingBody.trim() === "" ? (
diff --git a/test/domain/block-leak.test.ts b/test/domain/block-leak.test.ts new file mode 100644 index 0000000..af2fc88 --- /dev/null +++ b/test/domain/block-leak.test.ts @@ -0,0 +1,92 @@ +/* The ::task block leaking into the message body. + + Reported from the app: the raw text "::task translate" and its five + sentences appeared as prose above the exercise those same lines had + already been rendered into. + + It is lib/blocks.js, not the model. parse() removes ::words by + truncating the body at its index, then removes ::task by substring — + but RE.task's terminator (?:\n::|$) is part of the match, so t[0] ends + with the "\n::" of the following ::words that the truncation just cut + off. The substring no longer occurs, replace() is a no-op, and the whole + task block stays in the body. + + Order is what decides it. The stub tutor emits ::words before ::task and + is therefore fine; a real model emitted ::task first. Nothing in the + prompt requires either order, so this was always reachable. + + The fixture is a verbatim capture from gpt-oss-20b through the server. */ + +import { describe, it, expect } from "vitest"; +import { parse } from "@lib/blocks.js"; +import { parseMessage, stripLeakedBlocks } from "@app/domain/gloss.js"; + +import type { ParsedMessage } from "@lib/blocks.js"; + +/** Narrow the Task union; only translate tasks appear in these fixtures. */ +function translateItems(task: ParsedMessage["task"]): { q: string }[] { + if (!task || task.type !== "translate") throw new Error(`expected a translate task, got ${task?.type}`); + return task.items; +} + +const CAPTURED = "::task translate\n\ub098 \uac00\ub2e4 \n\ub108 \uba39\ub2e4 \n\uc6b0\ub9ac \ub9c8\uc2dc\ub2e4 \n\uc774 \uc790\ub2e4 \n\uadf8 \ub098\ubb34 \uc11c\ub2e4 \n\n::words\n\ub098 | I, me (casual) | pron \n\uac00\ub2e4 | to go | verb (plain) \n\ub108 | you (casual) | pron \n\uba39\ub2e4 | to eat | verb (plain) \n\uc6b0\ub9ac | we, our | pron \n\ub9c8\uc2dc\ub2e4 | to drink | verb (plain) \n\uc774 | this | det \n\uc790\ub2e4 | to sleep | verb (plain) \n\uadf8 | that (near you) | det \n\ub098\ubb34 | tree | noun \n\uc11c\ub2e4 | to stand | verb (plain) \n\n::progress 20"; + +describe("lib/blocks.js — the leak, pinned as it is", () => { + it("leaves the whole ::task block in body when ::task precedes ::words", () => { + const r = parse(CAPTURED); + // The blocks themselves parse correctly... + expect(r.task?.type).toBe("translate"); + expect(translateItems(r.task)).toHaveLength(5); + expect(r.words).toHaveLength(11); + // ...and the body still carries the markup that produced them. + expect(r.body).toContain("::task translate"); + expect(r.body).toContain("나 가다"); + }); + + it("is fine in the other order, which is why the stub never showed it", () => { + const swapped = "Here you go.\n\n::words\n나 | I | pron\n\n::task translate\n나 가다\n"; + expect(parse(swapped).body).not.toContain("::task"); + }); +}); + +describe("parseMessage — the workaround", () => { + it("keeps the blocks and drops the leaked markup", () => { + const r = parseMessage(CAPTURED); + expect(r.body).not.toContain("::"); + expect(r.body).not.toContain("나 가다"); + // Nothing the blocks needed was lost. + expect(r.task?.type).toBe("translate"); + expect(translateItems(r.task)).toHaveLength(5); + expect(r.words).toHaveLength(11); + expect(r.progress?.score).toBe(20); + }); + + it("keeps prose that comes before the blocks", () => { + const r = parseMessage("좋아요. Try these.\n\n::task translate\n나 가다\n\n::words\n나 | I | pron\n"); + expect(r.body).toBe("좋아요. Try these."); + expect(translateItems(r.task)).toHaveLength(1); + }); + + it("still handles the order the stub uses", () => { + const r = parseMessage("Here you go.\n\n::words\n나 | I | pron\n\n::task translate\n나 가다\n"); + expect(r.body).toBe("Here you go."); + expect(translateItems(r.task)).toHaveLength(1); + }); + + it("leaves an ordinary message alone", () => { + const r = parseMessage("좋아요!\n\nThat is the shape of it."); + expect(r.body).toBe("좋아요!\n\nThat is the shape of it."); + }); +}); + +describe("stripLeakedBlocks", () => { + it("does nothing without a directive", () => { + expect(stripLeakedBlocks("a\n\nb")).toBe("a\n\nb"); + }); + it("cuts from the first directive line", () => { + expect(stripLeakedBlocks("keep\n::task translate\ndrop\n::words\ndrop")).toBe("keep"); + }); + it("does not trip on a colon that is not at the start of a line", () => { + expect(stripLeakedBlocks("see:: this")).toBe("see:: this"); + }); +});