From e8cb5e9c4f5f274401eeb555aff62e713035cbf4 Mon Sep 17 00:00:00 2001 From: Shxiao Date: Sun, 27 Sep 2026 10:10:08 +0900 Subject: [PATCH] fix(skills): keep unmatched markdown brackets out of quadratic backtracking (#5884) * fix(skills): bail out of the markdown-link scan when no link construct exists (#5714) * fix(skills): scan markdown links linearly and skip failed target runs (#5714) * fix(skills): make link scan exactly match finditer, blank spans by position Address review feedback: drive the scan by openers (the regex engine's own start-position scan) so it reproduces finditer exactly, blank matched spans by position instead of str.replace, pin the equivalence with tests, and add CHANGELOG entries. * fix: resolve skill review test lint errors --------- Co-authored-by: Willem Jiang --- CHANGELOG.md | 11 ++ CHANGELOG_zh.md | 9 ++ .../deerflow/skills/review/resource_graph.py | 97 +++++++++++++- backend/tests/test_skill_review_core.py | 118 ++++++++++++++++++ 4 files changed, 230 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dfefa9b9e5..4be7a38d87 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1266,6 +1266,16 @@ This release closes that milestone with **181 merged pull requests**. referenced. Code-span tokens now split the fragment the same way and strip trailing sentence punctuation; dotted filenames such as `references/v1.0.md#notes` keep their extension dots. ([#5841]) +- **skills:** Keep the skill review markdown-link scan linear on adversarial + input. The extractor re-scanned the same target run from every candidate + opener, so a long run of unmatched `[` or a `](`-dense invalid target drove + the review quadratic — #5714 measured 19 s on 256 KiB of `[`, and the + resource graph scans every text member with `PackageLimits.max_file_bytes` + at 64 MiB, so a single large SKILL.md is squarely in scope. The scan now + walks candidate openers once, reproducing `finditer` exactly (verified by + differential fuzzing against the base regex), and blanks matched spans by + position instead of `str.replace`, which could blank an unrelated later copy + of the same text. ([#5884]) - **config:** Sign the bytes the config cache parsed, not a second read. The app-config loader opened `config.yaml` twice — parse once, then hash a fresh read to record the `(mtime, size, sha256)` signature — so a write landing @@ -6127,6 +6137,7 @@ with **180 merged pull requests** since the first 2.0 milestone tag. [#5859]: https://github.com/bytedance/deer-flow/pull/5859 [#5879]: https://github.com/bytedance/deer-flow/pull/5879 [#5881]: https://github.com/bytedance/deer-flow/pull/5881 +[#5884]: https://github.com/bytedance/deer-flow/pull/5884 [#5893]: https://github.com/bytedance/deer-flow/pull/5893 [#5900]: https://github.com/bytedance/deer-flow/pull/5900 diff --git a/CHANGELOG_zh.md b/CHANGELOG_zh.md index 88fc98341f..90ccf51165 100644 --- a/CHANGELOG_zh.md +++ b/CHANGELOG_zh.md @@ -1202,6 +1202,14 @@ code-span token 现在以同样方式切分 fragment 并去掉尾部 句读标点；`references/v1.0.md#notes` 这类带点的文件名会 保留其扩展名中的点。([#5841]) +- **技能：** 技能评审的 Markdown 链接扫描在对抗性输入下保持线性。 + 提取器曾对每个候选 opener 重复扫描同一目标串，一长串未闭合的 `[` + 或 `](` 密集的非法目标都会让评审退化为平方复杂度——#5714 在 256 KiB + 的 `[` 下实测 19 秒，而资源图会扫描每个 text 成员， + `PackageLimits.max_file_bytes` 为 64 MiB，单个大 SKILL.md 完全在影响 + 范围内。扫描现在只遍历一次候选 opener，经与基准正则的差分 fuzz 验证， + 与 `finditer` 完全一致；已匹配区间改按位置清除，不再使用 + `str.replace`，避免误删后方相同的文本。([#5884]) - **配置：** 配置缓存签名的应是它实际解析的字节，而不是 第二次读取。app-config 加载器此前打开 `config.yaml` 两次 ——先解析一次，再对一次全新的读取做哈希来记录 @@ -5210,5 +5218,6 @@ DeerFlow 2.0 是围绕"超级智能体"框架的彻底重写，核心包含子 [#5859]: https://github.com/bytedance/deer-flow/pull/5859 [#5879]: https://github.com/bytedance/deer-flow/pull/5879 [#5881]: https://github.com/bytedance/deer-flow/pull/5881 +[#5884]: https://github.com/bytedance/deer-flow/pull/5884 [#5893]: https://github.com/bytedance/deer-flow/pull/5893 [#5900]: https://github.com/bytedance/deer-flow/pull/5900 diff --git a/backend/packages/harness/deerflow/skills/review/resource_graph.py b/backend/packages/harness/deerflow/skills/review/resource_graph.py index 13718c6307..3991de4dc5 100644 --- a/backend/packages/harness/deerflow/skills/review/resource_graph.py +++ b/backend/packages/harness/deerflow/skills/review/resource_graph.py @@ -11,7 +11,6 @@ from typing import Any from deerflow.skills.package_paths import is_eval_fixture_path from deerflow.skills.review.models import make_finding, normalize_relative_path -_MARKDOWN_LINK_RE = re.compile(r"!?\[[^\]]*]\(([^)\s]+)(?:\s+\"[^\"]*\")?\)") _CODE_SPAN_RE = re.compile(r"`([^`]+)`") _PATH_TOKEN_RE = re.compile(r"(? tuple[dict[str, Any], list _TRAILING_SENTENCE_PUNCTUATION = ".?!" +def _quoted_title_end(content: str, terminator: int) -> int: + """Return the index just past a ``](target "title")`` closing paren, or -1. + + The optional markdown title is ``\\s+"[^"]*"``: whitespace, a quoted string + (possibly empty), then the closing paren. + """ + length = len(content) + quote = terminator + while quote < length and content[quote].isspace(): + quote += 1 + if quote >= length or content[quote] != '"': + return -1 + closing = content.find('"', quote + 1) + if closing < 0 or closing + 1 >= length or content[closing + 1] != ")": + return -1 + return closing + 2 + + def _extract_references(content: str) -> set[str]: refs: set[str] = set() # Markdown link syntax fixes the fragment semantics: the text after '#' @@ -106,10 +123,80 @@ def _extract_references(content: str) -> set[str]: # bare-path passes below are literal-path contexts where '#' may be # part of a real filename, and they must never see link-internal text. # Trailing sentence punctuation is still stripped here (#5739). - residual = content - for match in _MARKDOWN_LINK_RE.finditer(content): - refs.add(match.group(1).split("#", 1)[0].rstrip(_TRAILING_SENTENCE_PUNCTUATION)) - residual = residual.replace(match.group(0), " " * len(match.group(0))) + # + # The link scan is opener-driven instead of a regex: `finditer` over + # `!?\\[[^\\]]*]\\(([^)\\s]+)(?:\\s+"[^"]*")?\\)` re-scans the same target + # run from every candidate opener, which is quadratic on content dense in + # "](" closers whose run never completes (#5714: "[a](" + "x]([" * 16_000 + # + "b y)" took 11 s). The scan below reproduces `finditer` exactly — the + # regex engine's own start-position scan — while skipping provably + # match-less regions whole, so each character is visited a constant number + # of times and the scan stays linear: + # - `[^\\]]*` cannot cross "]", so the only viable "]" for an opener is + # the first one after it; no backtracking can rescue a failed "](". + # - Every opener sharing that first "]" fails the target run identically, + # so a failed run is skipped whole instead of retried inside; only + # openers past the last "]" before the terminator may still match and + # are retried individually. + length = len(content) + spans: list[tuple[int, int]] = [] + position = 0 + while True: + # A match can only start at "[" (or at the "!" of "!["): this is the + # regex engine's start-position scan, one candidate at a time. + opener = content.find("[", position) + if opener < 0: + break + start = opener - 1 if opener > 0 and content[opener - 1] == "!" else opener + closer = content.find("]", opener + 1) + if closer < 0: + # No "]" anywhere later: no opener at/after this one can match. + break + if closer + 1 >= length or content[closer + 1] != "(": + # Every "[" in [opener, closer) shares this same first "]" and + # fails the "(" check identically: skip them all. This keeps the + # scan linear on opener-dense input ("[" * n + "]" * n). + position = closer + 1 + continue + # The target is a maximal run of non-")"/non-whitespace characters. + target = closer + 2 + terminator = target + while terminator < length and content[terminator] != ")" and not content[terminator].isspace(): + terminator += 1 + if terminator == target: + # "[]()" / "[]( x)": the target needs at least one character; + # every "[" in [opener, closer) fails the same way. + position = target + continue + if terminator < length and content[terminator] == ")": + end = terminator + 1 + else: + end = _quoted_title_end(content, terminator) + if end < 0: + # Failed target run. Openers up to the last "]" before the + # terminator share this same failure (their target run ends + # at the same terminator with the same title verdict), but an + # opener after that "]" may still match — its target run + # starts past the terminator — so only the provably failing + # prefix is skipped. + position = content.rfind("]", closer, terminator) + 1 + continue + refs.add(content[target:terminator].split("#", 1)[0].rstrip(_TRAILING_SENTENCE_PUNCTUATION)) + spans.append((start, end)) + position = end + # Blank matched spans by position, not by text: `str.replace` blanks an + # unrelated later occurrence of the same text once spans can overlap + # consumed text, and it re-scans the whole residual per match (O(n*m) in + # link count). Spans are non-overlapping and ordered, so a single pass + # over them blanks each exactly once. + parts: list[str] = [] + cursor = 0 + for span_start, span_end in spans: + parts.append(content[cursor:span_start]) + parts.append(" " * (span_end - span_start)) + cursor = span_end + parts.append(content[cursor:]) + residual = "".join(parts) for match in _CODE_SPAN_RE.finditer(residual): token = match.group(1).strip() if "/" in token: diff --git a/backend/tests/test_skill_review_core.py b/backend/tests/test_skill_review_core.py index 3f02a5cb0e..836c0bfa26 100644 --- a/backend/tests/test_skill_review_core.py +++ b/backend/tests/test_skill_review_core.py @@ -2,6 +2,7 @@ import io import json import stat import tempfile +import time import zipfile from pathlib import Path @@ -13,6 +14,7 @@ from deerflow.skills.review.cli import main as review_cli_main from deerflow.skills.review.models import PackageLimits, normalize_relative_path from deerflow.skills.review.readers import ArchivePackageReader, parse_skill_uri from deerflow.skills.review.renderer import build_static_report, render_report_markdown +from deerflow.skills.review.resource_graph import _extract_references CONTRACTS_DIR = Path(__file__).resolve().parents[2] / "contracts" / "skill_review" @@ -291,6 +293,122 @@ def test_resource_graph_ignores_eval_fixture_references(tmp_path): assert not any(f["rule_id"] == "resource.missing" and f["path"].startswith("evals/fixtures/") for f in facts["findings"]) +@pytest.mark.parametrize( + "make_payload", + [ + pytest.param(lambda n: "[" * n, id="unmatched-brackets"), + pytest.param(lambda n: "[a](" + "x]([" * n + "b y)", id="closer-dense"), + ], +) +def test_resource_graph_link_scan_stays_linear(make_payload): + # #5714: both shapes drove the markdown-link scan quadratic. A long run of + # unmatched "[" has no "](" at all, and the "]("-dense run below never + # completes a target: each of its candidates re-scanned the whole suffix + # (11 s at 16k repetitions measured before the fix). + # + # Assert the shape, not an absolute budget: the quadratic/linear + # distinction is ~4x vs ~2x per doubling, which no CI machine can + # confuse, while an absolute bound is a coin-flip on a slower host. + # _extract_references is measured directly so the skillscan/digest/eval + # passes add no host-dependent variance. min() over repeats keeps the + # tiny-input ratios stable. + small = make_payload(32768) + large = make_payload(65536) + + def timed(payload): + return min(_elapsed(_extract_references, payload) for _ in range(5)) + + small_elapsed = timed(small) + large_elapsed = timed(large) + + assert small_elapsed < 1.0, f"link scan took {small_elapsed:.2f}s" + assert large_elapsed / small_elapsed < 3, f"link scan looks superlinear: 32K took {small_elapsed:.4f}s, 64K took {large_elapsed:.4f}s" + + +def _elapsed(fn, payload): + started = time.monotonic() + fn(payload) + return time.monotonic() - started + + +@pytest.mark.parametrize( + "payload, expected", + [ + pytest.param("[x]([)y](z)", {"["}, id="opener-inside-consumed-construct"), + pytest.param( + "a [x]([) references/notes.md b](x)", + {"[", "references/notes.md"}, + id="no-bogus-ref-from-overlap", + ), + pytest.param("[x]([)y](z) [)y](z)", {"[", "z"}, id="blanking-keeps-later-match"), + ], +) +def test_resource_graph_link_scan_matches_finditer(payload, expected): + # The scan must agree with the regex it replaces exactly: `finditer` + # matches are non-overlapping and resume at the end of the previous + # match, so an opener inside an already-consumed construct can never + # start a new match. `[x]([)y](z)` matches `[x]([)` and stops — the `z` + # link is not real — while `[x]([)y](z) [)y](z)` has two genuine, + # non-overlapping matches that must both be found and blanked. + assert _extract_references(payload) == expected + + +@pytest.mark.parametrize( + "payload, expected", + [ + pytest.param('[a](foo/bar.md "Title")', {"foo/bar.md"}, id="titled-link"), + pytest.param( + '[a](foo/bar.md "Title with ) paren")', + {"foo/bar.md"}, + id="paren-inside-title", + ), + pytest.param('[a](foo/bar.md "")', {"foo/bar.md"}, id="empty-title"), + pytest.param('[a](foo/bar.md\t"Title")', {"foo/bar.md"}, id="tab-separator"), + pytest.param('![a](foo/bar.png "Logo")', {"foo/bar.png"}, id="image-with-title"), + pytest.param('[a](foo/bar.md "unterminated', set(), id="unterminated-title"), + ], +) +def test_resource_graph_quoted_title_targets(payload, expected): + # The hand-rolled title parser replaces `(?:\s+"[^"]*")?`: pin its + # behavior on the shapes that distinguish it from the regex — the + # `)`-inside-title case in particular exercises `content.find('"')` + # against the regex's `[^"]*`. + assert _extract_references(payload) == expected + + +def test_resource_graph_link_blanking_starts_at_the_leftmost_opener(tmp_path): + # A link construct starts at the first "[" after the previous "]" (the + # leftmost match wins), so the whole construct is blanked out of the + # residual text. A path token inside it must not reach the bare-path pass + # and resurrect a reference: only the real link target is extracted. + _write( + tmp_path / "SKILL.md", + _valid_skill() + "\n[references/hidden.md[a](references/kept.md)\n", + ) + _write(tmp_path / "references" / "kept.md", "# Kept\n") + + facts = analyze_skill_package(LocalDirectoryReader(tmp_path).read()) + + assert {"source": "SKILL.md", "target": "references/kept.md"} in facts["resources"]["edges"] + assert not any(f["rule_id"] == "resource.missing" and "hidden" in f["message"] for f in facts["findings"]) + + +def test_resource_graph_non_link_reference_passes_survive_link_guard(tmp_path): + # The link scan must only skip the markdown-link pass: code-span and + # bare-path references carry no "](" construct and must still be extracted. + _write( + tmp_path / "SKILL.md", + _valid_skill() + "\nSee `references/from-code-span.md` and references/from-bare-path.md.\n", + ) + _write(tmp_path / "references" / "from-code-span.md", "# A\n") + _write(tmp_path / "references" / "from-bare-path.md", "# B\n") + + facts = analyze_skill_package(LocalDirectoryReader(tmp_path).read()) + + for target in ("references/from-code-span.md", "references/from-bare-path.md"): + assert {"source": "SKILL.md", "target": target} in facts["resources"]["edges"] + + def test_package_digest_is_path_independent(tmp_path): one = tmp_path / "one" two = tmp_path / "two"