From c3108d9ef98dfe72f6b0cc12aeacf4406af9927e Mon Sep 17 00:00:00 2001 From: Emre Sokullu Date: Mon, 12 Oct 2026 02:27:50 +0300 Subject: [PATCH] Fix clarify card showing literal backslash-quote escapes --- src/chrome/src/agent/agent.js | 15 +++++++----- src/chrome/src/agent/text-sanitize.js | 32 ++++++++++++++++++++++++++ src/chrome/src/agent/tools.js | 4 ++-- src/chrome/src/ui/sidepanel.js | 23 ++++++++++++++++++ src/firefox/src/agent/agent.js | 15 +++++++----- src/firefox/src/agent/text-sanitize.js | 32 ++++++++++++++++++++++++++ src/firefox/src/agent/tools.js | 4 ++-- src/firefox/src/ui/sidepanel.js | 23 ++++++++++++++++++ test/run.js | 31 +++++++++++++++++++++++-- 9 files changed, 161 insertions(+), 18 deletions(-) diff --git a/src/chrome/src/agent/agent.js b/src/chrome/src/agent/agent.js index 35cc1a7fd3..27152b7469 100644 --- a/src/chrome/src/agent/agent.js +++ b/src/chrome/src/agent/agent.js @@ -156,7 +156,7 @@ import { resolveMaxOutputTokens } from '../providers/context-windows.js'; import { generateImage, readMediaConfig } from './generative-media.js'; import { GENERATIVE_MEDIA_SETUP_NOTE, isImageGenConfigured, mediaPermissionUrl, validateMediaConfig } from './media-config.js'; import { extractFirstJsonObject } from './json-extract.js'; -import { repairAssistantDisplayText, sanitizeText as sanitizePlannerText } from './text-sanitize.js'; +import { repairAssistantDisplayText, repairClarifyText, sanitizeText as sanitizePlannerText } from './text-sanitize.js'; import { emptyOutputFailureMessage, modelOutputDiagnostics } from './model-output-diagnostics.js'; import { buildCustomSkillsPrompt, buildSkillLoaderDefinition, buildSkillToolDefinitions, buildSkillToolRegistry, getEligibleCustomSkills, getEligibleSkillCatalog, normalizeCustomSkills } from './skills.js'; import { OTP_EMAIL_PROVIDER_IDS, OTP_EMAIL_SKILL_ID, OTP_EMAIL_TOOL_NAME, otpEmailCandidates, otpEmailProviderForUrl, otpEmailUrlLooksLikeMessage, otpOpenMessageRootRef, otpRedactRefs, otpServiceDisplay, otpServiceKey, otpVerificationMessageExcerpt, selectOtpMailboxTab, selectUniqueOtpCandidateByPreview } from './otp-email-tool.js'; @@ -38167,18 +38167,21 @@ If the user has already named or confirmed this exact recipient, do NOT ask agai // Permission and form-submit prompts use separate helpers and never time out. if (name === 'clarify') { const clarificationGuard = this._planExecutionGuards.get(tabId); - const question = String(args?.question || '').trim(); + // Models sometimes copy a JSON-escaped post body into clarify text, + // leaving literal `\"` after the tool-call JSON is parsed. The card + // renders via textContent, so repair before display/storage. + const question = repairClarifyText(String(args?.question || '').trim()).slice(0, 600); if (!question) { return { success: false, error: 'clarify: `question` is required (a single sentence asking the user something specific).' }; } const rawOptions = Array.isArray(args?.options) ? args.options.slice(0, 4) : []; - const normalizedOptions = rawOptions.map(s => String(s).trim().slice(0, 200)); + const normalizedOptions = rawOptions.map(s => repairClarifyText(String(s).trim()).slice(0, 200)); const hasVisibleOptionContent = option => /[\p{L}\p{N}\p{P}\p{S}]/u.test(option.replace(/[\p{Default_Ignorable_Code_Point}\u2800]/gu, '')); const options = normalizedOptions.every(hasVisibleOptionContent) ? normalizedOptions : []; const safeFirst = args?.safe_first === true && options.length > 0; - const reason = args?.reason ? String(args.reason).slice(0, 300) : null; + const reason = args?.reason ? repairClarifyText(String(args.reason)).slice(0, 300) : null; const purpose = args?.purpose === 'research_escalation' ? 'research_escalation' : (args?.purpose === 'recipient_change' @@ -38205,8 +38208,8 @@ If the user has already named or confirmed this exact recipient, do NOT ask agai } const isResearchEscalation = purpose === 'research_escalation'; const requireExplicitAnswer = isResearchEscalation || args?.require_explicit_answer === true; - const researchRequest = isResearchEscalation ? normalizeResearchRequest(args?.research_request) : ''; - const approveOption = isResearchEscalation ? String(args?.approve_option || '').trim().slice(0, 200) : ''; + const researchRequest = isResearchEscalation ? repairClarifyText(normalizeResearchRequest(args?.research_request)) : ''; + const approveOption = isResearchEscalation ? repairClarifyText(String(args?.approve_option || '').trim()).slice(0, 200) : ''; if (isResearchEscalation) { if (!this.researchEscalationEnabled) { return { success: false, error: 'Research escalation is disabled in Settings. Continue the research locally.' }; diff --git a/src/chrome/src/agent/text-sanitize.js b/src/chrome/src/agent/text-sanitize.js index 2bc1f616d4..fdd505943e 100644 --- a/src/chrome/src/agent/text-sanitize.js +++ b/src/chrome/src/agent/text-sanitize.js @@ -142,3 +142,35 @@ function repairJsonQuotedPageTitleLines(value) { export function repairAssistantDisplayText(value) { return repairJsonQuotedPageTitleLines(repairDoubleEscapedAssistantText(value)); } + +/** + * Strip one extra JSON-escaping layer from clarify question/options/reason. + * + * Models sometimes copy a JSON-escaped post body into a clarify tool call + * (e.g. Publish: "Just finished \"The Last Economy\" ..."), leaving literal + * `\"` sequences after the tool-call JSON itself has already been parsed. + * The clarify card renders via textContent, so those backslashes show up + * verbatim in the UI ("\" ??" bug). Runs of backslashes immediately before a + * single or double quote are therefore treated as display noise and removed. + * + * Known edge (accepted): a trailing path separator before a closing quote + * (e.g. Save to "C:\Temp\") loses its slash and renders as "C:\Temp". That + * prose is rare in single-sentence clarify prompts and stays readable; + * showing literal `\"` in every publish confirmation is worse. + * + * A surviving `\\` (double backslash) is the same over-escaping artifact for + * a literal backslash, so it is collapsed once — except a UNC leading `\\` + * (start of string or after whitespace/quote/paren/bracket + word char), + * which is preserved. Lone single backslashes (e.g. tab\tx) are left alone. + */ +export function repairClarifyText(value) { + if (typeof value !== 'string' || !value) return value == null ? '' : String(value); + if (!value.includes('\\')) return value; + const stripped = value.replace(/\\+(?=["'])/g, ''); + return stripped.replace(/\\\\/g, (match, offset, str) => { + const before = offset === 0 ? '' : str[offset - 1]; + const after = str[offset + 2] || ''; + if ((offset === 0 || /[\s"'(\[{]/.test(before)) && /[A-Za-z0-9]/.test(after)) return match; + return '\\'; + }); +} diff --git a/src/chrome/src/agent/tools.js b/src/chrome/src/agent/tools.js index 74e8308d42..d8c7d6d8f2 100644 --- a/src/chrome/src/agent/tools.js +++ b/src/chrome/src/agent/tools.js @@ -883,12 +883,12 @@ export const AGENT_TOOLS = [ properties: { question: { type: 'string', - description: 'The question to show the user. One sentence, plain English. Frame it as a real either/or, not "is this OK?" — e.g. "Did you mean the WordPress REST API application password, or a specific plugin\'s API key?" not "Should I proceed?"', + description: 'The question to show the user. One sentence, plain English. Frame it as a real either/or, not "is this OK?" — e.g. "Did you mean the WordPress REST API application password, or a specific plugin\'s API key?" not "Should I proceed?" Write plain text with real quotation marks only — never include backslash escapes like \\". Stray escapes are stripped before display, so include only characters you want shown.', }, options: { type: 'array', items: { type: 'string' }, - description: 'Optional list of 2-4 suggested answers shown as buttons. The user can also type a custom answer. If the user does not answer before the clarify timeout, options[0] is auto-selected — put the safe/default choice first. Omit if the question is genuinely open-ended (timeout then yields a non-confirmation timeout marker, not user approval).', + description: 'Optional list of 2-4 suggested answers shown as buttons. The user can also type a custom answer. If the user does not answer before the clarify timeout, options[0] is auto-selected — put the safe/default choice first. Omit if the question is genuinely open-ended (timeout then yields a non-confirmation timeout marker, not user approval). Write plain text with real quotation marks only — never include backslash escapes like \\". Stray escapes are stripped before display.', }, safe_first: { type: 'boolean', diff --git a/src/chrome/src/ui/sidepanel.js b/src/chrome/src/ui/sidepanel.js index 6e1266f173..78fb442ede 100644 --- a/src/chrome/src/ui/sidepanel.js +++ b/src/chrome/src/ui/sidepanel.js @@ -10935,6 +10935,29 @@ function workflowHealingTargetLabel(target) { function renderClarifyCard(data) { // A clarification / permission card needs the user's attention right now. + // Repair display-side too so cards persisted before the agent-side fix (or + // arriving via an older background) no longer show literal `\"`. Inlined + // (no external helper) so function-extraction test harnesses keep working. + if (data && typeof data === 'object') { + // Mirrors repairClarifyText agent-side, inlined so function-extraction + // test harnesses keep working. Preserves UNC leading `\\`. + const cleanClarify = v => { + if (typeof v !== 'string' || !v.includes('\\')) return v; + const stripped = v.replace(/\\+(?=["'])/g, ''); + return stripped.replace(/\\\\/g, (m, offset, str) => { + const before = offset === 0 ? '' : str[offset - 1]; + const after = str[offset + 2] || ''; + if ((offset === 0 || /[\s"'(\[{]/.test(before)) && /[A-Za-z0-9]/.test(after)) return m; + return '\\'; + }); + }; + if (typeof data.question === 'string') data = { ...data, question: cleanClarify(data.question) }; + if (typeof data.reason === 'string') data = { ...data, reason: cleanClarify(data.reason) }; + if (Array.isArray(data.options)) data = { ...data, options: data.options.map(opt => typeof opt === 'string' ? cleanClarify(opt) : opt) }; + if (data.researchEscalation && typeof data.researchEscalation.request === 'string') { + data = { ...data, researchEscalation: { ...data.researchEscalation, request: cleanClarify(data.researchEscalation.request) } }; + } + } const tabId = data?.scheduledTabId ?? data?.tabId ?? currentTabId; if (tabId == null) return; const scheduledJobId = data?.scheduledJobId ? String(data.scheduledJobId) : ''; diff --git a/src/firefox/src/agent/agent.js b/src/firefox/src/agent/agent.js index 32ff71ad83..f175aed0e0 100644 --- a/src/firefox/src/agent/agent.js +++ b/src/firefox/src/agent/agent.js @@ -149,7 +149,7 @@ import { resolveMaxOutputTokens } from '../providers/context-windows.js'; import { generateImage, readMediaConfig } from './generative-media.js'; import { GENERATIVE_MEDIA_SETUP_NOTE, isImageGenConfigured, mediaPermissionUrl, validateMediaConfig } from './media-config.js'; import { extractFirstJsonObject } from './json-extract.js'; -import { repairAssistantDisplayText, sanitizeText as sanitizePlannerText } from './text-sanitize.js'; +import { repairAssistantDisplayText, repairClarifyText, sanitizeText as sanitizePlannerText } from './text-sanitize.js'; import { emptyOutputFailureMessage, modelOutputDiagnostics } from './model-output-diagnostics.js'; import { buildCustomSkillsPrompt, buildSkillLoaderDefinition, buildSkillToolDefinitions, buildSkillToolRegistry, getEligibleCustomSkills, getEligibleSkillCatalog, normalizeCustomSkills } from './skills.js'; import { OTP_EMAIL_PROVIDER_IDS, OTP_EMAIL_SKILL_ID, OTP_EMAIL_TOOL_NAME, otpEmailCandidates, otpEmailProviderForUrl, otpEmailUrlLooksLikeMessage, otpOpenMessageRootRef, otpRedactRefs, otpServiceDisplay, otpServiceKey, otpVerificationMessageExcerpt, selectOtpMailboxTab, selectUniqueOtpCandidateByPreview } from './otp-email-tool.js'; @@ -34704,18 +34704,21 @@ Rules: no prose intro, no conclusion, no "this screenshot shows...", no layout d // helpers and never time out. if (name === 'clarify') { const clarificationGuard = this._planExecutionGuards.get(tabId); - const question = String(args?.question || '').trim(); + // Models sometimes copy a JSON-escaped post body into clarify text, + // leaving literal `\"` after the tool-call JSON is parsed. The card + // renders via textContent, so repair before display/storage. + const question = repairClarifyText(String(args?.question || '').trim()).slice(0, 600); if (!question) { return { success: false, error: 'clarify: `question` is required (a single sentence asking the user something specific).' }; } const rawOptions = Array.isArray(args?.options) ? args.options.slice(0, 4) : []; - const normalizedOptions = rawOptions.map(s => String(s).trim().slice(0, 200)); + const normalizedOptions = rawOptions.map(s => repairClarifyText(String(s).trim()).slice(0, 200)); const hasVisibleOptionContent = option => /[\p{L}\p{N}\p{P}\p{S}]/u.test(option.replace(/[\p{Default_Ignorable_Code_Point}\u2800]/gu, '')); const options = normalizedOptions.every(hasVisibleOptionContent) ? normalizedOptions : []; const safeFirst = args?.safe_first === true && options.length > 0; - const reason = args?.reason ? String(args.reason).slice(0, 300) : null; + const reason = args?.reason ? repairClarifyText(String(args.reason)).slice(0, 300) : null; const purpose = args?.purpose === 'research_escalation' ? 'research_escalation' : (args?.purpose === 'recipient_change' @@ -34742,8 +34745,8 @@ Rules: no prose intro, no conclusion, no "this screenshot shows...", no layout d } const isResearchEscalation = purpose === 'research_escalation'; const requireExplicitAnswer = isResearchEscalation || args?.require_explicit_answer === true; - const researchRequest = isResearchEscalation ? normalizeResearchRequest(args?.research_request) : ''; - const approveOption = isResearchEscalation ? String(args?.approve_option || '').trim().slice(0, 200) : ''; + const researchRequest = isResearchEscalation ? repairClarifyText(normalizeResearchRequest(args?.research_request)) : ''; + const approveOption = isResearchEscalation ? repairClarifyText(String(args?.approve_option || '').trim()).slice(0, 200) : ''; if (isResearchEscalation) { if (!this.researchEscalationEnabled) { return { success: false, error: 'Research escalation is disabled in Settings. Continue the research locally.' }; diff --git a/src/firefox/src/agent/text-sanitize.js b/src/firefox/src/agent/text-sanitize.js index 2bc1f616d4..fdd505943e 100644 --- a/src/firefox/src/agent/text-sanitize.js +++ b/src/firefox/src/agent/text-sanitize.js @@ -142,3 +142,35 @@ function repairJsonQuotedPageTitleLines(value) { export function repairAssistantDisplayText(value) { return repairJsonQuotedPageTitleLines(repairDoubleEscapedAssistantText(value)); } + +/** + * Strip one extra JSON-escaping layer from clarify question/options/reason. + * + * Models sometimes copy a JSON-escaped post body into a clarify tool call + * (e.g. Publish: "Just finished \"The Last Economy\" ..."), leaving literal + * `\"` sequences after the tool-call JSON itself has already been parsed. + * The clarify card renders via textContent, so those backslashes show up + * verbatim in the UI ("\" ??" bug). Runs of backslashes immediately before a + * single or double quote are therefore treated as display noise and removed. + * + * Known edge (accepted): a trailing path separator before a closing quote + * (e.g. Save to "C:\Temp\") loses its slash and renders as "C:\Temp". That + * prose is rare in single-sentence clarify prompts and stays readable; + * showing literal `\"` in every publish confirmation is worse. + * + * A surviving `\\` (double backslash) is the same over-escaping artifact for + * a literal backslash, so it is collapsed once — except a UNC leading `\\` + * (start of string or after whitespace/quote/paren/bracket + word char), + * which is preserved. Lone single backslashes (e.g. tab\tx) are left alone. + */ +export function repairClarifyText(value) { + if (typeof value !== 'string' || !value) return value == null ? '' : String(value); + if (!value.includes('\\')) return value; + const stripped = value.replace(/\\+(?=["'])/g, ''); + return stripped.replace(/\\\\/g, (match, offset, str) => { + const before = offset === 0 ? '' : str[offset - 1]; + const after = str[offset + 2] || ''; + if ((offset === 0 || /[\s"'(\[{]/.test(before)) && /[A-Za-z0-9]/.test(after)) return match; + return '\\'; + }); +} diff --git a/src/firefox/src/agent/tools.js b/src/firefox/src/agent/tools.js index a3f65f8144..ab5d77cca3 100644 --- a/src/firefox/src/agent/tools.js +++ b/src/firefox/src/agent/tools.js @@ -745,12 +745,12 @@ export const AGENT_TOOLS = [ properties: { question: { type: 'string', - description: 'The question to show the user. One sentence, plain English. Frame it as a real either/or, not "is this OK?" — e.g. "Did you mean the WordPress REST API application password, or a specific plugin\'s API key?" not "Should I proceed?"', + description: 'The question to show the user. One sentence, plain English. Frame it as a real either/or, not "is this OK?" — e.g. "Did you mean the WordPress REST API application password, or a specific plugin\'s API key?" not "Should I proceed?" Write plain text with real quotation marks only — never include backslash escapes like \\". Stray escapes are stripped before display, so include only characters you want shown.', }, options: { type: 'array', items: { type: 'string' }, - description: 'Optional list of 2-4 suggested answers shown as buttons. The user can also type a custom answer. If the user does not answer before the clarify timeout, options[0] is auto-selected — put the safe/default choice first. Omit if the question is genuinely open-ended (timeout then yields a non-confirmation timeout marker, not user approval).', + description: 'Optional list of 2-4 suggested answers shown as buttons. The user can also type a custom answer. If the user does not answer before the clarify timeout, options[0] is auto-selected — put the safe/default choice first. Omit if the question is genuinely open-ended (timeout then yields a non-confirmation timeout marker, not user approval). Write plain text with real quotation marks only — never include backslash escapes like \\". Stray escapes are stripped before display.', }, safe_first: { type: 'boolean', diff --git a/src/firefox/src/ui/sidepanel.js b/src/firefox/src/ui/sidepanel.js index da99b396ba..7c886a3fbb 100644 --- a/src/firefox/src/ui/sidepanel.js +++ b/src/firefox/src/ui/sidepanel.js @@ -10444,6 +10444,29 @@ function workflowHealingTargetLabel(target) { } function renderClarifyCard(data) { + // Repair display-side too so cards persisted before the agent-side fix (or + // arriving via an older background) no longer show literal `\"`. Inlined + // (no external helper) so function-extraction test harnesses keep working. + if (data && typeof data === 'object') { + // Mirrors repairClarifyText agent-side, inlined so function-extraction + // test harnesses keep working. Preserves UNC leading `\\`. + const cleanClarify = v => { + if (typeof v !== 'string' || !v.includes('\\')) return v; + const stripped = v.replace(/\\+(?=["'])/g, ''); + return stripped.replace(/\\\\/g, (m, offset, str) => { + const before = offset === 0 ? '' : str[offset - 1]; + const after = str[offset + 2] || ''; + if ((offset === 0 || /[\s"'(\[{]/.test(before)) && /[A-Za-z0-9]/.test(after)) return m; + return '\\'; + }); + }; + if (typeof data.question === 'string') data = { ...data, question: cleanClarify(data.question) }; + if (typeof data.reason === 'string') data = { ...data, reason: cleanClarify(data.reason) }; + if (Array.isArray(data.options)) data = { ...data, options: data.options.map(opt => typeof opt === 'string' ? cleanClarify(opt) : opt) }; + if (data.researchEscalation && typeof data.researchEscalation.request === 'string') { + data = { ...data, researchEscalation: { ...data.researchEscalation, request: cleanClarify(data.researchEscalation.request) } }; + } + } const tabId = data?.scheduledTabId ?? data?.tabId ?? currentTabId; if (tabId == null) return; const scheduledJobId = data?.scheduledJobId ? String(data.scheduledJobId) : ''; diff --git a/test/run.js b/test/run.js index efd6cccce1..7ad46f2857 100644 --- a/test/run.js +++ b/test/run.js @@ -1187,10 +1187,10 @@ const { repairDoubleEscapedAssistantText: repairDoubleEscapedAssistantTextCh } = const { repairDoubleEscapedAssistantText: repairDoubleEscapedAssistantTextFx } = await import( 'file://' + path.join(ROOT, 'src/firefox/src/agent/text-sanitize.js').replace(/\\/g, '/') ); -const { repairAssistantDisplayText: repairAssistantDisplayTextCh } = await import( +const { repairAssistantDisplayText: repairAssistantDisplayTextCh, repairClarifyText: repairClarifyTextCh } = await import( 'file://' + path.join(ROOT, 'src/chrome/src/agent/text-sanitize.js').replace(/\\/g, '/') ); -const { repairAssistantDisplayText: repairAssistantDisplayTextFx } = await import( +const { repairAssistantDisplayText: repairAssistantDisplayTextFx, repairClarifyText: repairClarifyTextFx } = await import( 'file://' + path.join(ROOT, 'src/firefox/src/agent/text-sanitize.js').replace(/\\/g, '/') ); const userMemoryCh = await import( @@ -71836,6 +71836,33 @@ test('assistant display repair rejects page-title layout and control characters' } }); +test('clarify repair strips JSON over-escaping but preserves UNC and lone backslashes', () => { + const cases = [ + // Reported bug: model copies JSON-escaped post body into clarify text. + ['Do you want to publish this post: "Just finished \\"The Last Economy\\" by @EMostaque?"', + 'Do you want to publish this post: "Just finished "The Last Economy" by @EMostaque?"'], + ['Publish: "Just finished \\"The Last Economy\\" by @EMostaque"', + 'Publish: "Just finished "The Last Economy" by @EMostaque"'], + ["it's", "it's"], + ['plain question?', 'plain question?'], + ['tab\\tx', 'tab\\tx'], + // Over-escaped backslash collapses once ... + ['C:\\\\Users\\\\name', 'C:\\Users\\name'], + // ... except a UNC leading `\\` is preserved. + ['\\\\server\\share', '\\\\server\\share'], + ['see \\\\server\\share', 'see \\\\server\\share'], + // Non-string / empty edge cases. + [null, ''], + ['', ''], + ]; + for (const [label, repair] of [['chrome', repairClarifyTextCh], ['firefox', repairClarifyTextFx]]) { + for (const [input, expected] of cases) { + assert.equal(repair(input), expected, `${label}: repairClarifyText(${JSON.stringify(input)})`); + } + } + assert.equal(repairClarifyTextCh.toString(), repairClarifyTextFx.toString(), 'chrome/firefox repairClarifyText stay mirrored'); +}); + test('provider response path preserves raw assistant content and metadata', async () => { const expected = '**Result**\n\n- First\n- Second'; const malformed = JSON.stringify(expected).slice(1, -1);