From 5c29e66d7dc9a4d8faadfa50ed89c5d4fdbaea00 Mon Sep 17 00:00:00 2001 From: Jonas Kunert Date: Tue, 28 Jul 2026 17:34:46 +0200 Subject: [PATCH 01/22] fix: escape MCP server config display (fork-issue-39, escaping slice only) displayMCPServers() built configDisplay (server command, args, url and type) by interpolating config.command/config.args.join(' ')/config.url/ serverType straight into serverItem.innerHTML with no escaping. Any of those four values can come from external ~/.claude.json / .mcp.json data, so a malicious config.type like rendered as a live element. Wrap all four interpolations in escapeHtml(), matching the display- hardening half of upstream commit f759a955 (fork-issue-39, "harden MCP configuration handling"). That commit also contains a Windows spawn-quoting fix and a "show CLI local-scope servers" feature; both are unrelated config-path changes that belong in the MCP-cluster bundle, not in this XSS-hardening branch, so only the escaping lines are picked here. Placed first so every later commit in this branch builds on top of an already-escaped configDisplay. Co-Authored-By: Claude Opus 5 --- src/script.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/script.ts b/src/script.ts index 4c949e2..c146afa 100644 --- a/src/script.ts +++ b/src/script.ts @@ -2076,14 +2076,14 @@ const getScript = (isTelemetryEnabled: boolean, opencreditsApiUrl: string = 'htt let configDisplay = ''; if (serverType === 'stdio') { - configDisplay = \`Command: \${config.command || 'Not specified'}\`; + configDisplay = \`Command: \${escapeHtml(config.command || 'Not specified')}\`; if (config.args && Array.isArray(config.args)) { - configDisplay += \`
Args: \${config.args.join(' ')}\`; + configDisplay += \`
Args: \${escapeHtml(config.args.join(' '))}\`; } } else if (serverType === 'http' || serverType === 'sse') { - configDisplay = \`URL: \${config.url || 'Not specified'}\`; + configDisplay = \`URL: \${escapeHtml(config.url || 'Not specified')}\`; } else { - configDisplay = \`Type: \${serverType}\`; + configDisplay = \`Type: \${escapeHtml(serverType)}\`; } const scopeLabel = serverScope === 'global' ? 'Global' : serverScope === 'project' ? 'Project' : 'Extension'; From 296b032e23e110d06cecf5c9462a45c6e2886425 Mon Sep 17 00:00:00 2001 From: Jonas Kunert Date: Fri, 24 Jul 2026 18:06:40 +0200 Subject: [PATCH 02/22] fix: escape raw HTML in chat messages so tags cannot corrupt the view (fork-issue-40) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit parseSimpleMarkdown escaped fenced code blocks but passed prose and inline-code content through unescaped into innerHTML, so a literal HTML tag in a response — for example a " in + // Claude's/the user's text gets parsed as HTML and an unbalanced tag can + // corrupt the whole message. Code blocks were already pulled out above into + // __CODEBLOCK_N__ placeholders (their contents are escaped individually), + // and those placeholders contain only [A-Za-z0-9_], so escapeHtml leaves + // them unchanged. + processedMarkdown = escapeHtml(processedMarkdown); + // Handle inline code with single backticks const inlineCodeRegex = new RegExp('\\\`([^\\\`]+)\\\`', 'g'); processedMarkdown = processedMarkdown.replace(inlineCodeRegex, '$1'); From 019d1a3f8ed561a94624bf80937fe9dea5798daa Mon Sep 17 00:00:00 2001 From: Jonas Kunert Date: Fri, 24 Jul 2026 22:29:32 +0200 Subject: [PATCH 03/22] fix: escape user message previews in the history list (fork-issue-40) The conversation list interpolated firstUserMessage/lastUserMessage raw into innerHTML, so raw HTML typed in chat rendered as live elements in the history. Escape after truncation so entities are never cut apart. Co-Authored-By: Claude Fable 5 --- src/script.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/script.ts b/src/script.ts index fdb0d11..455d273 100644 --- a/src/script.ts +++ b/src/script.ts @@ -4767,9 +4767,9 @@ const getScript = (isTelemetryEnabled: boolean, opencreditsApiUrl: string = 'htt } item.innerHTML = \` -
\${conv.firstUserMessage.substring(0, 60)}\${conv.firstUserMessage.length > 60 ? '...' : ''}
+
\${escapeHtml(conv.firstUserMessage.substring(0, 60))}\${conv.firstUserMessage.length > 60 ? '...' : ''}
\${date} at \${time} • \${conv.messageCount} messages • \${usageStr}
-
Last: \${conv.lastUserMessage.substring(0, 80)}\${conv.lastUserMessage.length > 80 ? '...' : ''}
+
Last: \${escapeHtml(conv.lastUserMessage.substring(0, 80))}\${conv.lastUserMessage.length > 80 ? '...' : ''}
\`; listDiv.appendChild(item); From b9594d0215571ba1e754ec35b8ec388571cc59ca Mon Sep 17 00:00:00 2001 From: Jonas Kunert Date: Sun, 26 Jul 2026 09:01:40 +0200 Subject: [PATCH 04/22] feat: collapse long code blocks and add per-message fold control (fork-issue-48) Fenced code blocks over a configurable line threshold (default 20) render as native
/ with a caret, language and line count in the header; shorter blocks keep byte-identical markup. New claudeCodeChat.ui.collapseLongCodeBlocks / .collapseCodeBlockLines settings, with a catch-up pass in the webview because settingsData arrives after history replay. Every user/claude/error message also gets a manual fold button in its header, independent of the auto-collapse. Threshold/line-count logic lives in the self-contained, .toString()-spliced src/collapse-rules.ts, covered by a new test:collapse-rules suite (18 cases, including a splice-sandbox test). Co-Authored-By: Claude Sonnet 5 --- package.json | 15 ++++- src/collapse-rules.ts | 37 +++++++++++ src/collapse-script.ts | 53 ++++++++++++++++ src/extension.ts | 2 + src/script.ts | 53 +++++++++++++++- src/test/collapse-rules.test.ts | 108 ++++++++++++++++++++++++++++++++ src/ui-styles.ts | 47 ++++++++++++++ src/ui.ts | 15 +++++ 8 files changed, 327 insertions(+), 3 deletions(-) create mode 100644 src/collapse-rules.ts create mode 100644 src/collapse-script.ts create mode 100644 src/test/collapse-rules.test.ts diff --git a/package.json b/package.json index 6bcdf89..e01ba2d 100644 --- a/package.json +++ b/package.json @@ -208,6 +208,18 @@ "type": "boolean", "default": false, "description": "Enable the local router to convert OpenAI format to Anthropic format. Required for providers that use OpenAI-compatible APIs." + }, + "claudeCodeChat.ui.collapseLongCodeBlocks": { + "type": "boolean", + "default": true, + "description": "Collapse code blocks longer than the line threshold by default. They stay foldable via their header either way." + }, + "claudeCodeChat.ui.collapseCodeBlockLines": { + "type": "number", + "default": 20, + "minimum": 5, + "maximum": 500, + "description": "Number of lines a code block must exceed before it gets a collapsible header (5-500)." } } } @@ -221,7 +233,8 @@ "test": "vscode-test", "test:downloader": "npm run compile && mocha --ui tdd \"out/test/downloader*.test.js\" --reporter spec --timeout 360000", "test:downloader:unit": "npm run compile && mocha --ui tdd out/test/downloader.test.js --reporter spec", - "test:models": "npm run compile && mocha --ui tdd out/test/model-updater.test.js --reporter spec" + "test:models": "npm run compile && mocha --ui tdd out/test/model-updater.test.js --reporter spec", + "test:collapse-rules": "npm run compile && mocha --ui tdd out/test/collapse-rules.test.js --reporter spec" }, "devDependencies": { "@types/mocha": "^10.0.10", diff --git a/src/collapse-rules.ts b/src/collapse-rules.ts new file mode 100644 index 0000000..542a33b --- /dev/null +++ b/src/collapse-rules.ts @@ -0,0 +1,37 @@ +// Pure threshold/line-count logic for the #48 collapsible-code-blocks feature (upstream +// #151): decides whether a fenced code block parseSimpleMarkdown is about to render should +// start collapsed, based on its line count and the configured threshold. No vscode import, +// so this runs under plain mocha like diff-utils/shell-utils/auto-model-switch/math-segments. +// +// Neither function is ever called directly by the webview's copy of this file -- it has +// none. Instead collapse-script.ts injects each function's own compiled source via +// .toString() into the page (see collapse-script.ts for why). That means EVERY +// default/helper these functions need must be declared INSIDE their bodies: .toString() +// only ever returns the function's own text, not the rest of this module, so a +// module-level const referenced from inside a function would be undefined in the browser. + +export interface CodeBlockCollapseInfo { lineCount: number; collapse: boolean; maxLines: number; } + +export function normalizeCollapseThreshold(value: unknown): number { + // Defaults/Grenzen MUESSEN im Funktionskoerper stehen: .toString() liefert nur den + // Text dieser Funktion, kein Modul-Level-Symbol. + const DEFAULT_LINES = 20; // in sync mit claudeCodeChat.ui.collapseCodeBlockLines (package.json) + const MIN_LINES = 5; + const MAX_LINES = 500; + const n = typeof value === 'number' ? value : Number(value); + if (!Number.isFinite(n) || n <= 0) { return DEFAULT_LINES; } + const floored = Math.floor(n); + if (floored < MIN_LINES) { return MIN_LINES; } + if (floored > MAX_LINES) { return MAX_LINES; } + return floored; +} + +export function evaluateCodeBlockCollapse(code: string, configuredMaxLines: unknown): CodeBlockCollapseInfo { + const maxLines = normalizeCollapseThreshold(configuredMaxLines); + const text = typeof code === 'string' ? code : ''; + // Die Fence-Regex in parseSimpleMarkdown faengt das Newline VOR der schliessenden + // Fence mit: "a\nb\nc\n" sind 3 Zeilen, nicht 4. CRLF vorher normalisieren. + const normalized = text.replace(/\r\n/g, '\n').replace(/\n$/, ''); + const lineCount = normalized === '' ? 0 : normalized.split('\n').length; + return { lineCount: lineCount, collapse: lineCount > maxLines, maxLines: maxLines }; +} diff --git a/src/collapse-script.ts b/src/collapse-script.ts new file mode 100644 index 0000000..4d02942 --- /dev/null +++ b/src/collapse-script.ts @@ -0,0 +1,53 @@ +import { normalizeCollapseThreshold, evaluateCodeBlockCollapse } from './collapse-rules'; + +// Webview-side glue for the #48 collapsible-code-blocks feature (upstream #151), injected +// into script.ts's getScript() template the same way getMathScript()/getSkillsScript() are +// (see plugins-script.ts). Two different things happen below and they must not be confused: +// +// 1. normalizeCollapseThreshold.toString() / evaluateCodeBlockCollapse.toString() are REAL, +// host-side template interpolations (like findMathSegments.toString() in math-script.ts): +// they run in Node when getCollapseScript() is called, and splice each function's own +// *compiled* source into the returned string. collapse-rules.ts must stay fully +// self-contained for exactly this reason -- only its own text crosses into the browser, +// not the rest of that module. +// 2. Everything else below (markCodeBlockToggled, applyCodeBlockCollapseDefaults, +// toggleMessageCollapsed) is plain webview source written directly in this template +// literal. None of it happens to need a client-side "${...}" or a backtick, so nothing +// here needs the "\${"/"\`" escaping script.ts's own template literal requires elsewhere +// -- but if you add code that does, escape it the same way (see script.ts's +// parseSimpleMarkdown for examples). +const getCollapseScript = () => ` + // ─── Collapsible code blocks + per-message fold (#48) ─── + ${normalizeCollapseThreshold.toString()} + ${evaluateCodeBlockCollapse.toString()} + + // R2/R3: bound to the 's synchronous onclick, never the
's + // ontoggle -- toggle fires asynchronously and also for a programmatic .open + // assignment, which would make applyCodeBlockCollapseDefaults() below unable to + // tell a real user click from its own nachzieh pass after the first run. + function markCodeBlockToggled(summaryEl) { + summaryEl.parentElement.setAttribute('data-user-toggled', '1'); + } + + // Nachzieh-Pass (R1): settingsData arrives AFTER the history replay + // (extension.ts _loadConversationHistory -> _sendReadyMessage -> _sendCurrentSettings), + // so blocks rendered from history always start out using the webview's hardcoded + // default. This re-applies the real collapseLongCodeBlocks setting to every block the + // user hasn't touched yet; called once settingsData actually arrives (see script.ts). + function applyCodeBlockCollapseDefaults() { + var els = document.querySelectorAll('details.code-block-collapsible:not([data-user-toggled="1"])'); + for (var i = 0; i < els.length; i++) { els[i].open = !collapseLongCodeBlocks; } + } + + // Phase 2: manual per-message collapse via the caret button in .message-header + // (script.ts addMessage). Purely a CSS class toggle, no DOM removal -- see + // ui-styles.ts's ".message.collapsed" rules. + function toggleMessageCollapsed(messageDiv, btn) { + var collapsed = messageDiv.classList.toggle('collapsed'); + btn.textContent = collapsed ? '▸' : '▾'; + btn.title = collapsed ? 'Expand message' : 'Collapse message'; + btn.setAttribute('aria-expanded', collapsed ? 'false' : 'true'); + } +`; + +export default getCollapseScript; diff --git a/src/extension.ts b/src/extension.ts index 8fa37fb..1431652 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -3400,6 +3400,8 @@ class ClaudeChatProvider { 'executable.path': config.get('executable.path', ''), 'environment.variables': config.get>('environment.variables', {}), 'environment.disabled': config.get('environment.disabled', false), + 'ui.collapseLongCodeBlocks': config.get('ui.collapseLongCodeBlocks', true), + 'ui.collapseCodeBlockLines': config.get('ui.collapseCodeBlockLines', 20), 'isOpenCredits': this._isOpenCredits() }; diff --git a/src/script.ts b/src/script.ts index 455d273..ed1b3f7 100644 --- a/src/script.ts +++ b/src/script.ts @@ -1,5 +1,6 @@ import getSkillsScript from './skills-script'; import getPluginsScript from './plugins-script'; +import getCollapseScript from './collapse-script'; const getScript = (isTelemetryEnabled: boolean, opencreditsApiUrl: string = 'https://ccc.api.opencredits.ai', opencreditsWebUrl: string = 'https://ccc.opencredits.ai', opencreditsPublishableKey: string = 'oc_pk_c43da4f9a9484ae484ad29bc97cc354f') => `` diff --git a/src/test/collapse-rules.test.ts b/src/test/collapse-rules.test.ts new file mode 100644 index 0000000..bb513f0 --- /dev/null +++ b/src/test/collapse-rules.test.ts @@ -0,0 +1,108 @@ +// Unit tests for the #48 collapsible-code-blocks logic (normalizeCollapseThreshold / +// evaluateCodeBlockCollapse). Pure (no vscode, no network, no filesystem access), so these +// run under plain mocha against the compiled out/ output -- same pattern as +// diff-utils/shell-utils/auto-model-switch/math-segments. The first suite covers +// evaluateCodeBlockCollapse's line-counting edge cases (trailing newline, CRLF, empty +// input, a single very long line), the second covers normalizeCollapseThreshold's +// default/clamp behaviour, and the third is the splice-sandbox test that proves both +// functions still work with zero module context -- exactly how collapse-script.ts's +// .toString() splice runs them in the webview. Run with `npm run test:collapse-rules`. + +import * as assert from 'assert'; +import { evaluateCodeBlockCollapse, normalizeCollapseThreshold } from '../collapse-rules'; + +suite('collapse-rules: evaluateCodeBlockCollapse (line counting)', () => { + + test('3 lines at a threshold of 20 stay uncollapsed', () => { + const info = evaluateCodeBlockCollapse('a\nb\nc\n', 20); + assert.strictEqual(info.lineCount, 3); + assert.strictEqual(info.collapse, false); + }); + + test('exactly 20 lines at a threshold of 20 stay uncollapsed (strictly-greater-than collapses)', () => { + const twentyLines = new Array(20).fill('x').join('\n') + '\n'; + const info = evaluateCodeBlockCollapse(twentyLines, 20); + assert.strictEqual(info.lineCount, 20); + assert.strictEqual(info.collapse, false); + }); + + test('21 lines at a threshold of 20 collapses', () => { + const twentyOneLines = new Array(21).fill('x').join('\n') + '\n'; + const info = evaluateCodeBlockCollapse(twentyOneLines, 20); + assert.strictEqual(info.lineCount, 21); + assert.strictEqual(info.collapse, true); + }); + + test('a trailing newline does not count as an extra line ("a\\nb\\n" is 2 lines, not 3)', () => { + assert.strictEqual(evaluateCodeBlockCollapse('a\nb\n', 20).lineCount, 2); + }); + + test('CRLF line endings are normalized before counting ("a\\r\\nb\\r\\nc" is 3 lines)', () => { + assert.strictEqual(evaluateCodeBlockCollapse('a\r\nb\r\nc', 20).lineCount, 3); + }); + + test('an empty code block has lineCount 0 and never collapses', () => { + const info = evaluateCodeBlockCollapse('', 20); + assert.strictEqual(info.lineCount, 0); + assert.strictEqual(info.collapse, false); + }); + + test('a single very long line (5000 chars) does not collapse -- line-based only, not char-based', () => { + const info = evaluateCodeBlockCollapse('x'.repeat(5000), 20); + assert.strictEqual(info.lineCount, 1); + assert.strictEqual(info.collapse, false); + }); +}); + +suite('collapse-rules: normalizeCollapseThreshold (defaults/clamping)', () => { + + test('undefined falls back to the default of 20', () => { + assert.strictEqual(normalizeCollapseThreshold(undefined), 20); + }); + + test('null falls back to the default of 20', () => { + assert.strictEqual(normalizeCollapseThreshold(null), 20); + }); + + test('NaN falls back to the default of 20', () => { + assert.strictEqual(normalizeCollapseThreshold(NaN), 20); + }); + + test('a non-numeric string falls back to the default of 20', () => { + assert.strictEqual(normalizeCollapseThreshold('abc'), 20); + }); + + test('0 falls back to the default of 20', () => { + assert.strictEqual(normalizeCollapseThreshold(0), 20); + }); + + test('a negative number falls back to the default of 20', () => { + assert.strictEqual(normalizeCollapseThreshold(-5), 20); + }); + + test('a value below the minimum clamps up to 5', () => { + assert.strictEqual(normalizeCollapseThreshold(3), 5); + }); + + test('a value inside the valid range passes through unchanged', () => { + assert.strictEqual(normalizeCollapseThreshold(7), 7); + }); + + test('a decimal value inside the valid range is floored', () => { + assert.strictEqual(normalizeCollapseThreshold(12.7), 12); + }); + + test('a value above the maximum clamps down to 500', () => { + assert.strictEqual(normalizeCollapseThreshold(9999), 500); + }); +}); + +suite('collapse-rules: toString() splice sandbox', () => { + + test('both functions still work when spliced into a Function body with zero module context, matching the real call', () => { + const spliced = new Function( + normalizeCollapseThreshold.toString() + '\n' + evaluateCodeBlockCollapse.toString() + + '\nreturn evaluateCodeBlockCollapse(arguments[0], arguments[1]);'); + assert.deepStrictEqual(spliced('a\nb\nc\nd', 2), evaluateCodeBlockCollapse('a\nb\nc\nd', 2)); + }); +}); diff --git a/src/ui-styles.ts b/src/ui-styles.ts index 76bd766..1f56268 100644 --- a/src/ui-styles.ts +++ b/src/ui-styles.ts @@ -1141,6 +1141,28 @@ const styles = ` background-color: var(--vscode-list-hoverBackground); } + /* Manual per-message collapse (#48, upstream #151) */ + .message-collapse-btn { + background: transparent; + border: none; + color: var(--vscode-descriptionForeground); + cursor: pointer; + padding: 2px 4px; + border-radius: 3px; + font-size: 10px; + line-height: 1; + opacity: 0.35; + transition: opacity 0.2s ease; + } + .message:hover .message-collapse-btn { opacity: 0.8; } + .message-collapse-btn:hover { opacity: 1; background-color: var(--vscode-list-hoverBackground); } + /* Eingeklappt bleibt der Griff dauerhaft sichtbar, sonst findet ihn niemand wieder. */ + .message.collapsed .message-collapse-btn { opacity: 0.9; } + /* Alles ausser dem Header verbergen -- deckt .message-content UND Zusatzbloecke + wie .yolo-suggestion mit ab. */ + .message.collapsed > *:not(.message-header) { display: none; } + .message.collapsed .message-header { margin-bottom: 0; padding-bottom: 0; border-bottom: none; } + .message-icon { width: 18px; height: 18px; @@ -1262,6 +1284,31 @@ const styles = ` background: none; } + /* Collapsible long code blocks (#48, upstream #151). Die behaelt die + Klasse .code-block-header, damit alle Bestands- und Compact-Mode-Regeln + unveraendert greifen; nur Marker/Cursor/Flow kommen dazu. */ + details.code-block-container > summary.code-block-header { + cursor: pointer; + list-style: none; + user-select: none; + justify-content: flex-start; + gap: 6px; + } + details.code-block-container > summary.code-block-header::-webkit-details-marker { display: none; } + details.code-block-container > summary.code-block-header .code-copy-btn { margin-left: auto; } + .code-collapse-caret { + display: inline-block; + color: var(--vscode-descriptionForeground); + font-size: 9px; + transition: transform 0.15s ease; + } + details.code-block-container[open] > summary.code-block-header .code-collapse-caret { transform: rotate(90deg); } + .code-collapse-hint { + color: var(--vscode-descriptionForeground); + font-size: 10px; + opacity: 0.85; + } + /* Inline code */ .message-content code { background-color: var(--vscode-textCodeBlock-background); diff --git a/src/ui.ts b/src/ui.ts index 4ffcab7..d61b77e 100644 --- a/src/ui.ts +++ b/src/ui.ts @@ -414,6 +414,21 @@ const getHtml = (isTelemetryEnabled: boolean, opencreditsApiUrl: string = 'https

+

Appearance

+
+
+ + +
+
+ + +

+ Number of lines a code block must exceed before it gets a collapsible header (5-500). +

+
+
+ From 8b177cdf0684ef8f2f11b03fe9d2a109ebb5b237 Mon Sep 17 00:00:00 2001 From: Jonas Kunert Date: Sun, 26 Jul 2026 10:02:59 +0200 Subject: [PATCH 05/22] fix: escape tool input, command patterns and picker entries in the webview (fork-issue-49) Follow-up to fork-issue-40: the permission bubble, the tool-call input view, the prompt-snippet list and the file picker all interpolated model- or user-controlled text straight into innerHTML, so a Bash command like echo "hi" tore the permission bubble apart and a command containing a double quote broke out of the always-allow title attribute. escapeHtml() serialises through textContent/innerHTML and therefore leaves " and ' untouched, so attribute sinks need their own escaper: add escapeAttr in a self-contained html-escape.ts, spliced into the webview via .toString() like the collapse helper, and cover it with unit tests under plain mocha. Also drop the manual "/' decoding in toggleExpand -- getAttribute already returns decoded values, so that second pass destroyed any value containing entity text -- and render the TodoWrite summary as textContent, which never needed markup in the first place. Co-Authored-By: Claude Fable 5 --- package.json | 3 +- src/html-escape.ts | 16 +++++++ src/script.ts | 56 +++++++++++++----------- src/test/html-escape.test.ts | 85 ++++++++++++++++++++++++++++++++++++ 4 files changed, 134 insertions(+), 26 deletions(-) create mode 100644 src/html-escape.ts create mode 100644 src/test/html-escape.test.ts diff --git a/package.json b/package.json index e01ba2d..dde8613 100644 --- a/package.json +++ b/package.json @@ -234,7 +234,8 @@ "test:downloader": "npm run compile && mocha --ui tdd \"out/test/downloader*.test.js\" --reporter spec --timeout 360000", "test:downloader:unit": "npm run compile && mocha --ui tdd out/test/downloader.test.js --reporter spec", "test:models": "npm run compile && mocha --ui tdd out/test/model-updater.test.js --reporter spec", - "test:collapse-rules": "npm run compile && mocha --ui tdd out/test/collapse-rules.test.js --reporter spec" + "test:collapse-rules": "npm run compile && mocha --ui tdd out/test/collapse-rules.test.js --reporter spec", + "test:html-escape": "npm run compile && mocha --ui tdd out/test/html-escape.test.js --reporter spec" }, "devDependencies": { "@types/mocha": "^10.0.10", diff --git a/src/html-escape.ts b/src/html-escape.ts new file mode 100644 index 0000000..0873b9f --- /dev/null +++ b/src/html-escape.ts @@ -0,0 +1,16 @@ +// Attribut-Escaping fuer die Webview (#49). script.ts' escapeHtml() serialisiert ueber +// textContent->innerHTML und laesst " und ' deshalb STEHEN -- fuer title="..."/data-*="..." +// reicht das nicht (Attribut-Ausbruch). Diese Funktion wird NICHT hier aufgerufen: script.ts +// spleisst per .toString() nur ihren eigenen kompilierten Text in die Seite (Muster +// math-segments.ts/collapse-rules.ts). Deshalb MUSS sie self-contained bleiben -- kein +// Modul-Level-Symbol, kein Import, keine Hilfsfunktion ausserhalb des Bodys. +export function escapeAttr(value: unknown): string { + const s = value === null || value === undefined ? '' : String(value); + // '&' zwingend zuerst, sonst werden die eigenen Entities nachtraeglich zerlegt. + return s + .replace(/&/g, '&') + .replace(//g, '>') + .replace(/"/g, '"') + .replace(/'/g, '''); +} diff --git a/src/script.ts b/src/script.ts index ed1b3f7..7eb4fb9 100644 --- a/src/script.ts +++ b/src/script.ts @@ -1,6 +1,7 @@ import getSkillsScript from './skills-script'; import getPluginsScript from './plugins-script'; import getCollapseScript from './collapse-script'; +import { escapeAttr } from './html-escape'; const getScript = (isTelemetryEnabled: boolean, opencreditsApiUrl: string = 'https://ccc.api.opencredits.ai', opencreditsWebUrl: string = 'https://ccc.opencredits.ai', opencreditsPublishableKey: string = 'oc_pk_c43da4f9a9484ae484ad29bc97cc354f') => ` block'); + } + return match[1]; +} + +// Extracts one top-level "function NAME(...) { ... }" declaration's exact source text from +// the emitted script body via quote-aware brace matching, so formatFilePath/formatToolInputUI +// run exactly as emitted, never hand-copied from the TS source. +function extractFunction(source: string, name: string): string { + const sigMatch = new RegExp('function\\s+' + name + '\\s*\\(').exec(source); + if (!sigMatch) { + throw new Error('function ' + name + ' not found in emitted script'); + } + const braceStart = source.indexOf('{', sigMatch.index); + if (braceStart === -1) { + throw new Error('no opening brace found for function ' + name); + } + let depth = 0; + let inString: string | null = null; + for (let i = braceStart; i < source.length; i++) { + const ch = source[i]; + if (inString) { + if (ch === '\\') { i++; continue; } + if (ch === inString) { inString = null; } + continue; + } + if (ch === '\'' || ch === '"' || ch === '`') { inString = ch; continue; } + else if (ch === '{') { depth++; } + else if (ch === '}') { + depth--; + if (depth === 0) { return source.slice(sigMatch.index, i + 1); } + } + } + throw new Error('unbalanced braces while extracting function ' + name); +} + +// escapeHtml()'s only external dependency is document.createElement('div') (textContent set, +// innerHTML read). Per the HTML fragment serialisation spec, a Text node's innerHTML escapes +// only & < > (not " or ') -- real browsers behave the same; this is a faithful minimal stub. +class FakeDiv { + private _text = ''; + set textContent(v: string) { this._text = String(v); } + get innerHTML(): string { + return this._text.replace(/&/g, '&').replace(//g, '>'); + } +} + +interface Sandbox { + formatFilePath(filePath: string): string; + formatToolInputUI(input: unknown): string; +} + +function loadSandbox(): Sandbox { + const body = getEmittedScriptBody(); + const src = ['escapeHtml', 'escapeAttr', 'formatFilePath', 'formatToolInputUI'] + .map(name => extractFunction(body, name)) + .join('\n'); + const sandbox: Record = { + document: { + createElement(tag: string) { + if (tag !== 'div') { throw new Error('unexpected document.createElement(' + tag + ')'); } + return new FakeDiv(); + } + } + }; + vm.createContext(sandbox); + new vm.Script(src).runInContext(sandbox); + return sandbox as unknown as Sandbox; +} + +// First DFS match wins (document order) -- formatToolInputUI's output nests a +// span.file-path-truncated (from formatFilePath) inside a div.diff-file-path, and both +// currently carry a data-file-path attribute with the same value, so a "last match wins" +// walk would silently return the inner span's copy instead of the outer div's -- the one +// the div's own onclick="openFileInEditor(this.dataset.filePath)" actually reads. That would +// let a regression that drops data-file-path from the div alone (while leaving the span's +// copy intact) pass unnoticed. Use findAttrOn() below when a specific element matters. +function findAttr(html: string, attrName: string): { name: string; value: string } | undefined { + const frag = parse5.parseFragment(html); + let found: { name: string; value: string } | undefined; + (function walk(node: parse5.DefaultTreeAdapterMap['node']): void { + if (found) { return; } + const el = node as parse5.DefaultTreeAdapterMap['element']; + if (el.attrs) { + const a = el.attrs.find(x => x.name === attrName); + if (a) { found = a; return; } + } + const parent = node as parse5.DefaultTreeAdapterMap['parentNode']; + if (parent.childNodes) { parent.childNodes.forEach(walk); } + })(frag); + return found; +} + +// Scoped lookup: finds the first element carrying cssClass (document order) and returns +// attrName's value from THAT element specifically (undefined if the element lacks it) -- +// unlike findAttr(), this doesn't get confused by a same-named attribute on a nested element. +function findAttrOn(html: string, cssClass: string, attrName: string): { name: string; value: string } | undefined { + const frag = parse5.parseFragment(html); + let result: { name: string; value: string } | undefined; + let elementFound = false; + (function walk(node: parse5.DefaultTreeAdapterMap['node']): void { + if (elementFound) { return; } + const el = node as parse5.DefaultTreeAdapterMap['element']; + if (el.attrs) { + const classAttr = el.attrs.find(x => x.name === 'class'); + if (classAttr && classAttr.value.split(/\s+/).includes(cssClass)) { + elementFound = true; + result = el.attrs.find(x => x.name === attrName); + return; + } + } + const parent = node as parse5.DefaultTreeAdapterMap['parentNode']; + if (parent.childNodes) { parent.childNodes.forEach(walk); } + })(frag); + if (!elementFound) { + throw new Error('no element with class "' + cssClass + '" found in: ' + html); + } + return result; +} + +suite('webview attribute escaping: formatFilePath / formatToolInputUI (#57 PoC)', () => { + + test('an attribute-breakout file_path (a" onmouseover="alert(1)" zz=") produces no onmouseover attribute anywhere in the emitted element', () => { + const sandbox = loadSandbox(); + const payload = 'a" onmouseover="alert(1)" zz="'; + const html = sandbox.formatToolInputUI({ file_path: payload }); + assert.ok(!findAttr(html, 'onmouseover'), 'emitted HTML must not contain an onmouseover attribute; got: ' + html); + }); + + test('the same payload also stays contained when passed straight through formatFilePath (the #57 reference sink)', () => { + const sandbox = loadSandbox(); + const payload = 'a" onmouseover="alert(1)" zz="'; + const html = sandbox.formatFilePath(payload); + assert.ok(!findAttr(html, 'onmouseover'), 'emitted HTML must not contain an onmouseover attribute; got: ' + html); + }); + + test('the title and data-file-path attributes decode back to the exact raw payload (properly escaped, not mangled)', () => { + const sandbox = loadSandbox(); + const payload = 'a" onmouseover="alert(1)" zz="'; + const html = sandbox.formatFilePath(payload); + const title = findAttr(html, 'title'); + const dataFilePath = findAttr(html, 'data-file-path'); + assert.strictEqual(title && title.value, payload); + assert.strictEqual(dataFilePath && dataFilePath.value, payload); + }); + + test('a file path with an apostrophe (C:\\tmp\\it\'s\\a.txt) keeps the openFileInEditor handler syntactically intact', () => { + const sandbox = loadSandbox(); + const filePath = 'C:\\tmp\\it\'s\\a.txt'; + const html = sandbox.formatToolInputUI({ file_path: filePath }); + const onclick = findAttr(html, 'onclick'); + assert.ok(onclick, 'expected an onclick attribute on the emitted element; got: ' + html); + // The path is never embedded as a JS string literal in the handler (which an + // apostrophe would break) -- it's read from the element's own dataset instead, so the + // handler text itself is a fixed, path-independent string. + assert.strictEqual(onclick!.value, 'openFileInEditor(this.dataset.filePath)'); + }); + + test('data-file-path round-trips the exact original apostrophe path on the div that actually owns the onclick handler (matching how el.dataset.filePath reads it)', () => { + const sandbox = loadSandbox(); + const filePath = 'C:\\tmp\\it\'s\\a.txt'; + const html = sandbox.formatToolInputUI({ file_path: filePath }); + // findAttrOn, not findAttr: formatFilePath's inner span also carries a data-file-path + // copy, but this.dataset.filePath in the onclick handler reads it from the outer + // div.diff-file-path specifically -- that's the element that must be checked. + const dataFilePath = findAttrOn(html, 'diff-file-path', 'data-file-path'); + assert.ok(dataFilePath, 'expected a data-file-path attribute on div.diff-file-path; got: ' + html); + assert.strictEqual(dataFilePath!.value, filePath); + }); + + test('a plain file path with no special characters still renders visibly (no functional regression)', () => { + const sandbox = loadSandbox(); + const html = sandbox.formatToolInputUI({ file_path: '/home/user/project/index.ts' }); + assert.ok(html.includes('index.ts'), 'expected the file name to appear in the rendered output; got: ' + html); + const dataFilePath = findAttrOn(html, 'diff-file-path', 'data-file-path'); + assert.strictEqual(dataFilePath && dataFilePath.value, '/home/user/project/index.ts'); + }); +}); From e2b7f2a09ff3203f922b947a0aca01675403a3db Mon Sep 17 00:00:00 2001 From: Jonas Kunert Date: Mon, 27 Jul 2026 19:05:18 +0200 Subject: [PATCH 09/22] fix: never report YOLO mode as enabled unless it was persisted (fork-issue-59) _enableYoloMode wrote permissions.yoloMode with ConfigurationTarget.Workspace only, without the global fallback _updateSettings has for the same key. In a window with no workspace folder open, config.update() threw, the bare catch swallowed it and _sendCurrentSettings() never ran -- the setting was silently not persisted. The chat said "YOLO Mode enabled!" anyway, because that confirmation fired client-side the moment the button was clicked and never depended on a response from the extension host. That was the actual defect: the fallback alone would still have left the message unconditional. The workspace-then-global decision now lives in one pure, vscode-free function (settings-batch.ts), shared with _updateSettings' handling of the same key, and _enableYoloMode reports success only after the write went through -- otherwise _reportYoloModeEnableFailure logs the error, shows a VS Code error message and sends a yoloModeEnableFailed response. An outer try/catch makes that hold even for failures outside the update itself, e.g. a mismatched deploy where settings-batch.js is stale: previously that would have been an unhandled rejection with no feedback at all. Also dropped the module comment's "same pattern as shell-utils/perm-log-redact" comparison -- neither module exists in this branch (shell-utils.ts belongs to the separate MCP-cluster bundle; perm-log-redact.ts was removed here as a fork-issue-51 fix with no caller in this branch, see that commit's absence in this history). Co-Authored-By: Claude Opus 5 --- src/extension.ts | 68 +++++++++++++++++++++------- src/script.ts | 62 ++++++++++++++++++++----- src/settings-batch.ts | 80 +++++++++++++++++++++++++++++++++ src/test/settings-batch.test.ts | 67 +++++++++++++++++++++++++++ 4 files changed, 252 insertions(+), 25 deletions(-) create mode 100644 src/settings-batch.ts create mode 100644 src/test/settings-batch.test.ts diff --git a/src/extension.ts b/src/extension.ts index 1431652..cfa95fb 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -8,6 +8,7 @@ import { startRouter, stopRouter, setModelConfig, setBaseUrl } from './router'; import { fetchAndResolveModels } from './model-updater'; import recommendedModels from './recommended-models.json'; import { downloadClaude, detectPlatform, DownloaderError } from './claudeDownloader'; +import { updateWithWorkspaceThenGlobalFallback } from './settings-batch'; // OpenCredits environment configuration let OPENCREDITS_API_URL = 'https://ccc.api.opencredits.ai'; @@ -3411,23 +3412,54 @@ class ClaudeChatProvider { }); } + // #59: workspace-then-global fallback (same pattern _updateSettings already used for + // this key, via updateWithWorkspaceThenGlobalFallback). Before #59 this only tried + // Workspace and swallowed the error into the console -- in a window with no + // workspace folder open that meant YOLO mode was never actually persisted, while the + // webview's "YOLO Mode enabled!" chat message (script.ts's enableYoloMode()) fired + // unconditionally client-side, independent of any response from here. That message + // is now gated on the 'yoloModeEnabled' reply below, sent only after a successful + // write; a double failure gets a 'yoloModeEnableFailed' reply plus a native error + // notification instead, and never a success confirmation. + // + // opus-review follow-up: the outer try/catch below exists because this method is + // called fire-and-forget (extension.ts's message handler does + // `this._enableYoloMode();`, no `await`/`.catch`, see the switch above). Before this + // review pass, only the two config.update() calls inside + // updateWithWorkspaceThenGlobalFallback could reject; now that this method's own + // logic (e.g. a settings-batch.ts that's out of sync with extension.ts after a + // partial deploy, so updateWithWorkspaceThenGlobalFallback itself is undefined) can + // also throw, an uncaught rejection here would silently swallow the click with none + // of #59's reporting -- exactly the failure class #59 exists to close. private async _enableYoloMode(): Promise { try { - // Update VS Code configuration to enable YOLO mode const config = vscode.workspace.getConfiguration('claudeCodeChat'); + const result = await updateWithWorkspaceThenGlobalFallback( + async () => { await config.update('permissions.yoloMode', true, vscode.ConfigurationTarget.Workspace); }, + async () => { await config.update('permissions.yoloMode', true, vscode.ConfigurationTarget.Global); } + ); - // Clear any global setting and set workspace setting - await config.update('permissions.yoloMode', true, vscode.ConfigurationTarget.Workspace); - - - // Send updated settings to UI - this._sendCurrentSettings(); - - } catch (error) { - console.error('Error enabling YOLO mode:', error); + if (result.succeeded) { + // Send updated settings to UI + this._sendCurrentSettings(); + this._postMessage({ type: 'yoloModeEnabled' }); + } else { + this._reportYoloModeEnableFailure(result.globalError || result.workspaceError || 'Unknown error'); + } + } catch (error: any) { + this._reportYoloModeEnableFailure(error?.message || String(error)); } } + // Shared by _enableYoloMode's double-failure path and its outer catch: same + // treatment either way -- a caller must never see a silent no-op where the chat + // already claimed success. + private _reportYoloModeEnableFailure(message: string): void { + console.error('Error enabling YOLO mode:', message); + vscode.window.showErrorMessage(`Failed to enable YOLO mode: ${message}`); + this._postMessage({ type: 'yoloModeEnableFailed', error: message }); + } + private _saveInputText(text: string): void { this._draftMessage = text || ''; } @@ -3438,11 +3470,17 @@ class ClaudeChatProvider { try { for (const [key, value] of Object.entries(settings)) { if (key === 'permissions.yoloMode') { - // YOLO mode: try workspace first, fall back to global - try { - await config.update(key, value, vscode.ConfigurationTarget.Workspace); - } catch { - await config.update(key, value, vscode.ConfigurationTarget.Global); + // #59: YOLO mode: try workspace first, fall back to global (same + // helper _enableYoloMode uses). A double failure throws here, same as + // before, so applySettingsBatch's own per-key try/catch still records + // it as a failure -- no behavior change, just one shared + // implementation instead of two copies of this fallback. + const yoloResult = await updateWithWorkspaceThenGlobalFallback( + async () => { await config.update(key, value, vscode.ConfigurationTarget.Workspace); }, + async () => { await config.update(key, value, vscode.ConfigurationTarget.Global); } + ); + if (!yoloResult.succeeded) { + throw new Error(yoloResult.globalError || yoloResult.workspaceError || 'Unknown error'); } } else { // Other settings are global (user-wide) diff --git a/src/script.ts b/src/script.ts index 4fa000d..908b15d 100644 --- a/src/script.ts +++ b/src/script.ts @@ -89,6 +89,14 @@ const getScript = (isTelemetryEnabled: boolean, opencreditsApiUrl: string = 'htt let collapseLongCodeBlocks = true; let collapseCodeBlockLines = 20; let attachedImages = []; // Array of { filePath, previewUri } + // #59: text for the next 'yoloModeEnabled' response's chat message, set by + // enableYoloMode() right before it posts 'enableYoloMode' to the extension host. + // The two call sites (inline permission-menu item vs. the standalone chat + // button) use different wording, so this can't be a hardcoded string in the + // 'yoloModeEnabled' case itself; only one enable request is ever in flight at a + // time, so a single module-level slot is enough (same pragmatic pattern as + // sendOnEnter/renderMathEnabled above). + let pendingYoloEnableMessage = null; // Open diff using stored data (no file read needed) function openDiffEditor() { @@ -3656,6 +3664,32 @@ const getScript = (isTelemetryEnabled: boolean, opencreditsApiUrl: string = 'htt updateStatusWithTotals(); break; + case 'yoloModeEnabled': + // #59: confirmation only arrives here after the extension host + // actually persisted permissions.yoloMode (workspace, or global as + // fallback) -- the chat message moved here (out of enableYoloMode()) + // so it can no longer fire before/regardless of that write. + addMessage(pendingYoloEnableMessage || '✅ Yolo Mode enabled! All permission checks will be bypassed for future commands.', 'system'); + pendingYoloEnableMessage = null; + break; + + case 'yoloModeEnableFailed': + // #59: both the workspace and global config.update() attempts threw + // -- surface it in-chat too, not just via the extension host's native + // error notification, and never show the "enabled" message. The raw + // host error (message.error) deliberately does NOT go into this + // 'error'-typed content: a real-world double failure (e.g. settings.json + // not writable) throws "EACCES: permission denied, open '...'", which + // isPermissionError() would match, attaching a circular "Enable Yolo + // Mode" suggestion button to the very message reporting that enabling + // it just failed. The full text is still in the console (logged below) + // and in the native showErrorMessage notification the extension host + // already sent. + console.error('Failed to enable YOLO mode:', message.error); + addMessage('❌ Failed to enable YOLO mode. See the notification for details.', 'error'); + pendingYoloEnableMessage = null; + break; + case 'toolUse': if (typeof message.data === 'object') { addToolUseMessage(message.data); @@ -4150,37 +4184,45 @@ const getScript = (isTelemetryEnabled: boolean, opencreditsApiUrl: string = 'htt // inline button. The extension host's _enableYoloMode() persists // permissions.yoloMode and replies with settingsData, whose handler // (~6147/~6150) sets the checkbox and calls updateYoloWarning(). + // + // #59: the "enabled" chat message used to fire right here, unconditionally, + // the moment the button was clicked -- independent of whether + // _enableYoloMode() on the extension host actually managed to persist + // anything (it had no global fallback, so with no workspace folder open the + // setting silently never got saved while the chat still said "enabled"). + // That confirmation now only happens in the 'yoloModeEnabled' case above, + // once the extension host reports success; pendingYoloEnableMessage carries + // the wording across that round trip. A 'yoloModeEnableFailed' reply shows + // an in-chat error instead and never the "enabled" text. function enableYoloMode(permissionId) { sendStats('YOLO mode enabled'); - + if (permissionId) { // Hide the menu const menu = document.getElementById(\`permissionMenu-\${permissionId}\`); if (menu) { menu.style.display = 'none'; } - + + pendingYoloEnableMessage = '⚡ YOLO Mode enabled! All future permissions will be automatically allowed.'; + // Send message to enable YOLO mode vscode.postMessage({ type: 'enableYoloMode' }); - + // Auto-approve this permission respondToPermission(permissionId, true); - - // Show notification - addMessage('⚡ YOLO Mode enabled! All future permissions will be automatically allowed.', 'system'); return; } - + + pendingYoloEnableMessage = '✅ Yolo Mode enabled! All permission checks will be bypassed for future commands.'; + // Send message to enable YOLO mode (settingsData round trip updates // the checkbox + yolo warning banner, see comment above) vscode.postMessage({ type: 'enableYoloMode' }); - - // Show confirmation message - addMessage('✅ Yolo Mode enabled! All permission checks will be bypassed for future commands.', 'system'); } // Close permission menus when clicking outside diff --git a/src/settings-batch.ts b/src/settings-batch.ts new file mode 100644 index 0000000..8076e36 --- /dev/null +++ b/src/settings-batch.ts @@ -0,0 +1,80 @@ +// #59: permissions.yoloMode's workspace-then-global fallback, pulled into its own pure +// function so both _enableYoloMode (the inline "Enable Yolo Mode" chat button / menu +// item, #52) and _updateSettings's existing per-key handling of the same setting key +// can share one implementation instead of keeping two copies of the same try/catch in +// extension.ts. No vscode import here, so this runs under plain mocha. Run with +// `npm run test:settings-batch`. +// +// Note: upstream (fork) also has an applySettingsBatch helper in this module for a +// separate, unrelated fix (#56, whole-settings-batch loop resilience) that is not part +// of this security-hardening branch and is intentionally not included here. + +// Turns whatever a rejected update callback threw into a plain string, the same way the +// pre-#59 code's 'err=' + (error?.message || error) string-concatenation did (Error +// instances and message-bearing objects use .message; anything else -- a thrown string, +// undefined, a plain object -- coerces the same way String() / template-literal +// interpolation would), so a caller like extension.ts's _permLog never has to guard +// against a missing .message itself. +export function toErrorMessage(error: unknown): string { + if (typeof error === 'object' && error !== null && 'message' in error) { + const message = (error as { message: unknown }).message; + if (typeof message === 'string' && message) { + return message; + } + } + if (typeof error === 'string') { + return error; + } + return String(error); +} + +export interface WorkspaceThenGlobalFallbackResult { + succeeded: boolean; + // Which scope actually ended up holding the value on success, or which scope's + // error is the relevant "final" one when both attempts failed. + scope: 'workspace' | 'global'; + // Set whenever the workspace attempt was made and threw -- present both when the + // global fallback then succeeded (diagnostic-only) and when it also failed. + workspaceError?: string; + // Set only when the global fallback attempt itself threw. + globalError?: string; +} + +// #59: permissions.yoloMode's workspace-then-global fallback, pulled into its own pure +// function so both _enableYoloMode (the inline "Enable Yolo Mode" chat button / menu +// item, #52) and _updateSettings's existing per-key handling of the same setting key +// can share one implementation instead of keeping two copies of the same try/catch in +// extension.ts. extension.ts still owns the actual vscode.workspace config.update() +// calls via the injected callbacks. +// +// Before #59, _enableYoloMode had no global fallback at all and its bare catch only +// logged to the extension host's console -- in a window with no workspace folder open, +// config.update(..., Workspace) throws, _sendCurrentSettings() (and therefore the +// webview) never found out, and the setting was silently never persisted. The chat +// already said "YOLO Mode enabled!" regardless, because that confirmation was fired +// client-side the moment the button was clicked, not gated on any response from the +// extension host. This function's `succeeded` flag exists specifically so a caller can +// tell "both attempts threw" apart from "it worked" and refuse to report success in +// that case. +export async function updateWithWorkspaceThenGlobalFallback( + updateWorkspace: () => Promise, + updateGlobal: () => Promise +): Promise { + try { + await updateWorkspace(); + return { succeeded: true, scope: 'workspace' }; + } catch (workspaceError) { + const workspaceMessage = toErrorMessage(workspaceError); + try { + await updateGlobal(); + return { succeeded: true, scope: 'global', workspaceError: workspaceMessage }; + } catch (globalError) { + return { + succeeded: false, + scope: 'global', + workspaceError: workspaceMessage, + globalError: toErrorMessage(globalError) + }; + } + } +} diff --git a/src/test/settings-batch.test.ts b/src/test/settings-batch.test.ts new file mode 100644 index 0000000..ecff956 --- /dev/null +++ b/src/test/settings-batch.test.ts @@ -0,0 +1,67 @@ +// Unit tests for the #59 updateWithWorkspaceThenGlobalFallback fix. Pure (no vscode, no +// network, no filesystem access), so these run under plain mocha against the compiled +// out/ output. Run with `npm run test:settings-batch`. +// +// Note: upstream (fork) also has applySettingsBatch tests in this file for a separate, +// unrelated fix (#56, whole-settings-batch loop resilience) that is not part of this +// security-hardening branch and is intentionally not included here. + +import * as assert from 'assert'; +import { updateWithWorkspaceThenGlobalFallback } from '../settings-batch'; + +// #59: updateWithWorkspaceThenGlobalFallback -- the pure decision logic behind +// permissions.yoloMode's workspace-then-global fallback, shared by _enableYoloMode and +// _updateSettings. Before #59, _enableYoloMode had no fallback at all (a Workspace-scope +// write failing, e.g. no workspace folder open, meant the setting was silently never +// persisted while the webview still showed "YOLO Mode enabled!"); these tests pin the +// three outcomes a caller needs to distinguish: workspace succeeds, workspace fails but +// global saves it, and both fail (the case that must never be reported as success). +suite('settings-batch: updateWithWorkspaceThenGlobalFallback (workspace succeeds)', () => { + + test('the workspace callback is used and global is never attempted', async () => { + let globalCalls = 0; + const result = await updateWithWorkspaceThenGlobalFallback( + async () => { /* succeeds */ }, + async () => { globalCalls++; } + ); + assert.deepStrictEqual(result, { succeeded: true, scope: 'workspace' }); + assert.strictEqual(globalCalls, 0, 'global must not be attempted when workspace already succeeded'); + }); +}); + +suite('settings-batch: updateWithWorkspaceThenGlobalFallback (workspace throws, global fallback saves it)', () => { + + test('a workspace failure -- e.g. no workspace folder open -- falls back to global and still reports success', async () => { + const result = await updateWithWorkspaceThenGlobalFallback( + () => Promise.reject(new Error('Unable to write to Workspace Settings because no workspace is opened')), + async () => { /* global succeeds */ } + ); + assert.strictEqual(result.succeeded, true, '#59: this is exactly the case the old _enableYoloMode (no fallback at all) silently lost'); + assert.strictEqual(result.scope, 'global'); + assert.strictEqual(result.workspaceError, 'Unable to write to Workspace Settings because no workspace is opened'); + }); +}); + +suite('settings-batch: updateWithWorkspaceThenGlobalFallback (both attempts throw)', () => { + + test('a double failure is reported as NOT succeeded, with both error messages preserved -- never treated as success', async () => { + const result = await updateWithWorkspaceThenGlobalFallback( + () => Promise.reject(new Error('workspace boom')), + () => Promise.reject(new Error('global boom')) + ); + assert.strictEqual(result.succeeded, false, '#59: a caller (e.g. the webview YOLO Mode enabled! message) must never treat this as success'); + assert.strictEqual(result.scope, 'global'); + assert.strictEqual(result.workspaceError, 'workspace boom'); + assert.strictEqual(result.globalError, 'global boom'); + }); + + test('non-Error rejections are still normalized to strings on both sides', async () => { + const result = await updateWithWorkspaceThenGlobalFallback( + () => Promise.reject('plain workspace error'), + () => Promise.reject(undefined) + ); + assert.strictEqual(result.succeeded, false); + assert.strictEqual(result.workspaceError, 'plain workspace error'); + assert.strictEqual(result.globalError, 'undefined'); + }); +}); From 0aea6101520da3357c19d8f6da51f15b02ff992b Mon Sep 17 00:00:00 2001 From: Jonas Kunert Date: Mon, 27 Jul 2026 19:41:53 +0200 Subject: [PATCH 10/22] fix: keep the MCP server name out of inline handlers (fork-issue-60) displayMCPServers interpolated the server name straight into a JS string literal inside an HTML attribute -- onclick="editMCPServer('${name}', ...)" -- with no escaping at all, and passed the whole config object alongside it as JSON.stringify(...).replace(/"/g,'"'). A single quote in the name broke out of the call; a double quote broke out of the attribute entirely. Server names can come from a cloned workspace's own .mcp.json (extension.ts:2740, read in at extension.ts:2770), and the webview CSP is default-src * 'unsafe-inline' 'unsafe-eval'. The config object can never be escapeAttr'd as an attribute value, so it no longer touches an attribute: displayMCPServers records it in a per-render map and editMCPServer(name) looks it up. Name and scope travel through data-server-name / data-server-scope with escapeAttr plus this.dataset.*, and serverType is escapeHtml'd before it reaches innerHTML. Nine tests (src/test/webview-attr-escape.test.ts) cover both payload classes -- x'); alert(1); (' does not break out of the attribute but is valid multi-statement JS in the handler, while x" onmouseover="alert(1)" y=" injects a real attribute -- all verified to fail against the previous code. extractFunction now skips comments: an apostrophe in an existing comment inside editMCPServer desynchronised its brace matching. Co-Authored-By: Claude Opus 5 --- src/script.ts | 33 +++- src/test/webview-attr-escape.test.ts | 245 +++++++++++++++++++++++++++ 2 files changed, 272 insertions(+), 6 deletions(-) diff --git a/src/script.ts b/src/script.ts index 908b15d..c7cd882 100644 --- a/src/script.ts +++ b/src/script.ts @@ -1628,8 +1628,18 @@ const getScript = (isTelemetryEnabled: boolean, opencreditsApiUrl: string = 'htt } let editingServerName = null; - - function editMCPServer(name, config) { + // #60: configs keyed by server name -- editMCPServer used to receive the whole config + // object JSON.stringify()'d straight into an onclick(...) attribute (a second, + // un-escapeAttr-able sink alongside the name itself). The object now stays in JS-land; + // only the (escapeAttr'd) name crosses into the attribute. Reset on every + // displayMCPServers() render. + let mcpServerConfigsByName = {}; + + function editMCPServer(name) { + const config = mcpServerConfigsByName[name]; + if (!config) { + return; + } editingServerName = name; // Hide add button and popular servers @@ -2071,6 +2081,9 @@ const getScript = (isTelemetryEnabled: boolean, opencreditsApiUrl: string = 'htt function displayMCPServers(servers) { const serversList = document.getElementById('mcpServersList'); serversList.innerHTML = ''; + // #60: reset per render so editMCPServer can never resolve a stale/removed server's + // config through a name that no longer has a corresponding button. + mcpServerConfigsByName = {}; if (Object.keys(servers).length === 0) { serversList.innerHTML = '
' + @@ -2107,16 +2120,24 @@ const getScript = (isTelemetryEnabled: boolean, opencreditsApiUrl: string = 'htt } const scopeLabel = serverScope === 'global' ? 'Global' : serverScope === 'project' ? 'Project' : 'Extension'; + // #60: name/config moved out of the inline onclick -- a name containing a single + // quote used to break straight out of editMCPServer('...') into the attribute, + // and JSON.stringify(config) was a second, un-escapeAttr-able sink. The config + // now lives only in mcpServerConfigsByName; editMCPServer looks it up by name, + // which itself travels through data-server-name (escapeAttr) + this.dataset, + // same pattern as #57/#58. + mcpServerConfigsByName[name] = config; + const serverActionsHtml = \` + \`; serverItem.innerHTML = \`
-
\${name} \${scopeLabel}
-
\${serverType.toUpperCase()}
+
\${escapeHtml(name)} \${scopeLabel}
+
\${escapeHtml(serverType.toUpperCase())}
\${configDisplay}
- - + \${serverActionsHtml}
\`; diff --git a/src/test/webview-attr-escape.test.ts b/src/test/webview-attr-escape.test.ts index 138c10d..3e087b2 100644 --- a/src/test/webview-attr-escape.test.ts +++ b/src/test/webview-attr-escape.test.ts @@ -51,6 +51,21 @@ function extractFunction(source: string, name: string): string { if (ch === inString) { inString = null; } continue; } + // #60: comments can contain an unbalanced quote (e.g. editMCPServer's pre-existing + // "// Don't allow name changes when editing") that would otherwise be misread as a + // string start, desyncing the brace count for everything after it and pulling in + // unrelated trailing functions (found via editMCPServer, which no earlier suite ever + // extracted). Skip comment contents entirely, same as a real JS tokenizer would. + if (ch === '/' && source[i + 1] === '/') { + const nl = source.indexOf('\n', i); + i = nl === -1 ? source.length : nl; + continue; + } + if (ch === '/' && source[i + 1] === '*') { + const end = source.indexOf('*/', i + 2); + i = end === -1 ? source.length : end + 1; + continue; + } if (ch === '\'' || ch === '"' || ch === '`') { inString = ch; continue; } else if (ch === '{') { depth++; } else if (ch === '}') { @@ -203,3 +218,233 @@ suite('webview attribute escaping: formatFilePath / formatToolInputUI (#57 PoC)' assert.strictEqual(dataFilePath && dataFilePath.value, '/home/user/project/index.ts'); }); }); + +// ───────────────────────────────────────────────────────────────────────── +// #60 PoC: displayMCPServers() built the Edit/Delete buttons' onclick handlers by +// interpolating the raw server name (and, for Edit, JSON.stringify(config)) directly into a +// JS string literal inside an HTML attribute -- with NO escaping at all (not even the #57-era +// escapeHtml() mistake). A server name of x'); alert(1); (' from a workspace's own .mcp.json +// (loaded with _scope: 'project', see extension.ts) broke straight out of +// editMCPServer('...', ...) and ran arbitrary JS in a CSP of default-src * 'unsafe-inline' +// 'unsafe-eval'. Fix: name (and scope, for Delete) move into data-* (escapeAttr) + +// this.dataset.*, same pattern as #57/#58. The config object itself can never be +// escapeAttr()'d sanely as a JS-object-literal attribute value, so it no longer touches an +// attribute at all -- it's kept in a webview-scope map (mcpServerConfigsByName) and +// editMCPServer looks it up by name. +// ───────────────────────────────────────────────────────────────────────── + +// Extracts a top-level "let NAME = ...;" declaration's exact source text (used for +// mcpServerConfigsByName/editingServerName, which extractFunction() can't grab -- they're not +// function declarations). +function extractDeclaration(source: string, name: string): string { + const match = new RegExp('let\\s+' + name + '\\s*=\\s*[^;]+;').exec(source); + if (!match) { + throw new Error('declaration for ' + name + ' not found in emitted script'); + } + return match[0]; +} + +// Concatenates the direct #text children of the first element carrying cssClass (document +// order); throws if no such element exists. If escaping were missing, a payload like +// '' would parse as a real element instead of literal text, +// so those characters would be MISSING from this concatenation -- the full raw payload +// round-tripping back as text is what proves the escaping worked. +function textOn(html: string, cssClass: string): string { + const frag = parse5.parseFragment(html); + let result: string | undefined; + let elementFound = false; + (function walk(node: parse5.DefaultTreeAdapterMap['node']): void { + if (elementFound) { return; } + const el = node as parse5.DefaultTreeAdapterMap['element']; + if (el.attrs) { + const classAttr = el.attrs.find(x => x.name === 'class'); + if (classAttr && classAttr.value.split(/\s+/).includes(cssClass)) { + elementFound = true; + const parent = node as parse5.DefaultTreeAdapterMap['parentNode']; + result = (parent.childNodes || []) + .filter(n => n.nodeName === '#text') + .map(n => (n as parse5.DefaultTreeAdapterMap['textNode']).value) + .join(''); + return; + } + } + const parent = node as parse5.DefaultTreeAdapterMap['parentNode']; + if (parent.childNodes) { parent.childNodes.forEach(walk); } + })(frag); + if (!elementFound) { + throw new Error('no element with class "' + cssClass + '" found in: ' + html); + } + return result || ''; +} + +// Minimal DOM stand-in for displayMCPServers/editMCPServer/updateServerForm. All three only +// ever call getElementById(id).{value,disabled,textContent,style.display,innerHTML,className}, +// document.createElement('div'), element.appendChild(child), element.insertAdjacentHTML(...) +// (cosmetic only -- edit-form h5 title, never security-relevant), and document.querySelector( +// ...) (also only cosmetic lookups in editMCPServer). A single auto-vivifying registry keyed +// by id lets assertions read back e.g. elements populated by editMCPServer after the call. +class FakeElement { + className = ''; + value = ''; + disabled = false; + style: Record = {}; + children: FakeElement[] = []; + private _innerHTML = ''; + // Mirrors FakeDiv above (escapeHtml()'s only external dependency): setting textContent + // replaces the content with an HTML-escaped text serialization, exactly what escapeHtml() + // reads back via innerHTML -- without this, escapeHtml() (used for .server-name/.server-type + // text) silently returns '' for every input inside this sandbox. displayMCPServers/ + // editMCPServer, by contrast, only ever WRITE innerHTML directly (a full template string) + // and never read it back through textContent, so a plain passthrough setter is enough there. + set textContent(v: string) { + this._innerHTML = String(v).replace(/&/g, '&').replace(//g, '>'); + } + get innerHTML(): string { return this._innerHTML; } + set innerHTML(v: string) { this._innerHTML = v; } + appendChild(child: FakeElement): FakeElement { this.children.push(child); return child; } + insertAdjacentHTML(): void { /* cosmetic only */ } +} + +class FakeDocument { + elementsById = new Map(); + getElementById(id: string): FakeElement { + let el = this.elementsById.get(id); + if (!el) { el = new FakeElement(); this.elementsById.set(id, el); } + return el; + } + createElement(tag: string): FakeElement { + if (tag !== 'div') { throw new Error('unexpected document.createElement(' + tag + ')'); } + return new FakeElement(); + } + querySelector(): FakeElement { + return new FakeElement(); + } +} + +interface McpSandbox { + displayMCPServers(servers: Record): void; + editMCPServer(name: string): void; + // Test-only inspection shim -- top-level "let" bindings (editingServerName, + // mcpServerConfigsByName) live in the vm Script's lexical scope, not as properties on the + // sandbox/global object, so they aren't readable from outside via plain property access + // (only top-level function/var declarations become global-object properties -- see + // loadSandbox() above, which already relies on that half of the same rule). __testState is + // never part of the real script.ts output; it only exists to expose those two "let"s here. + __testState(): { editingServerName: string | null; configsByName: Record }; +} + +function loadMcpSandbox(): { sandbox: McpSandbox; document: FakeDocument } { + const body = getEmittedScriptBody(); + const document = new FakeDocument(); + const src = [ + extractDeclaration(body, 'editingServerName'), + extractDeclaration(body, 'mcpServerConfigsByName'), + extractFunction(body, 'escapeHtml'), + extractFunction(body, 'escapeAttr'), + extractFunction(body, 'updateServerForm'), + extractFunction(body, 'displayMCPServers'), + extractFunction(body, 'editMCPServer'), + 'function __testState() { return { editingServerName: editingServerName, configsByName: mcpServerConfigsByName }; }', + ].join('\n'); + const sandbox: Record = { document }; + vm.createContext(sandbox); + new vm.Script(src).runInContext(sandbox); + return { sandbox: sandbox as unknown as McpSandbox, document }; +} + +// Renders `servers` and returns the .mcp-server-item div's innerHTML for the given server name +// plus the sandbox/document for further assertions (mcpServerConfigsByName via __testState(), +// or calling editMCPServer(...) next). +function renderServerItem(servers: Record, name: string): { html: string; sandbox: McpSandbox; document: FakeDocument } { + const { sandbox, document } = loadMcpSandbox(); + sandbox.displayMCPServers(servers); + const serversList = document.getElementById('mcpServersList'); + const index = Object.keys(servers).indexOf(name); + const item = serversList.children[index]; + if (!item) { + throw new Error('displayMCPServers did not append an item for "' + name + '"; got ' + serversList.children.length + ' item(s)'); + } + return { html: item.innerHTML, sandbox, document }; +} + +suite('webview attribute escaping: displayMCPServers / editMCPServer (#60 PoC)', () => { + + test('an attribute-breakout server name (x\'); alert(1); (\') cannot inject extra JS statements -- the onclick text is always the fixed, name-independent dataset call', () => { + const payload = 'x\'); alert(1); (\''; + const { html } = renderServerItem({ [payload]: { type: 'stdio', command: 'echo' } }, payload); + const onclicks = [...html.matchAll(/onclick="([^"]*)"/g)].map(m => m[1]); + assert.ok(onclicks.length > 0, 'expected at least one onclick attribute; got: ' + html); + for (const oc of onclicks) { + assert.ok( + oc === 'editMCPServer(this.dataset.serverName)' || oc === 'deleteMCPServer(this.dataset.serverName, this.dataset.serverScope)', + 'onclick handler must be exactly the fixed dataset-based call, got: ' + oc + ); + } + }); + + test('a server name with a double quote (x" onmouseover=...) stays contained -- no extra attribute breaks out', () => { + const payload = 'x" onmouseover="alert(1)" y="'; + const { html } = renderServerItem({ [payload]: { type: 'stdio', command: 'echo' } }, payload); + assert.ok(!findAttr(html, 'onmouseover'), 'must not contain an onmouseover attribute; got: ' + html); + }); + + test('the Edit button\'s onclick is exactly editMCPServer(this.dataset.serverName), independent of the name', () => { + const payload = 'x\'); alert(1); (\''; + const { html } = renderServerItem({ [payload]: { type: 'stdio', command: 'echo' } }, payload); + const onclick = findAttrOn(html, 'server-edit-btn', 'onclick'); + assert.ok(onclick, 'expected an onclick attribute on .server-edit-btn; got: ' + html); + assert.strictEqual(onclick!.value, 'editMCPServer(this.dataset.serverName)'); + }); + + test('the Delete button\'s onclick is exactly deleteMCPServer(this.dataset.serverName, this.dataset.serverScope), independent of the name', () => { + const payload = 'x\'); alert(1); (\''; + const { html } = renderServerItem({ [payload]: { type: 'stdio', command: 'echo' } }, payload); + const onclick = findAttrOn(html, 'server-delete-btn', 'onclick'); + assert.ok(onclick, 'expected an onclick attribute on .server-delete-btn; got: ' + html); + assert.strictEqual(onclick!.value, 'deleteMCPServer(this.dataset.serverName, this.dataset.serverScope)'); + }); + + test('data-server-name on both buttons round-trips the exact raw payload', () => { + const payload = 'x\'); alert(1); (\''; + const { html } = renderServerItem({ [payload]: { type: 'stdio', command: 'echo' } }, payload); + const editName = findAttrOn(html, 'server-edit-btn', 'data-server-name'); + const deleteName = findAttrOn(html, 'server-delete-btn', 'data-server-name'); + assert.strictEqual(editName && editName.value, payload); + assert.strictEqual(deleteName && deleteName.value, payload); + }); + + test('mcpServerConfigsByName holds the exact original config object after rendering (replaces the old JSON.stringify-in-attribute round-trip)', () => { + const payload = 'x\'); alert(1); (\''; + const config = { type: 'stdio', command: 'echo', args: ['a', 'b'] }; + const { sandbox } = renderServerItem({ [payload]: config }, payload); + assert.strictEqual(sandbox.__testState().configsByName[payload], config); + }); + + test('editMCPServer(name), called with the exact string the rendered data-server-name attribute decodes to, looks up the right config and does not throw', () => { + const payload = 'x\'); alert(1); (\''; + const config = { type: 'http', url: 'https://example.com/mcp' }; + const { html, sandbox, document } = renderServerItem({ [payload]: config }, payload); + const editName = findAttrOn(html, 'server-edit-btn', 'data-server-name'); + assert.strictEqual(editName && editName.value, payload); + assert.doesNotThrow(() => sandbox.editMCPServer(editName!.value)); + assert.strictEqual(sandbox.__testState().editingServerName, payload); + assert.strictEqual(document.getElementById('serverType').value, 'http'); + assert.strictEqual(document.getElementById('serverUrl').value, 'https://example.com/mcp'); + }); + + test('a malicious config.type () renders as inert text in .server-type, not a live element', () => { + const payload = ''; + const { html } = renderServerItem({ srv: { type: payload } }, 'srv'); + assert.ok(!findAttr(html, 'onerror'), 'must not contain an onerror attribute; got: ' + html); + assert.strictEqual(textOn(html, 'server-type'), payload.toUpperCase()); + }); + + test('a plain server name/config with no special characters still renders visibly (no functional regression)', () => { + const { html, sandbox } = renderServerItem({ 'my-server': { type: 'stdio', command: 'node' } }, 'my-server'); + assert.ok(html.includes('my-server'), 'expected the server name to appear in the rendered output; got: ' + html); + assert.strictEqual(textOn(html, 'server-type'), 'STDIO'); + const editName = findAttrOn(html, 'server-edit-btn', 'data-server-name'); + assert.strictEqual(editName && editName.value, 'my-server'); + assert.strictEqual((sandbox.__testState().configsByName['my-server'] as { command: string }).command, 'node'); + }); +}); From 4ae5c2ae06c5f96580966e6a897af4c7c73e2a20 Mon Sep 17 00:00:00 2001 From: Jonas Kunert Date: Mon, 27 Jul 2026 20:36:57 +0200 Subject: [PATCH 11/22] fix: escape third-party data and guard URL schemes in the webview (fork-issue-61) Three sinks handled data from outside the extension with no escaping at all. renderDropdown, renderAllModels and renderOpenCreditsModelCards wrote model.id, model.name and model.owned_by/provider straight into data attributes and into innerHTML. The data comes from fetch(OPENCREDITS_API_URL + '/v1/models'): model-updater's resolveLatestModels() overwrites the bundled names with the API values, the extension posts them as updateRecommendedModels, and the webview replaces its model list and re-renders the cards without any user interaction. A name of produced a live element in the DOM. href and src from the open MCP registries were escaped after fork-issue-57 but their scheme was never checked, so a registry entry with "url": "javascript:..." was passed through verbatim. The new safeHttpUrl() only lets http:/https: through, strips tab/CR/LF and leading C0 control characters first and compares the scheme case-insensitively; anything it cannot make sense of is dropped rather than rendered, and callers omit the attribute instead of emitting a dead one. addEnvVariableRow built value="..." unescaped, which truncated any value containing a double quote. Covered by 21 tests (src/test/webview-attr-escape.test.ts), each one verified to fail against the previous code. Co-Authored-By: Claude Opus 5 --- src/html-escape.ts | 25 ++ src/script.ts | 69 +++-- src/test/webview-attr-escape.test.ts | 411 +++++++++++++++++++++++++++ 3 files changed, 485 insertions(+), 20 deletions(-) diff --git a/src/html-escape.ts b/src/html-escape.ts index 0873b9f..d20021a 100644 --- a/src/html-escape.ts +++ b/src/html-escape.ts @@ -14,3 +14,28 @@ export function escapeAttr(value: unknown): string { .replace(/"/g, '"') .replace(/'/g, '''); } + +// #61: escapeAttr() macht href=/src= ausbruchsicher, prueft aber kein Schema -- ein +// javascript:-Link aus Fremddaten (MCP-Registry-Eintrag) bleibt damit klickbar/wirksam. +// safeHttpUrl() laesst nur http:/https: durch, sonst leerer String (Aufrufer laesst das +// Attribut/den Link dann ganz weg statt ein totes Attribut zu rendern). Die Bereinigung vor +// dem Schema-Check spiegelt die ersten Schritte des WHATWG-URL-Parsers: Tab/Newline/CR werden +// ueberall im String entfernt (faengt "java\tscript:"), fuehrende/nachfolgende C0-Steuerzeichen +// und Leerzeichen werden abgeschnitten -- beides Tricks, mit denen Browser eine Schema-Pruefung +// per String-Vergleich sonst umgehen wuerden. Gleiche Selbstgenuegsamkeits-Regel wie escapeAttr: +// kein Modul-Level-Symbol, kein Import, keine Hilfsfunktion ausserhalb des Bodys. +// ACHTUNG (opus-Review): faellt bewusst fail-closed auch bei relativen ("/icons/x.png", +// "icon.png") und protokollrelativen ("//cdn.example/i.png") URLs auf '' -- ein Schema ist +// hier zwingend Voraussetzung, kein Sonderfall dafuer. Sollte eine Datenquelle kuenftig +// relative/protokollrelative Icon-URLs liefern, faellt das Icon still auf den Placeholder +// zurueck statt zu laden -- kein Bug, aber eine Verhaltensaenderung, die man sich merken sollte. +export function safeHttpUrl(value: unknown): string { + let s = value === null || value === undefined ? '' : String(value); + s = s.replace(/[\t\n\r]/g, ''); + s = s.replace(/^[\x00-\x20]+/, '').replace(/[\x00-\x20]+$/, ''); + const schemeMatch = /^([a-zA-Z][a-zA-Z0-9+\-.]*):/.exec(s); + if (!schemeMatch) { return ''; } + const scheme = schemeMatch[1].toLowerCase(); + if (scheme !== 'http' && scheme !== 'https') { return ''; } + return s; +} diff --git a/src/script.ts b/src/script.ts index c7cd882..fd72bc5 100644 --- a/src/script.ts +++ b/src/script.ts @@ -1,7 +1,7 @@ import getSkillsScript from './skills-script'; import getPluginsScript from './plugins-script'; import getCollapseScript from './collapse-script'; -import { escapeAttr } from './html-escape'; +import { escapeAttr, safeHttpUrl } from './html-escape'; const getScript = (isTelemetryEnabled: boolean, opencreditsApiUrl: string = 'https://ccc.api.opencredits.ai', opencreditsWebUrl: string = 'https://ccc.opencredits.ai', opencreditsPublishableKey: string = 'oc_pk_c43da4f9a9484ae484ad29bc97cc354f') => ` case)', () => { + const code = ''; + const dataRawCode = renderCodeBlockRawAttr(code); + assert.strictEqual(dataRawCode, code + '\n'); + const clipboardText = runCopyCodeBlock(dataRawCode); + assert.strictEqual(clipboardText, code + '\n'); + }); + + test('a code block literally containing "<" and ">" -- the clipboard text matches getAttribute() exactly', () => { + const code = 'a <div> tag as text'; + const dataRawCode = renderCodeBlockRawAttr(code); + assert.strictEqual(dataRawCode, code + '\n'); + const clipboardText = runCopyCodeBlock(dataRawCode); + assert.strictEqual(clipboardText, code + '\n'); + }); + + test('ordinary special characters (&, <, >, ", \') that do not spell out an entity -- the clipboard text matches getAttribute() exactly (no functional regression)', () => { + const code = 'const s = "a" + \'b\' & && d;'; + const dataRawCode = renderCodeBlockRawAttr(code); + assert.strictEqual(dataRawCode, code + '\n'); + const clipboardText = runCopyCodeBlock(dataRawCode); + assert.strictEqual(clipboardText, code + '\n'); + }); + + test('a multi-line code block mixing real special characters and literal entity text -- the clipboard text matches getAttribute() exactly', () => { + const code = [ + 'function f(a, b) {', + ' // literal example: "quoted" and &amp;', + ' return a < b && b > a ? "yes" : \'no\';', + '}' + ].join('\n'); + const dataRawCode = renderCodeBlockRawAttr(code); + assert.strictEqual(dataRawCode, code + '\n'); + const clipboardText = runCopyCodeBlock(dataRawCode); + assert.strictEqual(clipboardText, code + '\n'); + }); +}); From 521d539bae15c83af141d0c22fe0c3fb58d3ec0f Mon Sep 17 00:00:00 2001 From: Jonas Kunert Date: Tue, 28 Jul 2026 18:36:01 +0200 Subject: [PATCH 13/22] fix: dollar-sequence-safe code block restore in parseSimpleMarkdown (fork-issue-55) Replacement strings containing $&, $`, $' or $$ were interpreted as special replacement patterns by String.replace, corrupting restored code blocks. Extract the restore loop into markdown-restore.ts using a function replacement (same pattern as escapeAttr's build-time splice), add test:markdown-restore (9 tests). Cherry-picked from fork-issue-55's original branch onto this branch's own history (this branch never had fork-issue-47/KaTeX, so build/check-webview-syntax.js and its fork-issue-55 assertion additions, plus the now-irrelevant test:restore-commit-utils/test:perm-log-redact/ test:webview-syntax package.json lines and the renderMathEnabled restore guard, are intentionally dropped here -- foreign context from fork-issue-55's original branch that doesn't apply to this one). Co-Authored-By: Claude Fable 5 --- package.json | 3 +- src/markdown-restore.ts | 19 +++++++ src/script.ts | 19 ++++--- src/test/markdown-restore.test.ts | 87 +++++++++++++++++++++++++++++++ 4 files changed, 121 insertions(+), 7 deletions(-) create mode 100644 src/markdown-restore.ts create mode 100644 src/test/markdown-restore.test.ts diff --git a/package.json b/package.json index 112ad57..299d824 100644 --- a/package.json +++ b/package.json @@ -236,7 +236,8 @@ "test:models": "npm run compile && mocha --ui tdd out/test/model-updater.test.js --reporter spec", "test:collapse-rules": "npm run compile && mocha --ui tdd out/test/collapse-rules.test.js --reporter spec", "test:html-escape": "npm run compile && mocha --ui tdd out/test/html-escape.test.js --reporter spec", - "test:webview-attr-escape": "npm run compile && mocha --ui tdd out/test/webview-attr-escape.test.js --reporter spec" + "test:webview-attr-escape": "npm run compile && mocha --ui tdd out/test/webview-attr-escape.test.js --reporter spec", + "test:markdown-restore": "npm run compile && mocha --ui tdd out/test/markdown-restore.test.js --reporter spec" }, "devDependencies": { "@types/mocha": "^10.0.10", diff --git a/src/markdown-restore.ts b/src/markdown-restore.ts new file mode 100644 index 0000000..b06356e --- /dev/null +++ b/src/markdown-restore.ts @@ -0,0 +1,19 @@ +// Platzhalter-Ruecksubstitution fuer Code-Bloecke in parseSimpleMarkdown (#55, Befund aus +// dem #47-Review). String.replace(placeholder, value) interpretiert im Ersatzstring +// "$&"/"$`"/"$'"/"$$" als Substitutionsmuster -- ein Code-Block, dessen (bereits escapetes) +// Inhalt zufaellig eine solche Sequenz enthaelt (z. B. Shell-Code mit "$'...'"), zerlegt +// dadurch das umgebende HTML statt unveraendert zu erscheinen. restoreMathSegments +// (math-script.ts) hat diesen Fix von Anfang an -- diese Funktion zieht die Codeblock- +// Restore-Schleife auf denselben Stand (Funktions-Ersatz statt String-Ersatz). script.ts +// spleisst per .toString() nur den kompilierten Funktionstext in die Seite (Muster +// html-escape.ts/collapse-rules.ts) -- deshalb muss diese Funktion self-contained bleiben: +// kein Modul-Level-Symbol, kein Import, keine Hilfsfunktion ausserhalb des Bodys. +export function restoreCodeBlockPlaceholders(html: string, codeBlockPlaceholders: string[]): string { + for (let i = 0; i < codeBlockPlaceholders.length; i++) { + const placeholder = '__CODEBLOCK_' + i + '__'; + const value = codeBlockPlaceholders[i]; + // Funktions-Ersatz, NIE ein String direkt als 2. Argument (siehe Kommentar oben). + html = html.replace(placeholder, function () { return value; }); + } + return html; +} diff --git a/src/script.ts b/src/script.ts index d16e056..81f3458 100644 --- a/src/script.ts +++ b/src/script.ts @@ -2,6 +2,7 @@ import getSkillsScript from './skills-script'; import getPluginsScript from './plugins-script'; import getCollapseScript from './collapse-script'; import { escapeAttr, safeHttpUrl } from './html-escape'; +import { restoreCodeBlockPlaceholders } from './markdown-restore'; const getScript = (isTelemetryEnabled: boolean, opencreditsApiUrl: string = 'https://ccc.api.opencredits.ai', opencreditsWebUrl: string = 'https://ccc.opencredits.ai', opencreditsPublishableKey: string = 'oc_pk_c43da4f9a9484ae484ad29bc97cc354f') => ` block'); + } + return match[1]; +} + +// Duplicated from webview-attr-escape.test.ts (not exported there) -- see that file's own +// comment for why the quote-aware brace matching exists, and #64 for its known regex-literal +// gap. NOT repaired here, per #63's task scope; every call site below verifies its own +// extraction result instead of trusting it blindly (start/end/no dragged-in trailing function). +function extractFunction(source: string, name: string): string { + const sigMatch = new RegExp('function\\s+' + name + '\\s*\\(').exec(source); + if (!sigMatch) { + throw new Error('function ' + name + ' not found in emitted script'); + } + const braceStart = source.indexOf('{', sigMatch.index); + if (braceStart === -1) { + throw new Error('no opening brace found for function ' + name); + } + let depth = 0; + let inString: string | null = null; + for (let i = braceStart; i < source.length; i++) { + const ch = source[i]; + if (inString) { + if (ch === '\\') { i++; continue; } + if (ch === inString) { inString = null; } + continue; + } + if (ch === '/' && source[i + 1] === '/') { + const nl = source.indexOf('\n', i); + i = nl === -1 ? source.length : nl; + continue; + } + if (ch === '/' && source[i + 1] === '*') { + const end = source.indexOf('*/', i + 2); + i = end === -1 ? source.length : end + 1; + continue; + } + if (ch === '\'' || ch === '"' || ch === '`') { inString = ch; continue; } + else if (ch === '{') { depth++; } + else if (ch === '}') { + depth--; + if (depth === 0) { return source.slice(sigMatch.index, i + 1); } + } + } + throw new Error('unbalanced braces while extracting function ' + name); +} + +// Verifies an extractFunction() result actually starts/ends where expected and didn't drag in a +// trailing declaration (#64) -- the check the task asks for before building on the extraction. +function assertCleanExtraction(name: string, src: string, expectedStart: string): void { + assert.ok(src.startsWith(expectedStart), 'extractFunction(' + name + ') did not start where expected; got: ' + src.slice(0, 80)); + assert.ok(src.trimEnd().endsWith('}'), 'extractFunction(' + name + ') did not end at a closing brace; got: ' + src.slice(-80)); + assert.ok( + !/\n\s*function\s+\w+\s*\(/.test(src.slice(expectedStart.length)), + 'extractFunction(' + name + ') appears to have dragged in a trailing function declaration (#64); got length ' + src.length + ); +} + +// Extracts the exact statements inside `case '': ... break;` (quote-aware would be +// overkill here -- unlike extractFunction's braces, this case body contains no nested "break;" in +// any of the pre-#63/intermediate/current source variants, only its own terminating one). +// Verified below via an explicit assertion, same spirit as assertCleanExtraction for +// extractFunction. +function extractCaseBlock(source: string, caseLabel: string): string { + const re = new RegExp('case \'' + caseLabel + '\':([\\s\\S]*?)\\n\\s*break;'); + const m = re.exec(source); + if (!m) { + throw new Error('case \'' + caseLabel + '\': block not found in emitted script'); + } + return m[1]; +} + +// Faithful-enough DOM stand-in for addMessage(): unlike webview-attr-escape.test.ts's +// FakeElement/FakeDiv (built for direct innerHTML-string assignment sinks), addMessage builds +// the message via createElement()+appendChild() trees, so outerHTML must recursively serialise +// children -- this class does that. Only the setters/methods addMessage's (and escapeHtml's, +// which parseSimpleMarkdown/renderUserMessageContent also call) path actually touches are +// implemented; anything else the emitted code assigns (onclick, title, ...) lands as an ordinary +// dynamic JS property at runtime -- harmless, and irrelevant to what these tests assert on. +class FakeNode { + readonly tagName: string; + private _attrs = new Map(); + private _children: FakeNode[] = []; + private _text: string | null = null; + private _rawHtml: string | null = null; + + constructor(tagName: string) { this.tagName = tagName; } + + set className(v: string) { this._attrs.set('class', v); } + get className(): string { return this._attrs.get('class') || ''; } + + setAttribute(name: string, value: string): void { this._attrs.set(name, String(value)); } + + appendChild(child: FakeNode): FakeNode { + this._children.push(child); + this._text = null; + this._rawHtml = null; + return child; + } + + // Node.textContent setter semantics: replaces all children with a single implicit text node. + set textContent(v: string) { + this._text = String(v); + this._children = []; + this._rawHtml = null; + } + + // addMessage assigns innerHTML a hand-built HTML string (parseSimpleMarkdown's/ + // renderUserMessageContent's output, or copyBtn's fixed SVG) -- stored verbatim, meant to BE + // parsed as markup, unlike textContent above. + set innerHTML(v: string) { + this._rawHtml = String(v); + this._text = null; + this._children = []; + } + + // Real DOM Text-node HTML serialisation escapes only & < > (not " or ') -- matches + // webview-attr-escape.test.ts's FakeDiv, and is what escapeHtml() (document.createElement + // ('div').textContent = ...; return div.innerHTML) relies on internally. + get innerHTML(): string { + if (this._rawHtml !== null) { return this._rawHtml; } + if (this._text !== null) { + return this._text.replace(/&/g, '&').replace(//g, '>'); + } + return this._children.map(c => c.outerHTML).join(''); + } + + get outerHTML(): string { + const attrs = Array.from(this._attrs.entries()) + .map(([k, v]) => ' ' + k + '="' + v.replace(/&/g, '&').replace(/"/g, '"') + '"') + .join(''); + return '<' + this.tagName + attrs + '>' + this.innerHTML + ''; + } +} + +// Concatenates the direct #text children of the first element carrying cssClass (document +// order); throws if no such element exists. Copied from webview-attr-escape.test.ts's helper of +// the same name/behaviour (not exported there). Crucially only looks at DIRECT text children -- +// pre-#63 output wraps the text one level deeper in a

/

/
  • etc., and a real code-block +// (a real
    /
    element) is skipped entirely too -- so this returns the exact prose +// text around a code block, or '' if the message is nothing but a code block. +function textOn(html: string, cssClass: string): string { + const frag = parse5.parseFragment(html); + let result: string | undefined; + let elementFound = false; + (function walk(node: parse5.DefaultTreeAdapterMap['node']): void { + if (elementFound) { return; } + const el = node as parse5.DefaultTreeAdapterMap['element']; + if (el.attrs) { + const classAttr = el.attrs.find(x => x.name === 'class'); + if (classAttr && classAttr.value.split(/\s+/).includes(cssClass)) { + elementFound = true; + const parent = node as parse5.DefaultTreeAdapterMap['parentNode']; + result = (parent.childNodes || []) + .filter(n => n.nodeName === '#text') + .map(n => (n as parse5.DefaultTreeAdapterMap['textNode']).value) + .join(''); + return; + } + } + const parent = node as parse5.DefaultTreeAdapterMap['parentNode']; + if (parent.childNodes) { parent.childNodes.forEach(walk); } + })(frag); + if (!elementFound) { + throw new Error('no element with class "' + cssClass + '" found in: ' + html); + } + return result || ''; +} + +function tagExists(html: string, tagName: string): boolean { + const frag = parse5.parseFragment(html); + let found = false; + (function walk(node: parse5.DefaultTreeAdapterMap['node']): void { + if (found) { return; } + const el = node as parse5.DefaultTreeAdapterMap['element']; + if (el.tagName === tagName) { found = true; return; } + const parent = node as parse5.DefaultTreeAdapterMap['parentNode']; + if (parent.childNodes) { parent.childNodes.forEach(walk); } + })(frag); + return found; +} + +// Scoped lookup (per the task: findAttrOn, not the global-scan findAttr) -- finds the first +// element carrying cssClass and returns attrName's value from THAT element specifically. +function findAttrOn(html: string, cssClass: string, attrName: string): { name: string; value: string } | undefined { + const frag = parse5.parseFragment(html); + let result: { name: string; value: string } | undefined; + let elementFound = false; + (function walk(node: parse5.DefaultTreeAdapterMap['node']): void { + if (elementFound) { return; } + const el = node as parse5.DefaultTreeAdapterMap['element']; + if (el.attrs) { + const classAttr = el.attrs.find(x => x.name === 'class'); + if (classAttr && classAttr.value.split(/\s+/).includes(cssClass)) { + elementFound = true; + result = el.attrs.find(x => x.name === attrName); + return; + } + } + const parent = node as parse5.DefaultTreeAdapterMap['parentNode']; + if (parent.childNodes) { parent.childNodes.forEach(walk); } + })(frag); + if (!elementFound) { + throw new Error('no element with class "' + cssClass + '" found in: ' + html); + } + return result; +} + +interface FakeMessagesDiv { + scrollTop: number; + scrollHeight: number; + clientHeight: number; + lastAppended: FakeNode | undefined; + appendChild(child: FakeNode): FakeNode; +} + +interface UserInputPipelineSandbox { + addMessage(content: string, type: string): void; + parseSimpleMarkdown(markdown: string): string; + runUserInputCase(message: { data: string; timestamp?: string }): void; +} + +// Splices together the REAL extracted addMessage/parseSimpleMarkdown/extractCodeBlocks/ +// renderUserMessageContent (same recipe #62's loadCodeBlockSandbox already uses for +// parseSimpleMarkdown's own dependencies) and the REAL case 'userInput': block, wrapped +// as a callable function. Only addMessage's own free variables that are unrelated to #63 (scroll +// position, the copy-button raw-text map, the processing indicator, the two error-only helpers) +// are stubbed -- see the inline comments on each stub below for exactly why each one is safe to +// skip rather than extract. No timestamp/formatMessageTimestamp here -- that's a separate, +// unrelated feature not part of this security-hardening branch; addMessage here takes only +// (content, type), matching the actual signature in this branch. +function loadUserInputPipelineSandbox(): { sandbox: UserInputPipelineSandbox; messagesDiv: FakeMessagesDiv } { + const body = getEmittedScriptBody(); + + const addMessageSrc = extractFunction(body, 'addMessage'); + assertCleanExtraction('addMessage', addMessageSrc, 'function addMessage('); + + const parseSimpleMarkdownSrc = extractFunction(body, 'parseSimpleMarkdown'); + assertCleanExtraction('parseSimpleMarkdown', parseSimpleMarkdownSrc, 'function parseSimpleMarkdown('); + + const renderUserMessageContentSrc = extractFunction(body, 'renderUserMessageContent'); + assertCleanExtraction('renderUserMessageContent', renderUserMessageContentSrc, 'function renderUserMessageContent('); + + const caseBody = extractCaseBlock(body, 'userInput'); + assert.ok(caseBody.includes('addMessage('), 'case \'userInput\': block does not call addMessage; got: ' + caseBody); + assert.ok(!/\bbreak\s*;/.test(caseBody), 'case \'userInput\': extraction appears to have swallowed more than one case (unexpected nested break;); got: ' + caseBody); + + const src = [ + extractFunction(body, 'escapeHtml'), + extractFunction(body, 'escapeAttr'), + extractFunction(body, 'normalizeCollapseThreshold'), + extractFunction(body, 'evaluateCodeBlockCollapse'), + extractFunction(body, 'extractCodeBlocks'), + parseSimpleMarkdownSrc, + renderUserMessageContentSrc, + addMessageSrc, + // #46's copy-button raw-text store -- addMessage only .set()s into it, never reads it + // back, so a real (empty) WeakMap is enough, no extraction needed. + 'let messageRawText = new WeakMap();', + // Scroll-position bookkeeping, entirely orthogonal to what/how the message text renders. + 'function shouldAutoScroll() { return false; }', + 'function scrollToBottomIfNeeded() { /* no-op */ }', + // Needs isProcessing/showProcessingIndicator (unrelated globals) -- addMessage only ever + // calls it, never inspects its result. + 'function moveProcessingIndicatorToLast() { /* no-op */ }', + // Only reached when type === "error" (isPermissionError) -- never for the type === "user" + // tests below. + 'function isPermissionError() { return false; }', + 'function runUserInputCase(message) {\n' + caseBody + '\n}', + ].join('\n'); + + const messagesDiv: FakeMessagesDiv = { + scrollTop: 0, + scrollHeight: 0, + clientHeight: 0, + lastAppended: undefined, + appendChild(child: FakeNode) { this.lastAppended = child; return child; } + }; + const sandbox: Record = { + // parseSimpleMarkdown's/renderUserMessageContent's own settings -- math extraction + // skipped entirely (none of the payloads below contain "$"/"\("), same simplification + // #62's loadCodeBlockSandbox uses. + renderMathEnabled: false, + collapseLongCodeBlocks: true, + collapseCodeBlockLines: 20, + document: { + getElementById(id: string) { + if (id !== 'messages') { throw new Error('unexpected document.getElementById(' + id + ')'); } + return messagesDiv; + }, + createElement(tag: string) { + if (tag !== 'div' && tag !== 'button' && tag !== 'span') { + throw new Error('unexpected document.createElement(' + tag + ')'); + } + return new FakeNode(tag); + } + } + }; + vm.createContext(sandbox); + new vm.Script(src).runInContext(sandbox); + return { sandbox: sandbox as unknown as UserInputPipelineSandbox, messagesDiv }; +} + +// Runs the REAL case 'userInput': block (as compiled right now) against a mock incoming message +// and returns the resulting message element's outerHTML. Deliberately does NOT decide itself how +// the text gets rendered -- that's exactly what's compiled into caseBody, so this genuinely +// renders the pre-#63 markup-wrapped output against the old source, the flattened-code-block +// output against the intermediate textContent-only fix, and the raw-text-with-real-code-blocks +// output against the current source, without the test having to know which one it's looking at. +function renderUserInputMessage(payload: string): string { + const { sandbox, messagesDiv } = loadUserInputPipelineSandbox(); + sandbox.runUserInputCase({ data: payload, timestamp: undefined }); + if (!messagesDiv.lastAppended) { + throw new Error('case \'userInput\' did not append a message for payload: ' + payload); + } + return messagesDiv.lastAppended.outerHTML; +} + +suite('webview user-message raw text rendering (#63)', () => { + + test('a markdown-looking payload with no code fence renders as exactly that text -- no // elements', () => { + const payload = 'Bitte prüfe **src/_test_.ts** und `foo_bar`'; + const html = renderUserInputMessage(payload); + assert.strictEqual(textOn(html, 'message-content'), payload, 'message-content must contain the exact raw text; got: ' + html); + assert.ok(!tagExists(html, 'strong'), 'must not render a element; got: ' + html); + assert.ok(!tagExists(html, 'em'), 'must not render an element; got: ' + html); + assert.ok(!tagExists(html, 'code'), 'must not render a element (no fenced block in this payload); got: ' + html); + }); + + test('an XSS payload with no code fence renders as inert text -- no element and no onerror attribute', () => { + const payload = ''; + const html = renderUserInputMessage(payload); + assert.strictEqual(textOn(html, 'message-content'), payload, 'message-content must contain the exact raw text; got: ' + html); + assert.ok(!tagExists(html, 'img'), 'must not render an element; got: ' + html); + assert.ok(!findAttrOn(html, 'message-content', 'onerror'), 'message-content must not carry an onerror attribute; got: ' + html); + }); + + test('a multi-line payload with no code fence keeps its line breaks exactly', () => { + const payload = 'first line\nsecond line\nthird line'; + const html = renderUserInputMessage(payload); + assert.strictEqual(textOn(html, 'message-content'), payload, 'line breaks must survive exactly; got: ' + html); + }); + + test('a plain payload with no special characters still renders visibly (no functional regression)', () => { + const payload = 'hello world'; + const html = renderUserInputMessage(payload); + assert.strictEqual(textOn(html, 'message-content'), payload); + }); + + test('a fenced code block still renders as a real code-block-container (language label, copy button, exact data-raw-code), while the surrounding markdown-looking prose stays raw', () => { + const payload = 'Bitte **teste** das:\n```js\nconst x = 1;\n```\nDanke `dir`'; + const html = renderUserInputMessage(payload); + + // The fence itself is consumed by the real code-block element; the prose on either side + // stays exactly as typed (including its own "**"/backtick markdown-looking syntax). + assert.strictEqual( + textOn(html, 'message-content'), + 'Bitte **teste** das:\n\nDanke `dir`', + 'prose around the code block must stay raw and unwrapped; got: ' + html + ); + assert.ok(!tagExists(html, 'strong'), 'the prose\'s own "**teste**" must not become a element; got: ' + html); + assert.ok(!tagExists(html, 'em'), 'must not render an element; got: ' + html); + + assert.ok(!findAttrOn(html, 'message-content', 'onerror'), 'sanity: message-content itself must not carry an onerror attribute; got: ' + html); + const dataRawCode = findAttrOn(html, 'language-js', 'data-raw-code'); + assert.ok(dataRawCode, 'expected a data-raw-code attribute on the language-js code element; got: ' + html); + assert.strictEqual(dataRawCode!.value, 'const x = 1;\n', 'data-raw-code must contain the exact fenced code (#62 -- getAttribute()-equivalent, escapeAttr-escaped); got: ' + html); + assert.strictEqual(textOn(html, 'code-block-language'), 'js', 'the code block\'s language label must read "js"; got: ' + html); + assert.ok(tagExists(html, 'button'), 'expected the code block\'s copy button (#48/#62 copyCodeBlock) to exist; got: ' + html); + }); + + test('an XSS payload INSIDE a fenced code block renders as inert text -- no element anywhere, data-raw-code holds the exact raw code', () => { + const payload = '```\n\n```'; + const html = renderUserInputMessage(payload); + + assert.ok(!tagExists(html, 'img'), 'must not render a live element inside the code block; got: ' + html); + assert.ok(!findAttrOn(html, 'message-content', 'onerror'), 'sanity: message-content itself must not carry an onerror attribute; got: ' + html); + // No prose at all -- the payload is nothing but the fenced block. + assert.strictEqual(textOn(html, 'message-content'), '', 'a message that is only a code block must have no direct prose text; got: ' + html); + + const dataRawCode = findAttrOn(html, 'language-plaintext', 'data-raw-code'); + assert.ok(dataRawCode, 'expected a data-raw-code attribute on the language-plaintext code element; got: ' + html); + assert.strictEqual(dataRawCode!.value, '\n', 'data-raw-code must hold the exact raw (unescaped-at-read-time) code; got: ' + html); + // The rendered .code-line text must be the decoded literal string too (proves each code + // line is escapeHtml()'d, not parsed as HTML, same guarantee as parseSimpleMarkdown's + // existing code-block rendering). + assert.strictEqual(textOn(html, 'code-line'), '', 'the rendered code line must be the exact literal text, not a parsed element; got: ' + html); + }); + + test('Claude\'s own messages are unaffected -- addMessage still renders pre-built HTML via innerHTML for type "claude"', () => { + const { sandbox, messagesDiv } = loadUserInputPipelineSandbox(); + sandbox.addMessage( + '

    Bitte prüfe src/_test_.ts

    ', + 'claude' + ); + if (!messagesDiv.lastAppended) { + throw new Error('addMessage did not append a message'); + } + const html = messagesDiv.lastAppended.outerHTML; + assert.ok(tagExists(html, 'strong'), 'claude messages must still render pre-parsed HTML markup; got: ' + html); + assert.strictEqual(textOn(html, 'message-content'), '', 'the text must live inside the

    /, not as message-content\'s own direct text; got: ' + html); + }); + + test('Claude\'s own fenced code blocks (via the real parseSimpleMarkdown) still render correctly after the extractCodeBlocks refactor', () => { + const { sandbox } = loadUserInputPipelineSandbox(); + const rendered = sandbox.parseSimpleMarkdown('Bitte **teste** das:\n```js\nconst x = 1;\n```\nDanke'); + // Claude's own prose IS markdown-parsed -- "**teste**" becomes a real , unlike the + // user-message tests above. + assert.ok(tagExists(rendered, 'strong'), 'parseSimpleMarkdown must still render "**teste**" as for Claude messages; got: ' + rendered); + const dataRawCode = findAttrOn(rendered, 'language-js', 'data-raw-code'); + assert.ok(dataRawCode, 'expected a data-raw-code attribute on the language-js code element; got: ' + rendered); + assert.strictEqual(dataRawCode!.value, 'const x = 1;\n', 'data-raw-code must contain the exact fenced code; got: ' + rendered); + }); +}); diff --git a/src/test/webview-attr-escape.test.ts b/src/test/webview-attr-escape.test.ts index c8438ba..e7e662d 100644 --- a/src/test/webview-attr-escape.test.ts +++ b/src/test/webview-attr-escape.test.ts @@ -8,7 +8,7 @@ // // formatFilePath/formatToolInputUI only exist inline inside script.ts's giant getScript() // template literal -- never as an importable module, unlike escapeAttr/ -// evaluateCodeBlockCollapse/restoreCodeBlockPlaceholders -- so this suite extracts their exact +// evaluateCodeBlockCollapse -- so this suite extracts their exact // source text from the ACTUAL getScript() output (out/script.js, i.e. the real emitted webview // code, not a hand-copied version of the TS source) via brace-matching, runs it in a vm // sandbox with the one stub escapeHtml() needs (document.createElement), and parses the @@ -893,10 +893,13 @@ interface CodeBlockSandbox { // skipping math extraction entirely (rather than stubbing extractMathSegments/restoreMathSegments // and their katex dependency) keeps the sandbox to exactly the functions the data-raw-code path // actually needs: escapeHtml, escapeAttr, normalizeCollapseThreshold, evaluateCodeBlockCollapse, -// restoreCodeBlockPlaceholders, parseSimpleMarkdown. +// extractCodeBlocks, parseSimpleMarkdown. extractCodeBlocks (#63): parseSimpleMarkdown's +// fenced-code-block extraction moved into its own shared function (also used by +// renderUserMessageContent, see user-message-rawtext.test.ts) -- parseSimpleMarkdown now +// calls it instead of building the placeholder/collapse/copy-button HTML inline. function loadCodeBlockSandbox(): CodeBlockSandbox { const body = getEmittedScriptBody(); - const src = ['escapeHtml', 'escapeAttr', 'normalizeCollapseThreshold', 'evaluateCodeBlockCollapse', 'restoreCodeBlockPlaceholders', 'parseSimpleMarkdown'] + const src = ['escapeHtml', 'escapeAttr', 'normalizeCollapseThreshold', 'evaluateCodeBlockCollapse', 'extractCodeBlocks', 'parseSimpleMarkdown'] .map(name => extractFunction(body, name)) .join('\n'); const sandbox: Record = { diff --git a/src/ui-styles.ts b/src/ui-styles.ts index 1f56268..e155af2 100644 --- a/src/ui-styles.ts +++ b/src/ui-styles.ts @@ -1205,6 +1205,20 @@ const styles = ` padding-left: 6px; } + /* #63: user messages render their prose as raw text nodes (no

    wrapper), so pre-wrap is + needed to keep newlines/multi-space visible; break-word stops long unbroken tokens (paths, + URLs) from overflowing. line-height: 1.6 matches .message p's (ui-styles.ts, ~line 4045), + which no longer applies here since the prose text isn't wrapped in

    anymore -- without + this, user messages would fall back to .messages' line-height: 1.4 and look tighter than + before #63 (opus-Review finding). Code blocks inside a user message keep their own + .message-content pre.code-block { white-space: pre } untouched -- that rule targets the + 

     element directly, which always wins over inherited pre-wrap from this ancestor. */
+    .message.user .message-content {
+        white-space: pre-wrap;
+        word-wrap: break-word;
+        line-height: 1.6;
+    }
+
     /* Code blocks generated by markdown parser only */
     .message-content pre.code-block {
         background-color: var(--vscode-textCodeBlock-background);

From 6e653adbf31899c33ef8f3e16e811e65abd39eca Mon Sep 17 00:00:00 2001
From: Jonas Kunert 
Date: Tue, 28 Jul 2026 05:54:46 +0200
Subject: [PATCH 15/22] test: extract webview functions with the TypeScript
 parser (fork-issue-64)

extractFunction found a function's source by counting braces. The scanner knew
about strings and comments but not about regex literals, so escapeAttr's own
.replace(/'/g, ''') made it read the apostrophe as a string start and lose
the count: 362 characters of escapeAttr came back as 2727, dragging safeHttpUrl,
openFileInEditor, formatFilePath, toggleDiffExpansion and toggleResultExpansion
along and loading formatFilePath into the vm sandbox twice.

That was harmless only by accident -- escapeAttr sits at the front of the chunk
and the later, correctly extracted copy won. Editing or moving
toggleResultExpansion would have ended the extraction somewhere else or thrown
"unbalanced braces" and taken every test in the file with it.

TypeScript is already a devDependency, so the helper now asks its parser for the
declaration's span instead of re-implementing a tokenizer. 24 of the 25
extracted functions come back byte-identical; only escapeAttr changes, which is
the fix. The five helpers that had been hand-copied into two test files now live
in src/test/webview-dom-helpers.ts.

No production code changed.

Co-Authored-By: Claude Opus 5 
---
 src/test/user-message-rawtext.test.ts | 120 +------------
 src/test/webview-attr-escape.test.ts  | 238 +++++++++-----------------
 src/test/webview-dom-helpers.ts       | 134 +++++++++++++++
 3 files changed, 219 insertions(+), 273 deletions(-)
 create mode 100644 src/test/webview-dom-helpers.ts

diff --git a/src/test/user-message-rawtext.test.ts b/src/test/user-message-rawtext.test.ts
index 87d4942..4258953 100644
--- a/src/test/user-message-rawtext.test.ts
+++ b/src/test/user-message-rawtext.test.ts
@@ -21,8 +21,8 @@
 
 import * as assert from 'assert';
 import * as vm from 'vm';
-import * as parse5 from 'parse5';
 import getScript from '../script';
+import { extractFunction, findAttrOn, textOn, tagExists } from './webview-dom-helpers';
 
 function getEmittedScriptBody(): string {
 	const html = getScript(false);
@@ -33,50 +33,9 @@ function getEmittedScriptBody(): string {
 	return match[1];
 }
 
-// Duplicated from webview-attr-escape.test.ts (not exported there) -- see that file's own
-// comment for why the quote-aware brace matching exists, and #64 for its known regex-literal
-// gap. NOT repaired here, per #63's task scope; every call site below verifies its own
-// extraction result instead of trusting it blindly (start/end/no dragged-in trailing function).
-function extractFunction(source: string, name: string): string {
-	const sigMatch = new RegExp('function\\s+' + name + '\\s*\\(').exec(source);
-	if (!sigMatch) {
-		throw new Error('function ' + name + ' not found in emitted script');
-	}
-	const braceStart = source.indexOf('{', sigMatch.index);
-	if (braceStart === -1) {
-		throw new Error('no opening brace found for function ' + name);
-	}
-	let depth = 0;
-	let inString: string | null = null;
-	for (let i = braceStart; i < source.length; i++) {
-		const ch = source[i];
-		if (inString) {
-			if (ch === '\\') { i++; continue; }
-			if (ch === inString) { inString = null; }
-			continue;
-		}
-		if (ch === '/' && source[i + 1] === '/') {
-			const nl = source.indexOf('\n', i);
-			i = nl === -1 ? source.length : nl;
-			continue;
-		}
-		if (ch === '/' && source[i + 1] === '*') {
-			const end = source.indexOf('*/', i + 2);
-			i = end === -1 ? source.length : end + 1;
-			continue;
-		}
-		if (ch === '\'' || ch === '"' || ch === '`') { inString = ch; continue; }
-		else if (ch === '{') { depth++; }
-		else if (ch === '}') {
-			depth--;
-			if (depth === 0) { return source.slice(sigMatch.index, i + 1); }
-		}
-	}
-	throw new Error('unbalanced braces while extracting function ' + name);
-}
-
 // Verifies an extractFunction() result actually starts/ends where expected and didn't drag in a
-// trailing declaration (#64) -- the check the task asks for before building on the extraction.
+// trailing declaration (#64, fixed by making extractFunction parser-based) -- kept as a
+// belt-and-suspenders check before building on the extraction.
 function assertCleanExtraction(name: string, src: string, expectedStart: string): void {
 	assert.ok(src.startsWith(expectedStart), 'extractFunction(' + name + ') did not start where expected; got: ' + src.slice(0, 80));
 	assert.ok(src.trimEnd().endsWith('}'), 'extractFunction(' + name + ') did not end at a closing brace; got: ' + src.slice(-80));
@@ -163,79 +122,6 @@ class FakeNode {
 	}
 }
 
-// Concatenates the direct #text children of the first element carrying cssClass (document
-// order); throws if no such element exists. Copied from webview-attr-escape.test.ts's helper of
-// the same name/behaviour (not exported there). Crucially only looks at DIRECT text children --
-// pre-#63 output wraps the text one level deeper in a 
    /
    /
  •  etc., and a real code-block
-// (a real 
    /
     element) is skipped entirely too -- so this returns the exact prose
-// text around a code block, or '' if the message is nothing but a code block.
-function textOn(html: string, cssClass: string): string {
-	const frag = parse5.parseFragment(html);
-	let result: string | undefined;
-	let elementFound = false;
-	(function walk(node: parse5.DefaultTreeAdapterMap['node']): void {
-		if (elementFound) { return; }
-		const el = node as parse5.DefaultTreeAdapterMap['element'];
-		if (el.attrs) {
-			const classAttr = el.attrs.find(x => x.name === 'class');
-			if (classAttr && classAttr.value.split(/\s+/).includes(cssClass)) {
-				elementFound = true;
-				const parent = node as parse5.DefaultTreeAdapterMap['parentNode'];
-				result = (parent.childNodes || [])
-					.filter(n => n.nodeName === '#text')
-					.map(n => (n as parse5.DefaultTreeAdapterMap['textNode']).value)
-					.join('');
-				return;
-			}
-		}
-		const parent = node as parse5.DefaultTreeAdapterMap['parentNode'];
-		if (parent.childNodes) { parent.childNodes.forEach(walk); }
-	})(frag);
-	if (!elementFound) {
-		throw new Error('no element with class "' + cssClass + '" found in: ' + html);
-	}
-	return result || '';
-}
-
-function tagExists(html: string, tagName: string): boolean {
-	const frag = parse5.parseFragment(html);
-	let found = false;
-	(function walk(node: parse5.DefaultTreeAdapterMap['node']): void {
-		if (found) { return; }
-		const el = node as parse5.DefaultTreeAdapterMap['element'];
-		if (el.tagName === tagName) { found = true; return; }
-		const parent = node as parse5.DefaultTreeAdapterMap['parentNode'];
-		if (parent.childNodes) { parent.childNodes.forEach(walk); }
-	})(frag);
-	return found;
-}
-
-// Scoped lookup (per the task: findAttrOn, not the global-scan findAttr) -- finds the first
-// element carrying cssClass and returns attrName's value from THAT element specifically.
-function findAttrOn(html: string, cssClass: string, attrName: string): { name: string; value: string } | undefined {
-	const frag = parse5.parseFragment(html);
-	let result: { name: string; value: string } | undefined;
-	let elementFound = false;
-	(function walk(node: parse5.DefaultTreeAdapterMap['node']): void {
-		if (elementFound) { return; }
-		const el = node as parse5.DefaultTreeAdapterMap['element'];
-		if (el.attrs) {
-			const classAttr = el.attrs.find(x => x.name === 'class');
-			if (classAttr && classAttr.value.split(/\s+/).includes(cssClass)) {
-				elementFound = true;
-				result = el.attrs.find(x => x.name === attrName);
-				return;
-			}
-		}
-		const parent = node as parse5.DefaultTreeAdapterMap['parentNode'];
-		if (parent.childNodes) { parent.childNodes.forEach(walk); }
-	})(frag);
-	if (!elementFound) {
-		throw new Error('no element with class "' + cssClass + '" found in: ' + html);
-	}
-	return result;
-}
-
 interface FakeMessagesDiv {
 	scrollTop: number;
 	scrollHeight: number;
diff --git a/src/test/webview-attr-escape.test.ts b/src/test/webview-attr-escape.test.ts
index e7e662d..510ab7d 100644
--- a/src/test/webview-attr-escape.test.ts
+++ b/src/test/webview-attr-escape.test.ts
@@ -10,16 +10,17 @@
 // template literal -- never as an importable module, unlike escapeAttr/
 // evaluateCodeBlockCollapse -- so this suite extracts their exact
 // source text from the ACTUAL getScript() output (out/script.js, i.e. the real emitted webview
-// code, not a hand-copied version of the TS source) via brace-matching, runs it in a vm
-// sandbox with the one stub escapeHtml() needs (document.createElement), and parses the
-// resulting HTML string with parse5 -- the same HTML5-spec parser class real browsers use --
-// to assert no attribute-breakout / inline-handler-breakage survives. Run with
-// `npm run test:webview-attr-escape`.
+// code, not a hand-copied version of the TS source) via extractFunction() (webview-dom-helpers.ts,
+// parser-based since #64), runs it in a vm sandbox with the one stub escapeHtml() needs
+// (document.createElement), and parses the resulting HTML string with parse5 -- the same
+// HTML5-spec parser class real browsers use -- to assert no attribute-breakout / inline-handler-
+// breakage survives. Run with `npm run test:webview-attr-escape`.
 
 import * as assert from 'assert';
 import * as vm from 'vm';
 import * as parse5 from 'parse5';
 import getScript from '../script';
+import { extractFunction, findAttr, findAttrOn, textOn, tagExists } from './webview-dom-helpers';
 
 function getEmittedScriptBody(): string {
 	const html = getScript(false);
@@ -30,52 +31,6 @@ function getEmittedScriptBody(): string {
 	return match[1];
 }
 
-// Extracts one top-level "function NAME(...) { ... }" declaration's exact source text from
-// the emitted script body via quote-aware brace matching, so formatFilePath/formatToolInputUI
-// run exactly as emitted, never hand-copied from the TS source.
-function extractFunction(source: string, name: string): string {
-	const sigMatch = new RegExp('function\\s+' + name + '\\s*\\(').exec(source);
-	if (!sigMatch) {
-		throw new Error('function ' + name + ' not found in emitted script');
-	}
-	const braceStart = source.indexOf('{', sigMatch.index);
-	if (braceStart === -1) {
-		throw new Error('no opening brace found for function ' + name);
-	}
-	let depth = 0;
-	let inString: string | null = null;
-	for (let i = braceStart; i < source.length; i++) {
-		const ch = source[i];
-		if (inString) {
-			if (ch === '\\') { i++; continue; }
-			if (ch === inString) { inString = null; }
-			continue;
-		}
-		// #60: comments can contain an unbalanced quote (e.g. editMCPServer's pre-existing
-		// "// Don't allow name changes when editing") that would otherwise be misread as a
-		// string start, desyncing the brace count for everything after it and pulling in
-		// unrelated trailing functions (found via editMCPServer, which no earlier suite ever
-		// extracted). Skip comment contents entirely, same as a real JS tokenizer would.
-		if (ch === '/' && source[i + 1] === '/') {
-			const nl = source.indexOf('\n', i);
-			i = nl === -1 ? source.length : nl;
-			continue;
-		}
-		if (ch === '/' && source[i + 1] === '*') {
-			const end = source.indexOf('*/', i + 2);
-			i = end === -1 ? source.length : end + 1;
-			continue;
-		}
-		if (ch === '\'' || ch === '"' || ch === '`') { inString = ch; continue; }
-		else if (ch === '{') { depth++; }
-		else if (ch === '}') {
-			depth--;
-			if (depth === 0) { return source.slice(sigMatch.index, i + 1); }
-		}
-	}
-	throw new Error('unbalanced braces while extracting function ' + name);
-}
-
 // escapeHtml()'s only external dependency is document.createElement('div') (textContent set,
 // innerHTML read). Per the HTML fragment serialisation spec, a Text node's innerHTML escapes
 // only & < > (not " or ') -- real browsers behave the same; this is a faithful minimal stub.
@@ -110,56 +65,6 @@ function loadSandbox(): Sandbox {
 	return sandbox as unknown as Sandbox;
 }
 
-// First DFS match wins (document order) -- formatToolInputUI's output nests a
-// span.file-path-truncated (from formatFilePath) inside a div.diff-file-path, and both
-// currently carry a data-file-path attribute with the same value, so a "last match wins"
-// walk would silently return the inner span's copy instead of the outer div's -- the one
-// the div's own onclick="openFileInEditor(this.dataset.filePath)" actually reads. That would
-// let a regression that drops data-file-path from the div alone (while leaving the span's
-// copy intact) pass unnoticed. Use findAttrOn() below when a specific element matters.
-function findAttr(html: string, attrName: string): { name: string; value: string } | undefined {
-	const frag = parse5.parseFragment(html);
-	let found: { name: string; value: string } | undefined;
-	(function walk(node: parse5.DefaultTreeAdapterMap['node']): void {
-		if (found) { return; }
-		const el = node as parse5.DefaultTreeAdapterMap['element'];
-		if (el.attrs) {
-			const a = el.attrs.find(x => x.name === attrName);
-			if (a) { found = a; return; }
-		}
-		const parent = node as parse5.DefaultTreeAdapterMap['parentNode'];
-		if (parent.childNodes) { parent.childNodes.forEach(walk); }
-	})(frag);
-	return found;
-}
-
-// Scoped lookup: finds the first element carrying cssClass (document order) and returns
-// attrName's value from THAT element specifically (undefined if the element lacks it) --
-// unlike findAttr(), this doesn't get confused by a same-named attribute on a nested element.
-function findAttrOn(html: string, cssClass: string, attrName: string): { name: string; value: string } | undefined {
-	const frag = parse5.parseFragment(html);
-	let result: { name: string; value: string } | undefined;
-	let elementFound = false;
-	(function walk(node: parse5.DefaultTreeAdapterMap['node']): void {
-		if (elementFound) { return; }
-		const el = node as parse5.DefaultTreeAdapterMap['element'];
-		if (el.attrs) {
-			const classAttr = el.attrs.find(x => x.name === 'class');
-			if (classAttr && classAttr.value.split(/\s+/).includes(cssClass)) {
-				elementFound = true;
-				result = el.attrs.find(x => x.name === attrName);
-				return;
-			}
-		}
-		const parent = node as parse5.DefaultTreeAdapterMap['parentNode'];
-		if (parent.childNodes) { parent.childNodes.forEach(walk); }
-	})(frag);
-	if (!elementFound) {
-		throw new Error('no element with class "' + cssClass + '" found in: ' + html);
-	}
-	return result;
-}
-
 suite('webview attribute escaping: formatFilePath / formatToolInputUI (#57 PoC)', () => {
 
 	test('an attribute-breakout file_path (a" onmouseover="alert(1)" zz=") produces no onmouseover attribute anywhere in the emitted element', () => {
@@ -244,39 +149,6 @@ function extractDeclaration(source: string, name: string): string {
 	return match[0];
 }
 
-// Concatenates the direct #text children of the first element carrying cssClass (document
-// order); throws if no such element exists. If escaping were missing, a payload like
-// '' would parse as a real  element instead of literal text,
-// so those characters would be MISSING from this concatenation -- the full raw payload
-// round-tripping back as text is what proves the escaping worked.
-function textOn(html: string, cssClass: string): string {
-	const frag = parse5.parseFragment(html);
-	let result: string | undefined;
-	let elementFound = false;
-	(function walk(node: parse5.DefaultTreeAdapterMap['node']): void {
-		if (elementFound) { return; }
-		const el = node as parse5.DefaultTreeAdapterMap['element'];
-		if (el.attrs) {
-			const classAttr = el.attrs.find(x => x.name === 'class');
-			if (classAttr && classAttr.value.split(/\s+/).includes(cssClass)) {
-				elementFound = true;
-				const parent = node as parse5.DefaultTreeAdapterMap['parentNode'];
-				result = (parent.childNodes || [])
-					.filter(n => n.nodeName === '#text')
-					.map(n => (n as parse5.DefaultTreeAdapterMap['textNode']).value)
-					.join('');
-				return;
-			}
-		}
-		const parent = node as parse5.DefaultTreeAdapterMap['parentNode'];
-		if (parent.childNodes) { parent.childNodes.forEach(walk); }
-	})(frag);
-	if (!elementFound) {
-		throw new Error('no element with class "' + cssClass + '" found in: ' + html);
-	}
-	return result || '';
-}
-
 // Minimal DOM stand-in for displayMCPServers/editMCPServer/updateServerForm. All three only
 // ever call getElementById(id).{value,disabled,textContent,style.display,innerHTML,className},
 // document.createElement('div'), element.appendChild(child), element.insertAdjacentHTML(...)
@@ -470,11 +342,11 @@ interface DropdownSandbox {
 function loadDropdownSandbox(models: unknown[]): { sandbox: DropdownSandbox; dropdown: FakeElement } {
 	const body = getEmittedScriptBody();
 	const dropdown = new FakeElement();
-	// #64 (known infra gap, not fixed here): extractFunction('escapeAttr') also drags in
+	// #64 (fixed): extractFunction('escapeAttr') used to also drag in
 	// safeHttpUrl/openFileInEditor/formatFilePath/toggleDiffExpansion/toggleResultExpansion --
-	// escapeAttr's own /'/g regex literal desyncs the brace-matcher's naive quote tracking (see
-	// extractFunction's own comment above). Checked via a standalone extraction dump before
-	// relying on it here: harmless, those extra functions are only declared, never called.
+	// escapeAttr's own /'/g regex literal desynced the old brace-matcher's naive quote tracking.
+	// extractFunction is now parser-based (webview-dom-helpers.ts) and returns exactly the
+	// escapeAttr function, nothing more.
 	const src = [
 		extractFunction(body, 'escapeHtml'),
 		extractFunction(body, 'escapeAttr'),
@@ -686,19 +558,6 @@ suite('webview attribute escaping: renderOpenCreditsModelCards model-card grid (
 // entirely (icon placeholder / no GitHub link) instead of rendering a dead attribute.
 // ─────────────────────────────────────────────────────────────────────────
 
-function tagExists(html: string, tagName: string): boolean {
-	const frag = parse5.parseFragment(html);
-	let found = false;
-	(function walk(node: parse5.DefaultTreeAdapterMap['node']): void {
-		if (found) { return; }
-		const el = node as parse5.DefaultTreeAdapterMap['element'];
-		if (el.tagName === tagName) { found = true; return; }
-		const parent = node as parse5.DefaultTreeAdapterMap['parentNode'];
-		if (parent.childNodes) { parent.childNodes.forEach(walk); }
-	})(frag);
-	return found;
-}
-
 function classExists(html: string, cssClass: string): boolean {
 	const frag = parse5.parseFragment(html);
 	let found = false;
@@ -936,11 +795,10 @@ function renderCodeBlockRawAttr(code: string): string {
 // value, capturing what it hands to navigator.clipboard.writeText(...).
 function runCopyCodeBlock(dataRawCode: string): string {
 	const body = getEmittedScriptBody();
-	// #64 (known infra gap, not fixed here): extractFunction can drag in trailing functions when
-	// it misreads a regex literal as an unbalanced quote (see the renderDropdown suite's own #64
-	// comment above). copyCodeBlock's decode chain has four /pattern/g regex literals, none of
-	// which contain a brace or a "//"/quote that could desync the brace-matcher -- checked here
-	// via length/start/end instead of assuming that's safe.
+	// #64 (fixed): extractFunction is now parser-based and can no longer drag in trailing
+	// functions by misreading a regex literal as an unbalanced quote (copyCodeBlock's own decode
+	// chain has four /pattern/g regex literals). Kept as an explicit start/end/no-trailing-
+	// declaration check anyway, as a belt-and-suspenders regression guard for this one call site.
 	const src = extractFunction(body, 'copyCodeBlock');
 	assert.ok(src.startsWith('function copyCodeBlock(codeId) {'), 'extractFunction(copyCodeBlock) did not start where expected; got: ' + src.slice(0, 80));
 	assert.ok(src.trimEnd().endsWith('}'), 'extractFunction(copyCodeBlock) did not end at a closing brace; got: ' + src.slice(-80));
@@ -1018,3 +876,71 @@ suite('webview attribute escaping: copyCodeBlock double-decode (#62 Part A PoC)'
 		assert.strictEqual(clipboardText, code + '\n');
 	});
 });
+
+// ─────────────────────────────────────────────────────────────────────────
+// #64: extractFunction() (webview-dom-helpers.ts) used to find a function's source text via
+// hand-rolled, quote-aware brace matching, which knew about strings and comments but not about
+// regex literals. escapeAttr's own `.replace(/'/g, ''')` made the old scanner see `/` then
+// `'` and misread the apostrophe as a string start, desyncing the brace count for everything
+// after it -- pulling 2365 characters of unrelated trailing functions (safeHttpUrl,
+// openFileInEditor, formatFilePath, toggleDiffExpansion, toggleResultExpansion) into the
+// "escapeAttr" extraction, and duplicating formatFilePath into the vm sandbox. This was
+// previously harmless only because escapeAttr sits at the front of that chunk and a later,
+// correctly-extracted formatFilePath always overwrote the contaminated one -- reordering or
+// editing toggleResultExpansion would have broken every test in this file. extractFunction is
+// now parser-based (TypeScript's own parser) and structurally cannot have this failure mode;
+// these tests pin that down for the exact case that triggered it.
+// ─────────────────────────────────────────────────────────────────────────
+
+suite('extractFunction: regex literals containing a quote no longer desync extraction (#64)', () => {
+
+	test('extracting escapeAttr returns exactly its own body -- no trailing functions dragged in', () => {
+		const body = getEmittedScriptBody();
+		const src = extractFunction(body, 'escapeAttr');
+		assert.ok(src.startsWith('function escapeAttr('), 'expected the escapeAttr extraction to start with its own signature; got: ' + src.slice(0, 80));
+		assert.ok(src.trimEnd().endsWith('}'), 'expected the escapeAttr extraction to end at a closing brace; got: ' + src.slice(-80));
+		// The #64 bug specifically dragged in these five trailing declarations (in this order) --
+		// see the file header comment above.
+		for (const trailingName of ['safeHttpUrl', 'openFileInEditor', 'formatFilePath', 'toggleDiffExpansion', 'toggleResultExpansion']) {
+			assert.ok(
+				!src.includes('function ' + trailingName + '('),
+				'escapeAttr extraction must not contain a trailing function ' + trailingName + '(...) declaration (#64 regression); got: ' + src
+			);
+		}
+		// General form of the same check: no OTHER top-level "function NAME(" declaration may
+		// appear anywhere inside the extracted text at all.
+		assert.ok(
+			!/\n\s*function\s+\w+\s*\(/.test(src.slice('function escapeAttr('.length)),
+			'escapeAttr extraction appears to have dragged in a trailing function declaration (#64 regression); got length ' + src.length
+		);
+		// escapeAttr's own regex literals must still be present verbatim -- proves the fix didn't
+		// achieve a short extraction by truncating early instead of stopping at the right brace.
+		assert.ok(src.includes("replace(/'/g, ''')"), 'escapeAttr extraction is missing its own final .replace(/\'/g, \''\') call; got: ' + src);
+	});
+
+	test('extracting formatFilePath (declared after escapeAttr in the emitted script) still returns exactly its own body, not escapeAttr\'s', () => {
+		const body = getEmittedScriptBody();
+		const src = extractFunction(body, 'formatFilePath');
+		assert.ok(src.startsWith('function formatFilePath('), 'expected the formatFilePath extraction to start with its own signature; got: ' + src.slice(0, 80));
+		assert.ok(!src.includes('function escapeAttr('), 'formatFilePath extraction must not contain escapeAttr\'s declaration; got: ' + src);
+		assert.ok(!src.includes('function toggleDiffExpansion('), 'formatFilePath extraction must not contain toggleDiffExpansion\'s declaration; got: ' + src);
+	});
+
+	test('a synthetic function with an apostrophe inside a regex literal in its body extracts correctly and does not swallow the next declaration', () => {
+		const source = [
+			'function withQuoteInRegex(s) {',
+			"\treturn s.replace(/'/g, 'X');",
+			'}',
+			'',
+			'function nextFn() {',
+			'\treturn 1;',
+			'}'
+		].join('\n');
+		const src = extractFunction(source, 'withQuoteInRegex');
+		assert.strictEqual(src, [
+			'function withQuoteInRegex(s) {',
+			"\treturn s.replace(/'/g, 'X');",
+			'}'
+		].join('\n'));
+	});
+});
diff --git a/src/test/webview-dom-helpers.ts b/src/test/webview-dom-helpers.ts
new file mode 100644
index 0000000..397dc04
--- /dev/null
+++ b/src/test/webview-dom-helpers.ts
@@ -0,0 +1,134 @@
+// Shared extraction/DOM-inspection helpers for the webview vm-sandbox test suites
+// (webview-attr-escape.test.ts, user-message-rawtext.test.ts). Both files extracted
+// hand-copied duplicates of these five functions; pulled out here per #64.
+//
+// #64: extractFunction() used to find a function's source text via hand-rolled, quote-aware
+// brace matching. That scanner knew about strings and // and /* */ comments, but not about
+// regex literals -- escapeAttr's own `.replace(/'/g, ''')` made the scanner see `/` then
+// `'` and misread the apostrophe as a string start, desyncing the brace count for everything
+// after it and dragging 2365 chars of unrelated trailing functions (safeHttpUrl,
+// openFileInEditor, formatFilePath, toggleDiffExpansion, toggleResultExpansion) into the
+// "escapeAttr" extraction -- 362 chars of escapeAttr came back as 2727.
+// A real parser doesn't have this failure mode, so this now asks TypeScript's own parser
+// (already a devDependency) for the function declaration's exact span instead of re-implementing
+// a JS tokenizer by hand.
+import * as ts from 'typescript';
+import * as parse5 from 'parse5';
+
+// Finds a "function NAME(...) { ... }" declaration anywhere in `source` (at any nesting depth --
+// e.g. renderDropdown is declared inside initModelCombo) and returns its exact source text, in
+// document order (first match wins, same as the old regex-based scanner). Throws the same kind
+// of message as before when nothing matches.
+export function extractFunction(source: string, name: string): string {
+	const sourceFile = ts.createSourceFile('emitted-script.js', source, ts.ScriptTarget.Latest, true, ts.ScriptKind.JS);
+	let found: ts.FunctionDeclaration | undefined;
+	const visit = (node: ts.Node): void => {
+		if (found) { return; }
+		if (ts.isFunctionDeclaration(node) && node.name && node.name.text === name) {
+			found = node;
+			return;
+		}
+		ts.forEachChild(node, visit);
+	};
+	visit(sourceFile);
+	if (!found) {
+		throw new Error('function ' + name + ' not found in emitted script');
+	}
+	return source.slice(found.getStart(sourceFile), found.getEnd());
+}
+
+// First DFS match wins (document order) -- e.g. formatToolInputUI's output nests a
+// span.file-path-truncated (from formatFilePath) inside a div.diff-file-path, and both currently
+// carry a data-file-path attribute with the same value, so a "last match wins" walk would
+// silently return the inner span's copy instead of the outer div's -- the one the div's own
+// onclick="openFileInEditor(this.dataset.filePath)" actually reads. That would let a regression
+// that drops data-file-path from the div alone (while leaving the span's copy intact) pass
+// unnoticed. Use findAttrOn() below when a specific element matters.
+export function findAttr(html: string, attrName: string): { name: string; value: string } | undefined {
+	const frag = parse5.parseFragment(html);
+	let found: { name: string; value: string } | undefined;
+	(function walk(node: parse5.DefaultTreeAdapterMap['node']): void {
+		if (found) { return; }
+		const el = node as parse5.DefaultTreeAdapterMap['element'];
+		if (el.attrs) {
+			const a = el.attrs.find(x => x.name === attrName);
+			if (a) { found = a; return; }
+		}
+		const parent = node as parse5.DefaultTreeAdapterMap['parentNode'];
+		if (parent.childNodes) { parent.childNodes.forEach(walk); }
+	})(frag);
+	return found;
+}
+
+// Scoped lookup: finds the first element carrying cssClass (document order) and returns
+// attrName's value from THAT element specifically (undefined if the element lacks it) -- unlike
+// findAttr(), this doesn't get confused by a same-named attribute on a nested element.
+export function findAttrOn(html: string, cssClass: string, attrName: string): { name: string; value: string } | undefined {
+	const frag = parse5.parseFragment(html);
+	let result: { name: string; value: string } | undefined;
+	let elementFound = false;
+	(function walk(node: parse5.DefaultTreeAdapterMap['node']): void {
+		if (elementFound) { return; }
+		const el = node as parse5.DefaultTreeAdapterMap['element'];
+		if (el.attrs) {
+			const classAttr = el.attrs.find(x => x.name === 'class');
+			if (classAttr && classAttr.value.split(/\s+/).includes(cssClass)) {
+				elementFound = true;
+				result = el.attrs.find(x => x.name === attrName);
+				return;
+			}
+		}
+		const parent = node as parse5.DefaultTreeAdapterMap['parentNode'];
+		if (parent.childNodes) { parent.childNodes.forEach(walk); }
+	})(frag);
+	if (!elementFound) {
+		throw new Error('no element with class "' + cssClass + '" found in: ' + html);
+	}
+	return result;
+}
+
+// Concatenates the direct #text children of the first element carrying cssClass (document
+// order); throws if no such element exists. If escaping were missing, a payload like
+// '' would parse as a real  element instead of literal text, so
+// those characters would be MISSING from this concatenation -- the full raw payload
+// round-tripping back as text is what proves the escaping worked.
+export function textOn(html: string, cssClass: string): string {
+	const frag = parse5.parseFragment(html);
+	let result: string | undefined;
+	let elementFound = false;
+	(function walk(node: parse5.DefaultTreeAdapterMap['node']): void {
+		if (elementFound) { return; }
+		const el = node as parse5.DefaultTreeAdapterMap['element'];
+		if (el.attrs) {
+			const classAttr = el.attrs.find(x => x.name === 'class');
+			if (classAttr && classAttr.value.split(/\s+/).includes(cssClass)) {
+				elementFound = true;
+				const parent = node as parse5.DefaultTreeAdapterMap['parentNode'];
+				result = (parent.childNodes || [])
+					.filter(n => n.nodeName === '#text')
+					.map(n => (n as parse5.DefaultTreeAdapterMap['textNode']).value)
+					.join('');
+				return;
+			}
+		}
+		const parent = node as parse5.DefaultTreeAdapterMap['parentNode'];
+		if (parent.childNodes) { parent.childNodes.forEach(walk); }
+	})(frag);
+	if (!elementFound) {
+		throw new Error('no element with class "' + cssClass + '" found in: ' + html);
+	}
+	return result || '';
+}
+
+export function tagExists(html: string, tagName: string): boolean {
+	const frag = parse5.parseFragment(html);
+	let found = false;
+	(function walk(node: parse5.DefaultTreeAdapterMap['node']): void {
+		if (found) { return; }
+		const el = node as parse5.DefaultTreeAdapterMap['element'];
+		if (el.tagName === tagName) { found = true; return; }
+		const parent = node as parse5.DefaultTreeAdapterMap['parentNode'];
+		if (parent.childNodes) { parent.childNodes.forEach(walk); }
+	})(frag);
+	return found;
+}

From d3f90df9a640acb213c760effbc0000441f51e29 Mon Sep 17 00:00:00 2001
From: Jonas Kunert 
Date: Tue, 28 Jul 2026 18:50:44 +0200
Subject: [PATCH 16/22] test: full-pipeline dollar-sequence PoC for
 restoreCodeBlockPlaceholders (fork-issue-70)

Issue fork-issue-70 claimed parseSimpleMarkdown/renderUserMessageContent had reintroduced
the fork-issue-55 html.replace(placeholder, string) bug. At this branch's current tip
both call sites already used restoreCodeBlockPlaceholders (fork-issue-55's
function-replacer, already part of this branch's history), cherry-picked before fork-issue-63
landed -- fork-issue-63's own commit message documents that this had already been fixed up
within the same commit. No production code change was needed for the fork-issue-70 finding
itself.

Added the missing full-pipeline regression coverage fork-issue-70 asked for: a code
block containing "$&"/"$`"/"$'"/"$$" must not duplicate the surrounding
prose, exercised through the real parseSimpleMarkdown AND the real
renderUserMessageContent (not just a direct unit call to
restoreCodeBlockPlaceholders, which markdown-restore.test.ts already covered).

While wiring that up, found and fixed an unrelated, pre-existing gap in two
vm-sandbox test loaders: loadCodeBlockSandbox (webview-attr-escape.test.ts)
and loadUserInputPipelineSandbox (user-message-rawtext.test.ts) extracted
parseSimpleMarkdown/renderUserMessageContent but never extracted
restoreCodeBlockPlaceholders itself (script.ts splices it in via
`${restoreCodeBlockPlaceholders.toString()}`, a separate statement, not part
of either function's body) -- every test calling those two functions through
either sandbox threw "ReferenceError: restoreCodeBlockPlaceholders is not
defined" (14 failures, confirmed present before this commit via git stash).
Both loaders now include it alongside their other extracted dependencies.

141 passing, 0 failing (was 127 passing / 14 failing). eslint: 0 errors (25
pre-existing warnings, none in touched files). git grep -il agentdeck: empty.

Co-Authored-By: Claude Sonnet 5 
---
 src/test/user-message-rawtext.test.ts | 57 +++++++++++++++++++++++++++
 src/test/webview-attr-escape.test.ts  |  6 ++-
 2 files changed, 62 insertions(+), 1 deletion(-)

diff --git a/src/test/user-message-rawtext.test.ts b/src/test/user-message-rawtext.test.ts
index 4258953..0d2f5c8 100644
--- a/src/test/user-message-rawtext.test.ts
+++ b/src/test/user-message-rawtext.test.ts
@@ -133,6 +133,7 @@ interface FakeMessagesDiv {
 interface UserInputPipelineSandbox {
 	addMessage(content: string, type: string): void;
 	parseSimpleMarkdown(markdown: string): string;
+	renderUserMessageContent(text: string): string;
 	runUserInputCase(message: { data: string; timestamp?: string }): void;
 }
 
@@ -166,6 +167,11 @@ function loadUserInputPipelineSandbox(): { sandbox: UserInputPipelineSandbox; me
 		extractFunction(body, 'escapeAttr'),
 		extractFunction(body, 'normalizeCollapseThreshold'),
 		extractFunction(body, 'evaluateCodeBlockCollapse'),
+		// restoreCodeBlockPlaceholders (#55): script.ts splices this in via
+		// `${restoreCodeBlockPlaceholders.toString()}` (build-time, see markdown-restore.ts), so it
+		// appears in the emitted body as an ordinary function declaration extractFunction can find --
+		// both parseSimpleMarkdownSrc and renderUserMessageContentSrc below call it.
+		extractFunction(body, 'restoreCodeBlockPlaceholders'),
 		extractFunction(body, 'extractCodeBlocks'),
 		parseSimpleMarkdownSrc,
 		renderUserMessageContentSrc,
@@ -328,3 +334,54 @@ suite('webview user-message raw text rendering (#63)', () => {
 		assert.strictEqual(dataRawCode!.value, 'const x = 1;\n', 'data-raw-code must contain the exact fenced code; got: ' + rendered);
 	});
 });
+
+// ─────────────────────────────────────────────────────────────────────────
+// Gitea #70 PoC: extractCodeBlocks' per-block HTML (data-raw-code attribute plus escaped
+// code-line divs) does not strip "$"/"`"/"'" characters, so a code block whose content
+// contains one of the String.replace(placeholder, string) substitution sequences
+// ("$&"/"$`"/"$'"/"$$") reaches the placeholder-restore step still intact. Both
+// parseSimpleMarkdown and renderUserMessageContent (#63) restore their __CODEBLOCK_N__
+// placeholders via the shared restoreCodeBlockPlaceholders (#55) -- a FUNCTION replacer,
+// immune to this. These tests exercise the REAL, full pipeline (extractCodeBlocks +
+// escapeHtml/escapeAttr + restoreCodeBlockPlaceholders, not a direct unit call to
+// restoreCodeBlockPlaceholders as in markdown-restore.test.ts) through both entry points and
+// prove the surrounding prose is not corrupted: a plain html.replace(placeholder, codeBlockHtml)
+// here would splice codeBlockHtml's own preceding/following HTML ("$`"/"$'") or a literal "$"
+// duplicate ("$$") into the restored output, which would show up as MARKER_BEFORE/MARKER_AFTER
+// appearing more than once and/or a corrupted data-raw-code value.
+// ─────────────────────────────────────────────────────────────────────────
+
+suite('webview code-block restore: "$" substitution patterns cannot corrupt surrounding HTML (#70, full pipeline)', () => {
+
+	// Covers all four String.replace substitution sequences at once: "$&" (matched substring),
+	// "$`" (pre-match), "$'" (post-match), "$$" (literal "$").
+	const dollarPayload = 'echo $& run; VAR=$`date`; MSG=$\'ok\'; echo $$';
+
+	test('parseSimpleMarkdown: a code block containing "$&"/"$`"/"$\'"/"$$" does not duplicate the surrounding prose and round-trips byte-for-byte', () => {
+		const { sandbox } = loadUserInputPipelineSandbox();
+		const markdown = 'MARKER_BEFORE\n\n```\n' + dollarPayload + '\n```\n\nMARKER_AFTER';
+		const html = sandbox.parseSimpleMarkdown(markdown);
+
+		assert.strictEqual(html.split('MARKER_BEFORE').length - 1, 1, 'MARKER_BEFORE must appear exactly once, not spliced into the code block; got: ' + html);
+		assert.strictEqual(html.split('MARKER_AFTER').length - 1, 1, 'MARKER_AFTER must appear exactly once, not spliced into the code block; got: ' + html);
+
+		const dataRawCode = findAttrOn(html, 'language-plaintext', 'data-raw-code');
+		assert.ok(dataRawCode, 'expected a data-raw-code attribute on the language-plaintext code element; got: ' + html);
+		assert.strictEqual(dataRawCode!.value, dollarPayload + '\n', 'data-raw-code must contain the exact fenced code, dollar sequences untouched; got: ' + html);
+		assert.strictEqual(textOn(html, 'code-line'), dollarPayload, 'the rendered code line must be the exact literal text; got: ' + html);
+	});
+
+	test('renderUserMessageContent: a code block containing "$&"/"$`"/"$\'"/"$$" does not duplicate the surrounding raw-text prose and round-trips byte-for-byte', () => {
+		const { sandbox } = loadUserInputPipelineSandbox();
+		const text = 'MARKER_BEFORE\n```\n' + dollarPayload + '\n```\nMARKER_AFTER';
+		const html = sandbox.renderUserMessageContent(text);
+
+		assert.strictEqual(html.split('MARKER_BEFORE').length - 1, 1, 'MARKER_BEFORE must appear exactly once, not spliced into the code block; got: ' + html);
+		assert.strictEqual(html.split('MARKER_AFTER').length - 1, 1, 'MARKER_AFTER must appear exactly once, not spliced into the code block; got: ' + html);
+
+		const dataRawCode = findAttrOn(html, 'language-plaintext', 'data-raw-code');
+		assert.ok(dataRawCode, 'expected a data-raw-code attribute on the language-plaintext code element; got: ' + html);
+		assert.strictEqual(dataRawCode!.value, dollarPayload + '\n', 'data-raw-code must contain the exact fenced code, dollar sequences untouched; got: ' + html);
+		assert.strictEqual(textOn(html, 'code-line'), dollarPayload, 'the rendered code line must be the exact literal text; got: ' + html);
+	});
+});
diff --git a/src/test/webview-attr-escape.test.ts b/src/test/webview-attr-escape.test.ts
index 510ab7d..46aaa2d 100644
--- a/src/test/webview-attr-escape.test.ts
+++ b/src/test/webview-attr-escape.test.ts
@@ -758,7 +758,11 @@ interface CodeBlockSandbox {
 // calls it instead of building the placeholder/collapse/copy-button HTML inline.
 function loadCodeBlockSandbox(): CodeBlockSandbox {
 	const body = getEmittedScriptBody();
-	const src = ['escapeHtml', 'escapeAttr', 'normalizeCollapseThreshold', 'evaluateCodeBlockCollapse', 'extractCodeBlocks', 'parseSimpleMarkdown']
+	// restoreCodeBlockPlaceholders (#55): script.ts splices this in via
+	// `${restoreCodeBlockPlaceholders.toString()}` (build-time, see markdown-restore.ts), so it
+	// appears in the emitted body as an ordinary function declaration extractFunction can find,
+	// same as the others below -- parseSimpleMarkdown calls it to restore __CODEBLOCK_N__.
+	const src = ['escapeHtml', 'escapeAttr', 'normalizeCollapseThreshold', 'evaluateCodeBlockCollapse', 'restoreCodeBlockPlaceholders', 'extractCodeBlocks', 'parseSimpleMarkdown']
 		.map(name => extractFunction(body, name))
 		.join('\n');
 	const sandbox: Record = {

From f034ee4aa72fd68ea7affd72377519ff3842a947 Mon Sep 17 00:00:00 2001
From: Jonas Kunert 
Date: Tue, 28 Jul 2026 19:33:49 +0200
Subject: [PATCH 17/22] chore: translate internal review comments and fix
 per-key settings batch handling

Translates German-language code comments to English and removes internal
process/tracker references (review-tool name, tracker name, personal name)
so the branch reads cleanly as a standalone PR. Also fixes a real bug found
while correcting a stale comment in _updateSettings: the loop over settings
keys had a single outer try/catch, so one failing key silently discarded all
subsequent keys and skipped resending settings/balance to the webview. Each
key now gets its own try/catch instead.
---
 src/collapse-rules.ts                 | 10 ++---
 src/collapse-script.ts                |  4 +-
 src/extension.ts                      | 50 +++++++++++++-----------
 src/html-escape.ts                    | 42 ++++++++++----------
 src/markdown-restore.ts               | 23 +++++------
 src/script.ts                         | 56 +++++++++++++--------------
 src/test/markdown-restore.test.ts     |  2 +-
 src/test/user-message-rawtext.test.ts | 22 +++++------
 src/test/webview-attr-escape.test.ts  |  4 +-
 src/ui-styles.ts                      | 14 +++----
 10 files changed, 117 insertions(+), 110 deletions(-)

diff --git a/src/collapse-rules.ts b/src/collapse-rules.ts
index 542a33b..1664a4d 100644
--- a/src/collapse-rules.ts
+++ b/src/collapse-rules.ts
@@ -13,9 +13,9 @@
 export interface CodeBlockCollapseInfo { lineCount: number; collapse: boolean; maxLines: number; }
 
 export function normalizeCollapseThreshold(value: unknown): number {
-	// Defaults/Grenzen MUESSEN im Funktionskoerper stehen: .toString() liefert nur den
-	// Text dieser Funktion, kein Modul-Level-Symbol.
-	const DEFAULT_LINES = 20; // in sync mit claudeCodeChat.ui.collapseCodeBlockLines (package.json)
+	// Defaults/limits MUST live inside the function body: .toString() only returns
+	// this function's own text, not any module-level symbol.
+	const DEFAULT_LINES = 20; // kept in sync with claudeCodeChat.ui.collapseCodeBlockLines (package.json)
 	const MIN_LINES = 5;
 	const MAX_LINES = 500;
 	const n = typeof value === 'number' ? value : Number(value);
@@ -29,8 +29,8 @@ export function normalizeCollapseThreshold(value: unknown): number {
 export function evaluateCodeBlockCollapse(code: string, configuredMaxLines: unknown): CodeBlockCollapseInfo {
 	const maxLines = normalizeCollapseThreshold(configuredMaxLines);
 	const text = typeof code === 'string' ? code : '';
-	// Die Fence-Regex in parseSimpleMarkdown faengt das Newline VOR der schliessenden
-	// Fence mit: "a\nb\nc\n" sind 3 Zeilen, nicht 4. CRLF vorher normalisieren.
+	// The fence regex in parseSimpleMarkdown captures the newline BEFORE the closing
+	// fence: "a\nb\nc\n" is 3 lines, not 4. Normalize CRLF beforehand.
 	const normalized = text.replace(/\r\n/g, '\n').replace(/\n$/, '');
 	const lineCount = normalized === '' ? 0 : normalized.split('\n').length;
 	return { lineCount: lineCount, collapse: lineCount > maxLines, maxLines: maxLines };
diff --git a/src/collapse-script.ts b/src/collapse-script.ts
index 4d02942..9bb42d4 100644
--- a/src/collapse-script.ts
+++ b/src/collapse-script.ts
@@ -24,12 +24,12 @@ const getCollapseScript = () => `
 		// R2/R3: bound to the 's synchronous onclick, never the 
    's
 		// ontoggle -- toggle fires asynchronously and also for a programmatic .open
 		// assignment, which would make applyCodeBlockCollapseDefaults() below unable to
-		// tell a real user click from its own nachzieh pass after the first run.
+		// tell a real user click from its own catch-up pass after the first run.
 		function markCodeBlockToggled(summaryEl) {
 			summaryEl.parentElement.setAttribute('data-user-toggled', '1');
 		}
 
-		// Nachzieh-Pass (R1): settingsData arrives AFTER the history replay
+		// Catch-up pass (R1): settingsData arrives AFTER the history replay
 		// (extension.ts _loadConversationHistory -> _sendReadyMessage -> _sendCurrentSettings),
 		// so blocks rendered from history always start out using the webview's hardcoded
 		// default. This re-applies the real collapseLongCodeBlocks setting to every block the
diff --git a/src/extension.ts b/src/extension.ts
index cfa95fb..166a9f5 100644
--- a/src/extension.ts
+++ b/src/extension.ts
@@ -3422,7 +3422,7 @@ class ClaudeChatProvider {
 	// write; a double failure gets a 'yoloModeEnableFailed' reply plus a native error
 	// notification instead, and never a success confirmation.
 	//
-	// opus-review follow-up: the outer try/catch below exists because this method is
+	// Follow-up: the outer try/catch below exists because this method is
 	// called fire-and-forget (extension.ts's message handler does
 	// `this._enableYoloMode();`, no `await`/`.catch`, see the switch above). Before this
 	// review pass, only the two config.update() calls inside
@@ -3466,15 +3466,16 @@ class ClaudeChatProvider {
 
 	private async _updateSettings(settings: { [key: string]: any }): Promise {
 		const config = vscode.workspace.getConfiguration('claudeCodeChat');
+		const failures: string[] = [];
 
-		try {
-			for (const [key, value] of Object.entries(settings)) {
+		// Each key gets its own try/catch so one failing key (e.g. a double
+		// workspace+global failure on permissions.yoloMode below) can never silently
+		// prevent the remaining keys in this settings batch from being applied.
+		for (const [key, value] of Object.entries(settings)) {
+			try {
 				if (key === 'permissions.yoloMode') {
 					// #59: YOLO mode: try workspace first, fall back to global (same
-					// helper _enableYoloMode uses). A double failure throws here, same as
-					// before, so applySettingsBatch's own per-key try/catch still records
-					// it as a failure -- no behavior change, just one shared
-					// implementation instead of two copies of this fallback.
+					// helper _enableYoloMode uses).
 					const yoloResult = await updateWithWorkspaceThenGlobalFallback(
 						async () => { await config.update(key, value, vscode.ConfigurationTarget.Workspace); },
 						async () => { await config.update(key, value, vscode.ConfigurationTarget.Global); }
@@ -3486,24 +3487,29 @@ class ClaudeChatProvider {
 					// Other settings are global (user-wide)
 					await config.update(key, value, vscode.ConfigurationTarget.Global);
 				}
+			} catch (error: any) {
+				console.error(`Failed to update setting "${key}":`, error?.message || error);
+				failures.push(`${key}: ${error?.message || error}`);
 			}
+		}
 
-			// Re-send settings so webview gets updated isOpenCredits flag, etc.
-			this._sendCurrentSettings();
+		if (failures.length > 0) {
+			vscode.window.showErrorMessage(`Failed to update settings: ${failures.join('; ')}`);
+		}
 
-			// Update balance display based on new env vars
-			if (this._isOpenCredits() || this._getOpenCreditsKey()) {
-				this._sendOpenCreditsBalance();
-			} else {
-				// Clear balance if no longer OpenCredits
-				this._postMessage({
-					type: 'opencreditsBalance',
-					balance: null
-				});
-			}
-		} catch (error: any) {
-			console.error('Failed to update settings:', error?.message || error);
-			vscode.window.showErrorMessage(`Failed to update settings: ${error?.message || 'Unknown error'}`);
+		// Re-send settings so webview gets updated isOpenCredits flag, etc. Runs even if
+		// some keys above failed, so the keys that did succeed are still reflected back.
+		this._sendCurrentSettings();
+
+		// Update balance display based on new env vars
+		if (this._isOpenCredits() || this._getOpenCreditsKey()) {
+			this._sendOpenCreditsBalance();
+		} else {
+			// Clear balance if no longer OpenCredits
+			this._postMessage({
+				type: 'opencreditsBalance',
+				balance: null
+			});
 		}
 	}
 
diff --git a/src/html-escape.ts b/src/html-escape.ts
index d20021a..1c2c2f3 100644
--- a/src/html-escape.ts
+++ b/src/html-escape.ts
@@ -1,12 +1,12 @@
-// Attribut-Escaping fuer die Webview (#49). script.ts' escapeHtml() serialisiert ueber
-// textContent->innerHTML und laesst " und ' deshalb STEHEN -- fuer title="..."/data-*="..."
-// reicht das nicht (Attribut-Ausbruch). Diese Funktion wird NICHT hier aufgerufen: script.ts
-// spleisst per .toString() nur ihren eigenen kompilierten Text in die Seite (Muster
-// math-segments.ts/collapse-rules.ts). Deshalb MUSS sie self-contained bleiben -- kein
-// Modul-Level-Symbol, kein Import, keine Hilfsfunktion ausserhalb des Bodys.
+// Attribute escaping for the webview (#49). script.ts' escapeHtml() serializes via
+// textContent->innerHTML and therefore leaves " and ' UNTOUCHED -- for title="..."/data-*="..."
+// that isn't enough (attribute breakout). This function is NOT called here: script.ts
+// splices only its own compiled text into the page via .toString() (same pattern as
+// math-segments.ts/collapse-rules.ts). So it MUST stay self-contained -- no
+// module-level symbol, no import, no helper function outside the body.
 export function escapeAttr(value: unknown): string {
 	const s = value === null || value === undefined ? '' : String(value);
-	// '&' zwingend zuerst, sonst werden die eigenen Entities nachtraeglich zerlegt.
+	// '&' must come first, otherwise the entities we just produced get re-decoded afterwards.
 	return s
 		.replace(/&/g, '&')
 		.replace(/;
-				// kuerzere bleiben Zeichen fuer Zeichen wie vorher.
+				// #48 (upstream #151): blocks over the threshold become 
    ;
+				// shorter ones stay character-for-character as before.
 				const collapseInfo = evaluateCodeBlockCollapse(code, collapseCodeBlockLines);
 				const copyGuard = collapseInfo.collapse ? 'event.preventDefault();event.stopPropagation();' : '';
 				const copyBtnHtml = '';
@@ -4709,11 +4709,11 @@ const getScript = (isTelemetryEnabled: boolean, opencreditsApiUrl: string = 'htt
 			return html;
 		}
 
-		// #63 (revised per opus-Review): user messages stay raw text -- "**", "_", "#" lines,
+		// #63 (revised after further review): user messages stay raw text -- "**", "_", "#" lines,
 		// single backticks, paths like src/_test_.ts must show up exactly as typed -- EXCEPT
 		// fenced triple-backtick code blocks, which keep the #48 collapse/copy-button/
 		// language-label treatment (the plain textContent-only approach also flattened those,
-		// which Roman didn't want). Reuses extractCodeBlocks() -- the exact same function
+		// which was intentionally excluded from this behavior). Reuses extractCodeBlocks() -- the exact same function
 		// parseSimpleMarkdown calls above -- so the code-block HTML (incl. #62's
 		// data-raw-code via escapeAttr) is only ever built in one place. The remaining
 		// prose is only escapeHtml()'d, never markdown-parsed, so it's set via
@@ -5359,10 +5359,10 @@ const getScript = (isTelemetryEnabled: boolean, opencreditsApiUrl: string = 'htt
 				});
 			} else if (message.type === 'settingsData') {
 				// Update UI with current settings
-				// #48 (upstream #151): Defaults nachziehen. settingsData trifft NACH dem
-				// History-Replay ein (extension.ts _loadConversationHistory -> _sendReadyMessage),
-				// deshalb wird der Default hier rueckwirkend auf alle noch nicht vom Nutzer
-				// angefassten Bloecke angewandt.
+				// #48 (upstream #151): reconcile defaults. settingsData arrives AFTER the
+				// history replay (extension.ts _loadConversationHistory -> _sendReadyMessage),
+				// so the default is applied retroactively here to every block the user
+				// hasn't touched yet.
 				collapseLongCodeBlocks = message.data['ui.collapseLongCodeBlocks'] !== false;
 				collapseCodeBlockLines = normalizeCollapseThreshold(message.data['ui.collapseCodeBlockLines']);
 				document.getElementById('collapse-long-code').checked = collapseLongCodeBlocks;
diff --git a/src/test/markdown-restore.test.ts b/src/test/markdown-restore.test.ts
index fe4c122..541d87c 100644
--- a/src/test/markdown-restore.test.ts
+++ b/src/test/markdown-restore.test.ts
@@ -2,7 +2,7 @@
 // __CODEBLOCK_N__ half of parseSimpleMarkdown's placeholder dance. Pure (no vscode, no
 // network, no DOM), so these run under plain mocha against the compiled out/ output --
 // same pattern as diff-utils/shell-utils/auto-model-switch/math-segments/collapse-rules/
-// html-escape. #47's opus review found that the loop used html.replace(placeholder, str),
+// html-escape. #47's review found that the loop used html.replace(placeholder, str),
 // a plain STRING as the 2nd argument -- String.replace treats "$&"/"$`"/"$'"/"$$" in a
 // string replacement as substitution patterns, so a code block whose (already-escaped)
 // content happens to contain one of those sequences tears the surrounding HTML apart
diff --git a/src/test/user-message-rawtext.test.ts b/src/test/user-message-rawtext.test.ts
index 0d2f5c8..4b7e165 100644
--- a/src/test/user-message-rawtext.test.ts
+++ b/src/test/user-message-rawtext.test.ts
@@ -3,12 +3,12 @@
 // mentioning a path like src/_test_.ts -- got silently reinterpreted as formatting instead of
 // showing up exactly as typed).
 //
-// Fix, revised per opus-Review: the case 'userInput' handler now calls renderUserMessageContent
+// Fix, revised after further review: the case 'userInput' handler now calls renderUserMessageContent
 // instead of parseSimpleMarkdown. renderUserMessageContent keeps prose completely raw (only
 // escapeHtml, never markdown-parsed) but still runs fenced ``` code blocks through the same
 // extractCodeBlocks() machinery parseSimpleMarkdown itself uses -- an earlier, plain-
 // textContent-only version of this fix also flattened code blocks (lost the #48
-// collapse/copy-button/language-label treatment and #62's data-raw-code), which Roman didn't want.
+// collapse/copy-button/language-label treatment and #62's data-raw-code), which was intentionally excluded from this behavior.
 // Claude's/thinking's own messages are unchanged (parseSimpleMarkdown + innerHTML).
 //
 // These tests exercise the REAL case 'userInput': block's exact source text (extracted from the
@@ -241,7 +241,7 @@ function renderUserInputMessage(payload: string): string {
 suite('webview user-message raw text rendering (#63)', () => {
 
 	test('a markdown-looking payload with no code fence renders as exactly that text -- no // elements', () => {
-		const payload = 'Bitte prüfe **src/_test_.ts** und `foo_bar`';
+		const payload = 'Please check **src/_test_.ts** and `foo_bar`';
 		const html = renderUserInputMessage(payload);
 		assert.strictEqual(textOn(html, 'message-content'), payload, 'message-content must contain the exact raw text; got: ' + html);
 		assert.ok(!tagExists(html, 'strong'), 'must not render a  element; got: ' + html);
@@ -270,17 +270,17 @@ suite('webview user-message raw text rendering (#63)', () => {
 	});
 
 	test('a fenced code block still renders as a real code-block-container (language label, copy button, exact data-raw-code), while the surrounding markdown-looking prose stays raw', () => {
-		const payload = 'Bitte **teste** das:\n```js\nconst x = 1;\n```\nDanke `dir`';
+		const payload = 'Please **test** this:\n```js\nconst x = 1;\n```\nThanks `dir`';
 		const html = renderUserInputMessage(payload);
 
 		// The fence itself is consumed by the real code-block element; the prose on either side
 		// stays exactly as typed (including its own "**"/backtick markdown-looking syntax).
 		assert.strictEqual(
 			textOn(html, 'message-content'),
-			'Bitte **teste** das:\n\nDanke `dir`',
+			'Please **test** this:\n\nThanks `dir`',
 			'prose around the code block must stay raw and unwrapped; got: ' + html
 		);
-		assert.ok(!tagExists(html, 'strong'), 'the prose\'s own "**teste**" must not become a  element; got: ' + html);
+		assert.ok(!tagExists(html, 'strong'), 'the prose\'s own "**test**" must not become a  element; got: ' + html);
 		assert.ok(!tagExists(html, 'em'), 'must not render an  element; got: ' + html);
 
 		assert.ok(!findAttrOn(html, 'message-content', 'onerror'), 'sanity: message-content itself must not carry an onerror attribute; got: ' + html);
@@ -312,7 +312,7 @@ suite('webview user-message raw text rendering (#63)', () => {
 	test('Claude\'s own messages are unaffected -- addMessage still renders pre-built HTML via innerHTML for type "claude"', () => {
 		const { sandbox, messagesDiv } = loadUserInputPipelineSandbox();
 		sandbox.addMessage(
-			'
    Bitte prüfe src/_test_.ts
    ',
+			'
    Please check src/_test_.ts
    ',
 			'claude'
 		);
 		if (!messagesDiv.lastAppended) {
@@ -325,10 +325,10 @@ suite('webview user-message raw text rendering (#63)', () => {
 
 	test('Claude\'s own fenced code blocks (via the real parseSimpleMarkdown) still render correctly after the extractCodeBlocks refactor', () => {
 		const { sandbox } = loadUserInputPipelineSandbox();
-		const rendered = sandbox.parseSimpleMarkdown('Bitte **teste** das:\n```js\nconst x = 1;\n```\nDanke');
-		// Claude's own prose IS markdown-parsed -- "**teste**" becomes a real , unlike the
+		const rendered = sandbox.parseSimpleMarkdown('Please **test** this:\n```js\nconst x = 1;\n```\nThanks');
+		// Claude's own prose IS markdown-parsed -- "**test**" becomes a real , unlike the
 		// user-message tests above.
-		assert.ok(tagExists(rendered, 'strong'), 'parseSimpleMarkdown must still render "**teste**" as  for Claude messages; got: ' + rendered);
+		assert.ok(tagExists(rendered, 'strong'), 'parseSimpleMarkdown must still render "**test**" as  for Claude messages; got: ' + rendered);
 		const dataRawCode = findAttrOn(rendered, 'language-js', 'data-raw-code');
 		assert.ok(dataRawCode, 'expected a data-raw-code attribute on the language-js code element; got: ' + rendered);
 		assert.strictEqual(dataRawCode!.value, 'const x = 1;\n', 'data-raw-code must contain the exact fenced code; got: ' + rendered);
@@ -336,7 +336,7 @@ suite('webview user-message raw text rendering (#63)', () => {
 });
 
 // ─────────────────────────────────────────────────────────────────────────
-// Gitea #70 PoC: extractCodeBlocks' per-block HTML (data-raw-code attribute plus escaped
+// PoC for the code-block restore fix: extractCodeBlocks' per-block HTML (data-raw-code attribute plus escaped
 // code-line divs) does not strip "$"/"`"/"'" characters, so a code block whose content
 // contains one of the String.replace(placeholder, string) substitution sequences
 // ("$&"/"$`"/"$'"/"$$") reaches the placeholder-restore step still intact. Both
diff --git a/src/test/webview-attr-escape.test.ts b/src/test/webview-attr-escape.test.ts
index 46aaa2d..3dfad0f 100644
--- a/src/test/webview-attr-escape.test.ts
+++ b/src/test/webview-attr-escape.test.ts
@@ -469,7 +469,7 @@ suite('webview attribute escaping: renderAllModels "all models" modal (#61 Part
 });
 
 // ─────────────────────────────────────────────────────────────────────────
-// #61 Part A follow-up (opus-Review): openCreditsModels is overwritten wholesale by
+// #61 Part A follow-up: openCreditsModels is overwritten wholesale by
 // resolveLatestModels() (model-updater.ts) from fetch(apiBaseUrl + '/v1/models') -- the SAME
 // third-party endpoint as renderDropdown/renderAllModels above, just reached indirectly via
 // extension.ts's 'updateRecommendedModels' postMessage -- and renderOpenCreditsModelCards()
@@ -507,7 +507,7 @@ function loadModelCardsSandbox(openCreditsModels: unknown[]): { sandbox: ModelCa
 
 suite('webview attribute escaping: renderOpenCreditsModelCards model-card grid (#61 Part A follow-up PoC)', () => {
 
-	test('an  payload in model.name renders as inert text, not a live element (the opus-Review finding)', () => {
+	test('an  payload in model.name renders as inert text, not a live element (the review finding)', () => {
 		const payload = '';
 		const { sandbox, document } = loadModelCardsSandbox([{ id: 'openai/gpt-9.9', name: payload, provider: 'openai' }]);
 		sandbox.renderOpenCreditsModelCards();
diff --git a/src/ui-styles.ts b/src/ui-styles.ts
index e155af2..c27852e 100644
--- a/src/ui-styles.ts
+++ b/src/ui-styles.ts
@@ -1156,10 +1156,10 @@ const styles = `
     }
     .message:hover .message-collapse-btn { opacity: 0.8; }
     .message-collapse-btn:hover { opacity: 1; background-color: var(--vscode-list-hoverBackground); }
-    /* Eingeklappt bleibt der Griff dauerhaft sichtbar, sonst findet ihn niemand wieder. */
+    /* While collapsed, the handle stays permanently visible, otherwise nobody would find it again. */
     .message.collapsed .message-collapse-btn { opacity: 0.9; }
-    /* Alles ausser dem Header verbergen -- deckt .message-content UND Zusatzbloecke
-       wie .yolo-suggestion mit ab. */
+    /* Hide everything except the header -- this also covers .message-content AND extra
+       blocks like .yolo-suggestion. */
     .message.collapsed > *:not(.message-header) { display: none; }
     .message.collapsed .message-header { margin-bottom: 0; padding-bottom: 0; border-bottom: none; }
 
@@ -1210,7 +1210,7 @@ const styles = `
        URLs) from overflowing. line-height: 1.6 matches .message p's (ui-styles.ts, ~line 4045),
        which no longer applies here since the prose text isn't wrapped in 
     anymore -- without
        this, user messages would fall back to .messages' line-height: 1.4 and look tighter than
-       before #63 (opus-Review finding). Code blocks inside a user message keep their own
+       before #63 (a review finding). Code blocks inside a user message keep their own
        .message-content pre.code-block { white-space: pre } untouched -- that rule targets the
        
     element directly, which always wins over inherited pre-wrap from this ancestor. */
     .message.user .message-content {
@@ -1298,9 +1298,9 @@ const styles = `
         background: none;
     }
 
-    /* Collapsible long code blocks (#48, upstream #151). Die  behaelt die
-       Klasse .code-block-header, damit alle Bestands- und Compact-Mode-Regeln
-       unveraendert greifen; nur Marker/Cursor/Flow kommen dazu. */
+    /* Collapsible long code blocks (#48, upstream #151). The  keeps the
+       .code-block-header class so all existing and compact-mode rules still
+       apply unchanged; only marker/cursor/flow styling is added. */
     details.code-block-container > summary.code-block-header {
         cursor: pointer;
         list-style: none;

From 989266c5e897c5ccc64e96d7c8fdb78a7f3ebfbc Mon Sep 17 00:00:00 2001
From: Jonas Kunert 
Date: Tue, 28 Jul 2026 20:51:12 +0200
Subject: [PATCH 18/22] chore: qualify remaining internal issue references in
 code comments

The previous "translate internal review comments" pass removed jargon and
personal names but left every bare #NN comment reference untouched, so
GitHub would still auto-link them to unrelated issues on this repo. Each
bare reference (140 across 9 source and 6 test files) is requalified as
fork-issue-NN. Genuine "upstream #NNN" references (8x "upstream #151",
1x "upstream #63" in script.ts's extractCodeBlocks/parseSimpleMarkdown
comment) are left untouched since they point at the upstream project's
own issue tracker, not ours.
---
 src/collapse-rules.ts                 |  2 +-
 src/collapse-script.ts                |  4 +-
 src/extension.ts                      |  8 +--
 src/html-escape.ts                    |  4 +-
 src/markdown-restore.ts               |  4 +-
 src/plugins-script.ts                 |  2 +-
 src/script.ts                         | 84 +++++++++++++--------------
 src/settings-batch.ts                 | 14 ++---
 src/skills-script.ts                  |  4 +-
 src/test/collapse-rules.test.ts       |  2 +-
 src/test/html-escape.test.ts          |  2 +-
 src/test/markdown-restore.test.ts     |  8 +--
 src/test/settings-batch.test.ts       | 12 ++--
 src/test/user-message-rawtext.test.ts | 38 ++++++------
 src/test/webview-attr-escape.test.ts  | 72 +++++++++++------------
 src/test/webview-dom-helpers.ts       |  4 +-
 src/ui-styles.ts                      |  8 +--
 17 files changed, 136 insertions(+), 136 deletions(-)

diff --git a/src/collapse-rules.ts b/src/collapse-rules.ts
index 1664a4d..1f88588 100644
--- a/src/collapse-rules.ts
+++ b/src/collapse-rules.ts
@@ -1,4 +1,4 @@
-// Pure threshold/line-count logic for the #48 collapsible-code-blocks feature (upstream
+// Pure threshold/line-count logic for the fork-issue-48 collapsible-code-blocks feature (upstream
 // #151): decides whether a fenced code block parseSimpleMarkdown is about to render should
 // start collapsed, based on its line count and the configured threshold. No vscode import,
 // so this runs under plain mocha like diff-utils/shell-utils/auto-model-switch/math-segments.
diff --git a/src/collapse-script.ts b/src/collapse-script.ts
index 9bb42d4..e7cb234 100644
--- a/src/collapse-script.ts
+++ b/src/collapse-script.ts
@@ -1,6 +1,6 @@
 import { normalizeCollapseThreshold, evaluateCodeBlockCollapse } from './collapse-rules';
 
-// Webview-side glue for the #48 collapsible-code-blocks feature (upstream #151), injected
+// Webview-side glue for the fork-issue-48 collapsible-code-blocks feature (upstream #151), injected
 // into script.ts's getScript() template the same way getMathScript()/getSkillsScript() are
 // (see plugins-script.ts). Two different things happen below and they must not be confused:
 //
@@ -17,7 +17,7 @@ import { normalizeCollapseThreshold, evaluateCodeBlockCollapse } from './collaps
 //    -- but if you add code that does, escape it the same way (see script.ts's
 //    parseSimpleMarkdown for examples).
 const getCollapseScript = () => `
-		// ─── Collapsible code blocks + per-message fold (#48) ───
+		// ─── Collapsible code blocks + per-message fold (fork-issue-48) ───
 		${normalizeCollapseThreshold.toString()}
 		${evaluateCodeBlockCollapse.toString()}
 
diff --git a/src/extension.ts b/src/extension.ts
index 166a9f5..06909fe 100644
--- a/src/extension.ts
+++ b/src/extension.ts
@@ -3412,8 +3412,8 @@ class ClaudeChatProvider {
 		});
 	}
 
-	// #59: workspace-then-global fallback (same pattern _updateSettings already used for
-	// this key, via updateWithWorkspaceThenGlobalFallback). Before #59 this only tried
+	// fork-issue-59: workspace-then-global fallback (same pattern _updateSettings already used for
+	// this key, via updateWithWorkspaceThenGlobalFallback). Before fork-issue-59 this only tried
 	// Workspace and swallowed the error into the console -- in a window with no
 	// workspace folder open that meant YOLO mode was never actually persisted, while the
 	// webview's "YOLO Mode enabled!" chat message (script.ts's enableYoloMode()) fired
@@ -3430,7 +3430,7 @@ class ClaudeChatProvider {
 	// logic (e.g. a settings-batch.ts that's out of sync with extension.ts after a
 	// partial deploy, so updateWithWorkspaceThenGlobalFallback itself is undefined) can
 	// also throw, an uncaught rejection here would silently swallow the click with none
-	// of #59's reporting -- exactly the failure class #59 exists to close.
+	// of fork-issue-59's reporting -- exactly the failure class fork-issue-59 exists to close.
 	private async _enableYoloMode(): Promise {
 		try {
 			const config = vscode.workspace.getConfiguration('claudeCodeChat');
@@ -3474,7 +3474,7 @@ class ClaudeChatProvider {
 		for (const [key, value] of Object.entries(settings)) {
 			try {
 				if (key === 'permissions.yoloMode') {
-					// #59: YOLO mode: try workspace first, fall back to global (same
+					// fork-issue-59: YOLO mode: try workspace first, fall back to global (same
 					// helper _enableYoloMode uses).
 					const yoloResult = await updateWithWorkspaceThenGlobalFallback(
 						async () => { await config.update(key, value, vscode.ConfigurationTarget.Workspace); },
diff --git a/src/html-escape.ts b/src/html-escape.ts
index 1c2c2f3..553a9ad 100644
--- a/src/html-escape.ts
+++ b/src/html-escape.ts
@@ -1,4 +1,4 @@
-// Attribute escaping for the webview (#49). script.ts' escapeHtml() serializes via
+// Attribute escaping for the webview (fork-issue-49). script.ts' escapeHtml() serializes via
 // textContent->innerHTML and therefore leaves " and ' UNTOUCHED -- for title="..."/data-*="..."
 // that isn't enough (attribute breakout). This function is NOT called here: script.ts
 // splices only its own compiled text into the page via .toString() (same pattern as
@@ -15,7 +15,7 @@ export function escapeAttr(value: unknown): string {
 		.replace(/'/g, ''');
 }
 
-// #61: escapeAttr() makes href=/src= breakout-safe but doesn't check the scheme -- a
+// fork-issue-61: escapeAttr() makes href=/src= breakout-safe but doesn't check the scheme -- a
 // javascript:-link from third-party data (an MCP registry entry) stays clickable/live.
 // safeHttpUrl() only lets http:/https: through, otherwise an empty string (the caller then
 // omits the attribute/link entirely instead of rendering a dead attribute). The cleanup before
diff --git a/src/markdown-restore.ts b/src/markdown-restore.ts
index 174241b..3f1a23f 100644
--- a/src/markdown-restore.ts
+++ b/src/markdown-restore.ts
@@ -1,5 +1,5 @@
-// Placeholder back-substitution for code blocks in parseSimpleMarkdown (#55, a finding
-// from the #47 review). String.replace(placeholder, value) interprets "$&"/"$`"/"$'"/"$$"
+// Placeholder back-substitution for code blocks in parseSimpleMarkdown (fork-issue-55, a finding
+// from the fork-issue-47 review). String.replace(placeholder, value) interprets "$&"/"$`"/"$'"/"$$"
 // in the replacement string as substitution patterns -- a code block whose (already
 // escaped) content happens to contain such a sequence (e.g. shell code with "$'...'")
 // tears the surrounding HTML apart instead of appearing unchanged. restoreMathSegments
diff --git a/src/plugins-script.ts b/src/plugins-script.ts
index ed393b1..922b874 100644
--- a/src/plugins-script.ts
+++ b/src/plugins-script.ts
@@ -72,7 +72,7 @@ const getPluginsScript = () => `
 				var displayName = formatPluginName(name);
 				var desc = escapeHtml(plugin.description || 'No description');
 				var verified = plugin.verified;
-				// #57: escapeAttr replaces escapeHtml()+manual "'"->"'" replace, which left
+				// fork-issue-57: escapeAttr replaces escapeHtml()+manual "'"->"'" replace, which left
 				// " unescaped and able to break out of the data-plugin-id attribute below.
 				var safeId = escapeAttr(plugin.installId || name);
 
diff --git a/src/script.ts b/src/script.ts
index de3dd66..a348868 100644
--- a/src/script.ts
+++ b/src/script.ts
@@ -84,13 +84,13 @@ const getScript = (isTelemetryEnabled: boolean, opencreditsApiUrl: string = 'htt
 		let isWindows = false;
 		let lastPendingEditIndex = -1; // Track the last Edit/MultiEdit/Write toolUse without result
 		let lastPendingEditData = null; // Store diff data for the pending edit { filePath, oldContent, newContent }
-		// #48 (upstream #151): claudeCodeChat.ui.collapseLongCodeBlocks / .collapseCodeBlockLines.
+		// fork-issue-48 (upstream #151): claudeCodeChat.ui.collapseLongCodeBlocks / .collapseCodeBlockLines.
 		// Must sit up here (let isn't hoisted), even though the rest of the logic
 		// is spliced in further down via \${getCollapseScript()}.
 		let collapseLongCodeBlocks = true;
 		let collapseCodeBlockLines = 20;
 		let attachedImages = []; // Array of { filePath, previewUri }
-		// #59: text for the next 'yoloModeEnabled' response's chat message, set by
+		// fork-issue-59: text for the next 'yoloModeEnabled' response's chat message, set by
 		// enableYoloMode() right before it posts 'enableYoloMode' to the extension host.
 		// The two call sites (inline permission-menu item vs. the standalone chat
 		// button) use different wording, so this can't be a hardcoded string in the
@@ -176,7 +176,7 @@ const getScript = (isTelemetryEnabled: boolean, opencreditsApiUrl: string = 'htt
 				headerDiv.appendChild(labelDiv);
 				headerDiv.appendChild(copyBtn);
 
-				// #48 (upstream #151): manual collapse of an entire message.
+				// fork-issue-48 (upstream #151): manual collapse of an entire message.
 				// Deliberately inserted AFTER the copy button, because .copy-btn's
 				// margin-left:auto pushes both to the right (ui-styles.ts:1152).
 				const collapseBtn = document.createElement('button');
@@ -194,7 +194,7 @@ const getScript = (isTelemetryEnabled: boolean, opencreditsApiUrl: string = 'htt
 			const contentDiv = document.createElement('div');
 			contentDiv.className = 'message-content';
 			
-			// #63: user messages are pre-rendered by renderUserMessageContent (raw text,
+			// fork-issue-63: user messages are pre-rendered by renderUserMessageContent (raw text,
 			// only fenced code blocks turned into real markup) before reaching here, so
 			// they go through the same contentDiv.innerHTML path as Claude's/thinking's
 			// parseSimpleMarkdown output.
@@ -271,7 +271,7 @@ const getScript = (isTelemetryEnabled: boolean, opencreditsApiUrl: string = 'htt
 							todo.status === 'in_progress' ? '🔄' : '⏳';
 						todoHtml += '\\n' + status + ' ' + todo.content;
 					}
-					// #49: plain text, no markup -- .tool-input has white-space: pre-line
+					// fork-issue-49: plain text, no markup -- .tool-input has white-space: pre-line
 					contentDiv.textContent = todoHtml;
 				} else {
 					// Format raw input with expandable content for long values
@@ -353,7 +353,7 @@ const getScript = (isTelemetryEnabled: boolean, opencreditsApiUrl: string = 'htt
 			scrollToBottomIfNeeded(messagesDiv, shouldScroll);
 		}
 
-		// #62: the dead expandable-input helper (the same "[expand]" placeholder +
+		// fork-issue-62: the dead expandable-input helper (the same "[expand]" placeholder +
 		// data-key/data-value expand-btn pattern) removed as dead code -- unreachable (grep
 		// across src/ + out/ found only its own declaration, no call site, no window[...]/
 		// onclick-string dynamic invocation anywhere) and superseded by formatToolInputUI's own
@@ -497,7 +497,7 @@ const getScript = (isTelemetryEnabled: boolean, opencreditsApiUrl: string = 'htt
 				html += '
    Suggested actions:
    ';
 				input.allowedPrompts.forEach(function(p) {
 					var label = p.prompt || (p.tool + ' command');
-					// #57: value moved into data-prompt (escapeAttr) instead of an HTML-entity-
+					// fork-issue-57: value moved into data-prompt (escapeAttr) instead of an HTML-entity-
 					// escaped JS string literal inside onclick -- the old escapeHtml()+manual
 					// ' replace still left " unescaped, breaking out of the attribute.
 					html += '';
@@ -534,7 +534,7 @@ const getScript = (isTelemetryEnabled: boolean, opencreditsApiUrl: string = 'htt
 			// Special handling for Read tool with file_path
 			if (input.file_path && Object.keys(input).length === 1) {
 				const formattedPath = formatFilePath(input.file_path);
-				// #57: path moved into data-file-path (escapeAttr) + this.dataset.filePath --
+				// fork-issue-57: path moved into data-file-path (escapeAttr) + this.dataset.filePath --
 				// the old escapeHtml() + hand-escaped \' JS-string embed broke on both " (attribute
 				// breakout) and ' (premature end of the JS string argument).
 				return '
    ' + formattedPath + '
    ';
@@ -859,12 +859,12 @@ const getScript = (isTelemetryEnabled: boolean, opencreditsApiUrl: string = 'htt
 			return div.innerHTML;
 		}
 
-		// #49: attribute escaping -- escapeHtml() leaves " and ' untouched. Build-time splice
+		// fork-issue-49: attribute escaping -- escapeHtml() leaves " and ' untouched. Build-time splice
 		// (same pattern as math-script/collapse-script) so npm run test:html-escape can
 		// exercise the function under Node. NOTE: this deliberately uses "\${", not "\\\${".
 		${escapeAttr.toString()}
 
-		// #61: schema guard for href=/src= -- escapeAttr() alone lets javascript:-links
+		// fork-issue-61: schema guard for href=/src= -- escapeAttr() alone lets javascript:-links
 		// through untouched. Same build-time splice as escapeAttr directly above.
 		${safeHttpUrl.toString()}
 
@@ -922,7 +922,7 @@ const getScript = (isTelemetryEnabled: boolean, opencreditsApiUrl: string = 'htt
 
 		function toggleExpand(button) {
 			const key = button.getAttribute('data-key') || '';
-			// #49: getAttribute() already returns decoded values -- the manual
+			// fork-issue-49: getAttribute() already returns decoded values -- the manual
 			// "/' unescaping that used to be here was a SECOND decode.
 			const value = button.getAttribute('data-value') || '';
 
@@ -1619,7 +1619,7 @@ const getScript = (isTelemetryEnabled: boolean, opencreditsApiUrl: string = 'htt
 		}
 
 		let editingServerName = null;
-		// #60: configs keyed by server name -- editMCPServer used to receive the whole config
+		// fork-issue-60: configs keyed by server name -- editMCPServer used to receive the whole config
 		// object JSON.stringify()'d straight into an onclick(...) attribute (a second,
 		// un-escapeAttr-able sink alongside the name itself). The object now stays in JS-land;
 		// only the (escapeAttr'd) name crosses into the attribute. Reset on every
@@ -1914,7 +1914,7 @@ const getScript = (isTelemetryEnabled: boolean, opencreditsApiUrl: string = 'htt
 			(servers || []).forEach(function(server) {
 				var name = server.name || 'Unknown';
 				var desc = escapeHtml(server.description || 'No description');
-				// #61: no schema guard on the icon URL let a "javascript:" src (or similar)
+				// fork-issue-61: no schema guard on the icon URL let a "javascript:" src (or similar)
 				// through unescaped-but-well-formed -- safeHttpUrl() restricts src= to
 				// http:/https:, falling back to the placeholder instead of a dead src=""
 				var safeIcon = safeHttpUrl(server.icon || '');
@@ -1925,7 +1925,7 @@ const getScript = (isTelemetryEnabled: boolean, opencreditsApiUrl: string = 'htt
 				var starsHtml = stars > 0 ? '' + (stars >= 1000 ? (Math.round(stars / 100) / 10) + 'k' : stars) + ' ★' : '';
 				var typeHtml = installType ? '' + escapeHtml(installType) + '' : '';
 
-				// #57: escapeAttr replaces escapeHtml()+manual "'"->"'" replace, which left
+				// fork-issue-57: escapeAttr replaces escapeHtml()+manual "'"->"'" replace, which left
 				// " unescaped and able to break out of the data-server attribute below.
 				var safeId = escapeAttr(server.id || name);
 				html += '
    ' +
@@ -1973,7 +1973,7 @@ const getScript = (isTelemetryEnabled: boolean, opencreditsApiUrl: string = 'htt
 
 			var name = server.name || 'Unknown';
 			var desc = server.description || 'No description available.';
-			// #61: no schema guard on icon/url let "javascript:" through unescaped-but-
+			// fork-issue-61: no schema guard on icon/url let "javascript:" through unescaped-but-
 			// well-formed -- safeHttpUrl() restricts src=/href= to http:/https:, falling
 			// back to the placeholder / omitting the link instead of a dead attribute.
 			var safeIcon = safeHttpUrl(server.icon || '');
@@ -2078,7 +2078,7 @@ const getScript = (isTelemetryEnabled: boolean, opencreditsApiUrl: string = 'htt
 		function displayMCPServers(servers) {
 			const serversList = document.getElementById('mcpServersList');
 			serversList.innerHTML = '';
-			// #60: reset per render so editMCPServer can never resolve a stale/removed server's
+			// fork-issue-60: reset per render so editMCPServer can never resolve a stale/removed server's
 			// config through a name that no longer has a corresponding button.
 			mcpServerConfigsByName = {};
 
@@ -2117,12 +2117,12 @@ const getScript = (isTelemetryEnabled: boolean, opencreditsApiUrl: string = 'htt
 				}
 
 				const scopeLabel = serverScope === 'global' ? 'Global' : serverScope === 'project' ? 'Project' : 'Extension';
-				// #60: name/config moved out of the inline onclick -- a name containing a single
+				// fork-issue-60: name/config moved out of the inline onclick -- a name containing a single
 				// quote used to break straight out of editMCPServer('...') into the attribute,
 				// and JSON.stringify(config) was a second, un-escapeAttr-able sink. The config
 				// now lives only in mcpServerConfigsByName; editMCPServer looks it up by name,
 				// which itself travels through data-server-name (escapeAttr) + this.dataset,
-				// same pattern as #57/#58.
+				// same pattern as fork-issue-57/fork-issue-58.
 				mcpServerConfigsByName[name] = config;
 				const serverActionsHtml = \`
 						\`;
@@ -2220,7 +2220,7 @@ const getScript = (isTelemetryEnabled: boolean, opencreditsApiUrl: string = 'htt
 			if (moreBtn) moreBtn.style.display = '';
 			if (modelDropdown) modelDropdown.style.display = 'none';
 
-			// #61 follow-up: openCreditsModels is the same third-party-sourced
+			// fork-issue-61 follow-up: openCreditsModels is the same third-party-sourced
 			// data as renderOpenCreditsModelCards()'s model-card sink below -- this function is
 			// safe not because of the data source but because it never builds an HTML string:
 			// setAttribute()/textContent/a real function assigned to .onclick all treat their
@@ -2291,7 +2291,7 @@ const getScript = (isTelemetryEnabled: boolean, opencreditsApiUrl: string = 'htt
 					}
 				}
 
-				// #61 follow-up: openCreditsModels is overwritten wholesale by
+				// fork-issue-61 follow-up: openCreditsModels is overwritten wholesale by
 				// resolveLatestModels() (model-updater.ts) from fetch(apiBaseUrl + '/v1/models')
 				// -- the same third-party endpoint as renderDropdown/renderAllModels -- and
 				// renderOpenCreditsModelCards() runs unconditionally on that update, with no
@@ -2553,7 +2553,7 @@ const getScript = (isTelemetryEnabled: boolean, opencreditsApiUrl: string = 'htt
 				}) : models;
 
 				var html = filtered.slice(0, 50).map(function(m) {
-					// #61: models come from fetch(OPENCREDITS_API_URL + '/v1/models'), a
+					// fork-issue-61: models come from fetch(OPENCREDITS_API_URL + '/v1/models'), a
 					// third-party HTTP endpoint -- data-id is an attribute value (escapeAttr),
 					// the name/id text goes into innerHTML (escapeHtml).
 					return '
    ' +
@@ -2701,7 +2701,7 @@ const getScript = (isTelemetryEnabled: boolean, opencreditsApiUrl: string = 'htt
 				const isSelected = currentModel === model.id;
 				const contextLength = model.context_length ? Math.round(model.context_length / 1000) + 'K' : '';
 
-				// #61: models come from fetch(OPENCREDITS_API_URL + '/v1/models'), a
+				// fork-issue-61: models come from fetch(OPENCREDITS_API_URL + '/v1/models'), a
 				// third-party HTTP endpoint -- data-model-id is an attribute value
 				// (escapeAttr), the name/id/owned_by text goes into innerHTML (escapeHtml).
 				return '
    ' +
@@ -3576,7 +3576,7 @@ const getScript = (isTelemetryEnabled: boolean, opencreditsApiUrl: string = 'htt
 		function copyCodeBlock(codeId) {
 			const codeElement = document.getElementById(codeId);
 			if (codeElement) {
-				// #62: getAttribute() already returns the entity-decoded value (the browser
+				// fork-issue-62: getAttribute() already returns the entity-decoded value (the browser
 				// decodes data-raw-code's escapeAttr()-produced entities during HTML parsing --
 				// see the escapedCode assembly in parseSimpleMarkdown) -- there used to be a
 				// second, manual decode pass here that mangled code literally containing the
@@ -3654,7 +3654,7 @@ const getScript = (isTelemetryEnabled: boolean, opencreditsApiUrl: string = 'htt
 					
 				case 'userInput':
 					if (message.data.trim()) {
-						// #63: raw text except fenced code blocks -- see
+						// fork-issue-63: raw text except fenced code blocks -- see
 						// renderUserMessageContent (near parseSimpleMarkdown).
 						addMessage(renderUserMessageContent(message.data), 'user');
 					}
@@ -3707,7 +3707,7 @@ const getScript = (isTelemetryEnabled: boolean, opencreditsApiUrl: string = 'htt
 					break;
 					
 				case 'yoloModeEnabled':
-					// #59: confirmation only arrives here after the extension host
+					// fork-issue-59: confirmation only arrives here after the extension host
 					// actually persisted permissions.yoloMode (workspace, or global as
 					// fallback) -- the chat message moved here (out of enableYoloMode())
 					// so it can no longer fire before/regardless of that write.
@@ -3716,7 +3716,7 @@ const getScript = (isTelemetryEnabled: boolean, opencreditsApiUrl: string = 'htt
 					break;
 
 				case 'yoloModeEnableFailed':
-					// #59: both the workspace and global config.update() attempts threw
+					// fork-issue-59: both the workspace and global config.update() attempts threw
 					// -- surface it in-chat too, not just via the extension host's native
 					// error notification, and never show the "enabled" message. The raw
 					// host error (message.error) deliberately does NOT go into this
@@ -4212,7 +4212,7 @@ const getScript = (isTelemetryEnabled: boolean, opencreditsApiUrl: string = 'htt
 			menu.style.display = isVisible ? 'none' : 'block';
 		}
 
-		// #52: consolidated enableYoloMode - there used to be two separate
+		// fork-issue-52: consolidated enableYoloMode - there used to be two separate
 		// enableYoloMode function declarations in this scope; the later one
 		// silently won, so the argument-less inline chat button call hit
 		// getElementById('permissionMenu-undefined') and threw. With a
@@ -4227,7 +4227,7 @@ const getScript = (isTelemetryEnabled: boolean, opencreditsApiUrl: string = 'htt
 		// permissions.yoloMode and replies with settingsData, whose handler
 		// (~6147/~6150) sets the checkbox and calls updateYoloWarning().
 		//
-		// #59: the "enabled" chat message used to fire right here, unconditionally,
+		// fork-issue-59: the "enabled" chat message used to fire right here, unconditionally,
 		// the moment the button was clicked -- independent of whether
 		// _enableYoloMode() on the extension host actually managed to persist
 		// anything (it had no global fallback, so with no workspace folder open the
@@ -4534,7 +4534,7 @@ const getScript = (isTelemetryEnabled: boolean, opencreditsApiUrl: string = 'htt
 
 		updateStatus('Initializing...', 'disconnected');
 
-		// #55: restoreCodeBlockPlaceholders (the call site is further down in
+		// fork-issue-55: restoreCodeBlockPlaceholders (the call site is further down in
 		// parseSimpleMarkdown) -- build-time splice (same pattern as math-script/collapse-script/
 		// html-escape) so npm run test:markdown-restore can exercise the function under
 		// Node. The next line splices the compiled function source via toString()
@@ -4545,10 +4545,10 @@ const getScript = (isTelemetryEnabled: boolean, opencreditsApiUrl: string = 'htt
 		// __CODEBLOCK_N__ placeholder and returning the already-rendered code-block HTML
 		// (collapse wrapper, language label, copy button, data-raw-code) for each -- shared by
 		// parseSimpleMarkdown (Claude/thinking messages, full markdown) and
-		// renderUserMessageContent (#63, revised: user messages stay raw text except fenced code
-		// blocks, which keep the #48 collapse/copy-button/language-label treatment). Pure
+		// renderUserMessageContent (fork-issue-63, revised: user messages stay raw text except fenced code
+		// blocks, which keep the fork-issue-48 collapse/copy-button/language-label treatment). Pure
 		// extraction out of parseSimpleMarkdown -- same regex, same per-block HTML as before, so
-		// #62's getAttribute('data-raw-code')-via-escapeAttr guarantee still holds for both
+		// fork-issue-62's getAttribute('data-raw-code')-via-escapeAttr guarantee still holds for both
 		// callers, each restoring its own __CODEBLOCK_N__ placeholders afterwards.
 		function extractCodeBlocks(markdown) {
 			// Store code blocks temporarily to protect them from further processing
@@ -4570,11 +4570,11 @@ const getScript = (isTelemetryEnabled: boolean, opencreditsApiUrl: string = 'htt
 
 				// Create unique ID for this code block
 				const codeId = 'code_' + Math.random().toString(36).substr(2, 9);
-				// #57: escapeAttr (was escapeHtml() + a manual "\"" -> """ patch that left
+				// fork-issue-57: escapeAttr (was escapeHtml() + a manual "\"" -> """ patch that left
 				// "'" unescaped) for the data-raw-code attribute below.
 				const escapedCode = escapeAttr(code);
 
-				// #48 (upstream #151): blocks over the threshold become 
    ;
+				// fork-issue-48 (upstream #151): blocks over the threshold become 
    ;
 				// shorter ones stay character-for-character as before.
 				const collapseInfo = evaluateCodeBlockCollapse(code, collapseCodeBlockLines);
 				const copyGuard = collapseInfo.collapse ? 'event.preventDefault();event.stopPropagation();' : '';
@@ -4607,7 +4607,7 @@ const getScript = (isTelemetryEnabled: boolean, opencreditsApiUrl: string = 'htt
 			let processedMarkdown = codeBlockExtraction.text;
 			const codeBlockPlaceholders = codeBlockExtraction.placeholders;
 
-			// #40 (upstream #63): escape raw HTML in the remaining prose before any
+			// fork-issue-40 (upstream #63): escape raw HTML in the remaining prose before any
 			// further markdown processing. contentDiv.innerHTML = content (addMessage)
 			// renders this output as real DOM, so an unescaped tag like "