mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-10-26 13:16:24 +00:00
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 <willem.jiang@gmail.com>
This commit is contained in:
parent
4028b63647
commit
e8cb5e9c4f
11
CHANGELOG.md
11
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
|
||||
|
||||
|
||||
@ -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
|
||||
|
||||
@ -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"(?<![\w./-])(?:references|scripts|templates|assets|evals)/[A-Za-z0-9._~/%+#-]+")
|
||||
_RESOURCE_DIRS = {"references", "scripts", "templates", "assets", "evals"}
|
||||
@ -96,6 +95,24 @@ def build_resource_graph(snapshot: dict[str, Any]) -> 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:
|
||||
|
||||
@ -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('', {"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"
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user