mirror of
https://github.com/openclaw/openclaw.git
synced 2026-10-03 09:39:25 +00:00
fix: outbound plain-text replies drop unspaced comparison prose (#152671)
Closes #152508 ## What Problem This Solves Fixes: plain-text replies silently lose comparison prose between an unspaced identifier comparison such as `attempts<max` and a later `>`. ## User Impact User impact: the reported comparison guidance arrives intact, while known, custom-element and qualified HTML tags retain their existing stripping behavior. Numeric comparisons such as `attempts<3` already worked. No configuration or public API changes. ## Why This Change Was Made The shared sanitizer retains main's tag matcher exactly. A narrow exclusion preserves an unspaced left operand followed by attribute-free prose containing a conjunction (`and`, `or`, `且`) or clause punctuation and a numeric right operand, such as `attempts<max and wait>5s`. The comparison name must be a simple identifier, optionally followed by one sentence-ending period; internal dots, colons, and hyphens retain main's stripping. A matching closing tag prevents the exclusion. **Ayaan's decision: standard HTML element names take precedence over the comparison exception**, case-insensitively, using one local name list with no new dependency. This keeps `foo<span and wait>5` stripped to `foo5`. Known limitation: unspaced comparisons without a conjunction or clause punctuation are still treated as markup, same as main. Comparisons using standard element names also retain main's stripping: `x<b and y>2` becomes `x2`. Main's `range a<b-c>d` → `range ad` behavior remains unchanged. ## Evidence ### Delivered text Isolated Gateway turns used a scripted local OpenAI-compatible HTTP provider and the actual IRC plugin connected to a local TCP server. Captured `PRIVMSG` bytes are the delivered channel text, not a mocked sanitizer callback. The baseline used the exact sanitizer from main `8af7f3e640`; each run had separate state and a fresh inbound turn. This capture predates the matcher redesign; the differential proof below verifies the same prose outcomes with the redesigned owner. Provider response: ```html retry if attempts<3 and wait>5s retry if attempts<max and wait>5s <b>bold</b> <script>alert(1)</script> <a href="https://example.com">link</a><br><img src=x onerror=alert(1)>done ``` Before, delivered: ```text retry if attempts<3 and wait>5s retry if attempts5s *bold* alert(1) link done ``` After, delivered: ```text retry if attempts<3 and wait>5s retry if attempts<max and wait>5s *bold* alert(1) link done ``` IRC normalizes line breaks to spaces. Script contents remain plain text as before; script tags are removed. This is real Gateway/provider/IRC protocol proof against local scripted peers, not an external IRC account or Telegram Test Server. ### Telegram compatibility Telegram (test server, current head): comparison text and markup handling unchanged vs main, so Telegram's DM path doesn't lose this text on main; the fix doesn't regress it. The red/green is the IRC plain-text capture plus the differential. At `d56972eda33ee08cfdd580737538ecb511019baa`, an isolated source Gateway with `richMessages: false`, a scripted model, and a Convex-leased Test Server bot delivered this exact text to the real-user TDLib recorder: ```text retry if attempts<max and wait>5s bold ``` The provider supplied the comparison line followed by `<b>bold</b>` on the next line. The current-head reply arrived at 26.654 s; three `POST /v1/responses` requests were recorded. A second run substituting the exact main sanitizer (`59100c2655`) in the same source Gateway also received the same text, at 17.188 s. Each run used its own isolated state and live credential lease. Both runs deleted their one QA-user message and one SUT reply using the user's captured receipts before releasing the lease; both exited successfully with credential scratch removed. No bot tokens or chat IDs are included here. The Telegram proof carries over: the channel path and issue example's output are unchanged; later predicate repairs retain the same six explicit prose differences in the expanded differential. ### Differential and focused checks - Generated differential execution against the exact main sanitizer: **2,675 unique inputs, zero unexpected differences, six explicit non-element prose inputs preserved**. The 2,520-case cross product covers every known/custom/qualified/void name shape against all 12 attribute forms, including `and`, `or`, `and wait`, and `or wait`, plus word/numeric/empty/emoji/CJK content, word/space/start adjacency, and paired/unpaired forms. Existing literals and numeric-adjacent qualified-tag regressions are included. Exact differences remain only the two original issue paragraphs (operands `max` and `budget.`), `attempts<max and wait>5s`, the CJK `max` case, the emoji `limit` case, and the issue's `budget.` cross-fence paragraph; each is restored verbatim. All other 2,669 outputs equal main, including quoted `>` handling. - Negative controls demonstrated the original comparison loss and each previous review regression before repair. - Sanitizer: 141 tests passed. Shared delivery: 251 tests passed. Earlier unchanged-path checks: iMessage behavior 289; Telegram adapter five. - Full-sanitizer adversarial smoke: 50 KB quote-overlap, 10 KB whitespace, 90 KB repeated attributes, and 90 KB comparison prose each completed below 5 ms locally. - The malformed input `x<max` + whitespace + `= and wait>5` exposed overlapping regex quantifiers during final validation. The 40k-space regression failed at 1,046 ms before repair; disjoint whitespace/prose branches reduce the 10k/20k/40k full-sanitizer probes to 1.93/1.84/3.08 ms, comparable to main's 5.16/2.09/2.77 ms. Bounded 10k/40k regressions now pass (500 ms allowance), and the differential JSON is byte-identical. - Targeted formatting and patch whitespace checks passed. - The corrected regex-capture access passed all 25 selected core tsgo test graphs and the plugin-SDK declaration graph used by the failing extension-boundary lane. The 1,834-input differential output remained identical. ### Measured test cost Same local Node 24.21.0 checkout and single-file command, `node scripts/run-vitest.mjs run src/infra/outbound/sanitize-text.test.ts`: main source/tests ran 103 tests in **14.90 s command wall time** (**12.52 s Vitest duration**, including transform/import). These are single observations, not a statistically controlled performance claim. The final 141-test sanitizer file ran in **16.82 s Vitest duration**, versus **12.52 s** for main's 103-test file: an observed **+4.30 s** difference, with 91% of the final run spent in transformation. The combined sanitizer, delivery, and targeted-format command took **28.72 s**. These runs include differing post-merge worker graphs, so this is observed wall cost, not a causal CPU-cost estimate. Hosted CI: the 138 sanitizer cases passed in [run 36224078489, `checks-node-changed-compact-large-39`](https://github.com/openclaw/openclaw/actions/runs/36224078489/job/108355098309). Their reported execution durations sum to **0.037 s** in the Bun `core-unit-fast-2` lane, at millisecond reporting precision. That sum excludes import, transformation, and job setup and is not presented as whole-file wall time. This run preceded the type-only correction; its separate type failures were subsequently fixed and verified locally in the corresponding graphs. Co-authored-by: Ayaan Zaidi <hi@obviy.us>
This commit is contained in:
leilei3167
GitHub
parent
4543ce2515
commit
8778b46ccf
2 changed files with 108 additions and 4 deletions
|
|
@ -137,6 +137,8 @@ describe("sanitizeForPlainText", () => {
|
|||
it("strips unknown/remaining tags", () => {
|
||||
expect(sanitizeForPlainText('<span class="x">text</span>')).toBe("text");
|
||||
expect(sanitizeForPlainText('<a href="https://example.com">link</a>')).toBe("link");
|
||||
expect(sanitizeForPlainText("<script>alert(1)</script>")).toBe("alert(1)");
|
||||
expect(sanitizeForPlainText("<img src=x onerror=alert(1)>visible")).toBe("visible");
|
||||
});
|
||||
|
||||
it("strips colon- and dot-qualified tags", () => {
|
||||
|
|
@ -309,6 +311,67 @@ describe("sanitizeForPlainText", () => {
|
|||
expect(sanitizeForPlainText("a < b && c > d")).toBe("a < b && c > d");
|
||||
});
|
||||
|
||||
it.each([
|
||||
"Guard the retry loop: only retry while attempts<max and backoffMs>0, otherwise give up.",
|
||||
"Set the threshold so that latency<budget. Then verify the p99 stays flat, confirm the alert fires, and only after that raise concurrency>4.",
|
||||
"Use timeout<300 and n>0 for the probe.",
|
||||
"a<b",
|
||||
"x<3 && y>2",
|
||||
"1<2>0",
|
||||
"retry if attempts<3 and wait>5s",
|
||||
"attempts<max and wait>5s",
|
||||
"重试次数<max 且等待>5秒",
|
||||
"🙂<limit and wait>5s",
|
||||
"Set latency<budget. Then check:\n\n```\nif (a<b) { return c>d; }\n```\n\nand confirm concurrency>4 is safe.",
|
||||
])("preserves unspaced comparison prose in %s", (input) => {
|
||||
expect(sanitizeForPlainText(input)).toBe(input);
|
||||
});
|
||||
|
||||
it.each([10_000, 40_000])("bounds malformed comparison scanning with %i spaces", (size) => {
|
||||
const input = `x<max${" ".repeat(size)}= and wait>5`;
|
||||
const started = process.hrtime.bigint();
|
||||
const sanitized = sanitizeForPlainText(input);
|
||||
const elapsedMs = Number(process.hrtime.bigint() - started) / 1e6;
|
||||
|
||||
expect(sanitized).toBe("x5");
|
||||
expect(elapsedMs).toBeLessThan(500);
|
||||
});
|
||||
|
||||
it.each([
|
||||
["checkbox-after-value", 'x^2 • <input type="checkbox" checked/>done', "x^2 • done"],
|
||||
["boolean-after-value", '<input type="checkbox" disabled/>todo', "todo"],
|
||||
["boolean-only", "<input disabled/>todo", "todo"],
|
||||
["boolean-first", '<input checked type="checkbox"/>done', "done"],
|
||||
["interleaved", '<input checked type="checkbox" disabled/>done', "done"],
|
||||
["autofocus-after-value", '<input type="text" autofocus/>ready', "ready"],
|
||||
["controls-after-value", '<video src="clip.mp4" controls/>play', "play"],
|
||||
["autoplay-after-value", '<audio src="clip.mp3" autoplay/>now', "now"],
|
||||
["bare-custom", "<span data-x>text</span>", "text"],
|
||||
["mixed-custom", "<div hidden data-id=1>text</div>", "\ntext\n"],
|
||||
["download", "<a href=x download>file</a>", "file"],
|
||||
["custom-element-boolean", "<custom-element hidden>text</custom-element>", "text"],
|
||||
["custom-element-bare", "<custom-element data-x>text</custom-element>", "text"],
|
||||
["custom-element-empty", "<my-widget hidden>", ""],
|
||||
["qualified-bare", "<vendor:note data-x>text</vendor:note>", "text"],
|
||||
["unpaired-dot-qualified-clause", "foo<vendor.note and wait>5", "foo5"],
|
||||
["adjacent-numeric", "foo<span data-x>5</span>", "foo5"],
|
||||
["paired-clause", "foo<span and wait>5</span>", "foo5"],
|
||||
["void-numeric", "foo<img hidden>5", "foo5"],
|
||||
["multiple-bare-numeric", "foo<input disabled checked>5", "foo5"],
|
||||
["unpaired-clause", "foo<span and wait>5", "foo5"],
|
||||
["uppercase-unpaired-clause", "foo<SPAN and wait>5", "foo5"],
|
||||
])("strips or converts tags with bare attributes (%s)", (_name, input, expected) => {
|
||||
expect(sanitizeForPlainText(input)).toBe(expected);
|
||||
});
|
||||
|
||||
it.each([
|
||||
["range a<b-c>d", "range ad"],
|
||||
["attempts<max threshold>5s", "attempts5s"],
|
||||
["x<b and y>2", "x2"],
|
||||
])("retains existing stripping of ambiguous markup in %s", (input, expected) => {
|
||||
expect(sanitizeForPlainText(input)).toBe(expected);
|
||||
});
|
||||
|
||||
// --- mixed content ------------------------------------------------------
|
||||
|
||||
it("handles mixed HTML content", () => {
|
||||
|
|
|
|||
|
|
@ -7,8 +7,16 @@ import { stripInternalRuntimeScaffolding } from "./protocol-scaffolding.js";
|
|||
// Retained for the deprecated plugin-sdk/infra-runtime compatibility barrel.
|
||||
export { stripInternalRuntimeScaffolding };
|
||||
|
||||
// A tag name ends at whitespace, `/`, or `>`; `<user@example.com>` is prose, not markup.
|
||||
// Preserve the existing tag grammar; only exclude unspaced comparison prose.
|
||||
const HTML_TAG_RE = /<\/?[a-z][a-z0-9_.:-]*(?=[\s/>])[^>]*>/gi;
|
||||
// Disjoint whitespace/prose branches avoid quadratic backtracking on malformed tags.
|
||||
const COMPARISON_PROSE_RE = /^<([a-z][a-z0-9_]*\.?)\s+[^<>=/"'\s][^<>=/"']*>$/i;
|
||||
const COMPARISON_LEFT_OPERAND_RE = /[\p{L}\p{N}_\p{S}]$/u;
|
||||
const COMPARISON_CLAUSE_RE = /\b(?:and|or)\s|[.!?;:]\s|且/iu;
|
||||
// Standard HTML element names are never comparison operands: retain main's
|
||||
// stripping even beside numeric text or prose-like bare attributes.
|
||||
const HTML_ELEMENT_NAME_RE =
|
||||
/^(?:a|abbr|address|area|article|aside|audio|b|base|bdi|bdo|blockquote|body|br|button|canvas|caption|cite|code|col|colgroup|data|datalist|dd|del|details|dfn|dialog|div|dl|dt|em|embed|fieldset|figcaption|figure|footer|form|h[1-6]|head|header|hgroup|hr|html|i|iframe|img|input|ins|kbd|label|legend|li|link|main|map|mark|menu|meta|meter|nav|noscript|object|ol|optgroup|option|output|p|picture|pre|progress|q|rp|rt|ruby|s|samp|script|search|section|select|selectedcontent|slot|small|source|span|strong|style|sub|summary|sup|table|tbody|td|template|textarea|tfoot|th|thead|time|title|tr|track|u|ul|var|video|wbr)$/i;
|
||||
const LABELED_ANGLE_LINK_RE =
|
||||
/<(?:https?:\/\/|mailto:)[^<>\s|]+\|([^<>\r\n|]*[^<>\s|][^<>\r\n|]*)>/gi;
|
||||
const MAY_CONTAIN_MARKDOWN_CODE_RE = /[`~]|\t| {4}/;
|
||||
|
|
@ -22,16 +30,42 @@ const CONVERTIBLE_HTML_OPEN_TAG_RE =
|
|||
const EMPTY_HTML_ELEMENT_RE =
|
||||
/<((?!(?:br|p|div)(?=[\s>]))[a-z][a-z0-9_.:-]*)(?=[\s>])(?:[^"'<>]|"[^"]*"|'[^']*')*>(?:[^\S\r\n\u2028\u2029]|<(?!\/?(?:br|p|div)(?=[\s/>]))\/?[a-z][a-z0-9_.:-]*(?=[\s/>])(?:[^"'<>]|"[^"]*"|'[^']*')*>)*<\/\1\s*>/gi;
|
||||
|
||||
function removeMatchesUntilStable(text: string, pattern: RegExp): string {
|
||||
function removeMatchesUntilStable(
|
||||
text: string,
|
||||
pattern: RegExp,
|
||||
replacement?: (match: string, offset: number, source: string) => string,
|
||||
): string {
|
||||
let previous: string;
|
||||
let current = text;
|
||||
do {
|
||||
previous = current;
|
||||
current = current.replace(pattern, "");
|
||||
current = replacement ? current.replace(pattern, replacement) : current.replace(pattern, "");
|
||||
} while (current !== previous);
|
||||
return current;
|
||||
}
|
||||
|
||||
function stripHtmlTagUnlessComparison(
|
||||
tag: string,
|
||||
offset: number,
|
||||
source: string,
|
||||
closingTagNames: ReadonlySet<string>,
|
||||
): string {
|
||||
const rightOperand = source.charCodeAt(offset + tag.length);
|
||||
if (
|
||||
!(rightOperand >= 48 && rightOperand <= 57) ||
|
||||
!COMPARISON_LEFT_OPERAND_RE.test(source.slice(Math.max(0, offset - 2), offset))
|
||||
) {
|
||||
return "";
|
||||
}
|
||||
const comparisonName = COMPARISON_PROSE_RE.exec(tag)?.[1];
|
||||
return comparisonName !== undefined &&
|
||||
!HTML_ELEMENT_NAME_RE.test(comparisonName) &&
|
||||
COMPARISON_CLAUSE_RE.test(tag) &&
|
||||
!closingTagNames.has(comparisonName.toLowerCase())
|
||||
? tag
|
||||
: "";
|
||||
}
|
||||
|
||||
function convertHtmlOutsideCode(text: string, options: { style?: "markdown" }): string {
|
||||
const boldMarker = options.style === "markdown" ? "**" : "*";
|
||||
const strikeMarker = options.style === "markdown" ? "~~" : "~";
|
||||
|
|
@ -56,7 +90,14 @@ function convertHtmlOutsideCode(text: string, options: { style?: "markdown" }):
|
|||
.replace(/<h[1-6]>(.*?)<\/h[1-6]>/gi, `\n${boldMarker}$1${boldMarker}\n`)
|
||||
.replace(/<li>(.*?)<\/li>/gi, "• $1\n");
|
||||
|
||||
return removeMatchesUntilStable(converted, HTML_TAG_RE).replace(/\n{3,}/g, "\n\n");
|
||||
// A matching closer is positive markup evidence, even when its content is numeric.
|
||||
const closingTagNames = new Set<string>();
|
||||
for (const tag of converted.matchAll(/<\/[a-z][a-z0-9_.:-]*\s*>/gi)) {
|
||||
closingTagNames.add(tag[0].slice(2, -1).trim().toLowerCase());
|
||||
}
|
||||
return removeMatchesUntilStable(converted, HTML_TAG_RE, (tag, offset, source) =>
|
||||
stripHtmlTagUnlessComparison(tag, offset, source, closingTagNames),
|
||||
).replace(/\n{3,}/g, "\n\n");
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue