fix(skills): keep unmatched markdown brackets out of quadratic backtracking

[Shxiao101]

Why

_extract_references() in the skill-review resource graph scanned every text
file of a reviewed package with the regex
!?\[[^\]]*]\(([^)\s]+)(?:\s+"[^"]*")?\) via finditer. Two shapes of
untrusted package content drove it quadratic:

• a long run of unmatched [ with no ]( at all — the reported [bug] skill review is quadratic in unmatched markdown brackets: a 64 KiB file of '[' takes 9.2 s #5714 case;
• content dense in ]( closers whose target run never completes, where each
  candidate opener re-scans the same suffix.

Measured on this host through analyze_skill_package():

"[" * 65_536                     ->  1.22 s   (issue reported 9.2 s)
"[a](" + "x]([" * 16_000 + "b y)" -> 11.17 s   (same shape as #5728's review)

Both are reachable from the review CLI and the review_skill_package agent
tool, so an unwieldy or malicious skill package can stall review for
seconds-to-minutes.

What changed

The link pass is now a closer-driven scan instead of a regex, so the whole scan
is linear:

• Find the next ](; the opener is the first [ after the last ] before
  it (that is where finditer reported the match — [^\]]* cannot cross a ]
  and the leftmost match wins), including a leading ! for images.
• The target is a maximal run of non-)/non-whitespace characters. A run that
  ends at ) matches; otherwise the optional "title" form is tried.
• On failure the run is skipped whole rather than retried from the next
  closer: every closer inside one failed target run ends at the same terminator
  and fails identically. This is what removes the residual quadratic — the
  ](-dense payload above now scans each character a constant number of times.

Semantics are unchanged: same match spans (so the residual blanking that the
code-span / bare-path passes depend on is identical), same fragment stripping,
same trailing-punctuation handling. Verified by differential fuzzing against
the old finditer implementation — 60,000 randomized and adversarial inputs,
zero divergence in the extracted set, including inputs that place path tokens
next to bracket constructs to catch blanking-span drift. The scanner's character
predicates were also checked against the regex classes: re.\s and
str.isspace() agree on all 1,114,112 code points, and so does [^)\s].

Two regression tests: both payload shapes must stay under a 2 s smoke bound
through analyze_skill_package(), plus a test pinning that link blanking starts
at the leftmost opener (a path token inside the construct must not reach the
bare-path pass).

Fixes #5714.

Surface area

• Frontend UI
• Backend API
• Agents / LangGraph
• Sandbox
• Skills — skill-review resource graph under backend/packages/harness/deerflow/skills/review/
• Dependencies
• Default behavior change — extracted references are unchanged; only pathological-input timing changes
• Docs / tests / CI only

Bug fix verification

• Test paths: backend/tests/test_skill_review_core.py::test_resource_graph_link_scan_stays_linear[closer-dense]
  and ::test_resource_graph_link_scan_stays_linear[unmatched-brackets],
  ::test_resource_graph_link_blanking_starts_at_the_leftmost_opener.
• Red/green: with the previous guard-only implementation the closer-dense
  case fails at 11.1 s against the 2 s bound (and unmatched-brackets
  passes, it was already covered); both pass on this branch.

Validation

Windows 11 host, repo venv, branch based on current main (fc9fb2d):

pytest tests/ -k "resource_graph or skill_review or skillscan or review_skill" -q
# -> 252 passed, 6 skipped

pytest tests/test_skill_review_core.py tests/test_skill_review_waivers.py tests/test_skill_reviewer_public_skill.py -q
# -> 72 passed

ruff check packages/harness/deerflow/skills/review/resource_graph.py tests/test_skill_review_core.py
ruff format --check packages/harness/deerflow/skills/review/resource_graph.py tests/test_skill_review_core.py
# -> clean

Scaling through analyze_skill_package() after the change (2× input ≈ 2× time):

payload	32 KB	64 KB	128 KB
unmatched brackets	0.011 s	0.015 s	0.022 s
](-dense	0.012 s	0.020 s	0.030 s

The 64 KB ](-dense payload: 11.17 s → 0.004 s in the scan itself.

AI assistance

Tool(s) used: ZCode (GLM agent)

How you used it: the agent reproduced both quadratic shapes, drafted the
closer-driven scan and the memoized run-skip, ran the differential fuzz against
the old implementation and the red/green comparison; I reviewed the diff and
verified the numbers above.

• I've read and understand every line of this change and take responsibility for it — it's not unreviewed AI output.

[JY-M666]

Thanks for the focused guard — it fixes the reported no-]( case. There is still a quadratic path whenever ]( is present: the guard enters _MARKDOWN_LINK_RE.finditer(), and a closer-dense invalid target repeatedly rescans the same suffix.

Reproduction:

payload = "[a](" + "x]([" * 16_000 + "b y)"
_extract_references(payload)

This contains ](, produces no references, and keeps the regex on the quadratic path. In the prior review of #5728, this shape measured seconds at 16k repetitions.

A complete fix can parse link targets incrementally and cache the first terminator of a failed target run, while continuing to inspect later candidate openers. I validated that approach against the existing extractor with 30,000 randomized inputs and added a bounded regression for this payload.

Would you be open to incorporating that broader guard into this PR? I can share the focused patch if helpful.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Shxiao101]

Confirmed — and thank you, the repro is exactly right: on the guard-only revision "[a](" + "x]([" * k + "b y)" measured k=2000 → 0.17 s, 4000 → 0.69 s, 8000 → 2.74 s, 16000 → 11.17 s (4× per doubling), no references extracted. The guard only covers the no-]( shape.

I've replaced the regex pass with a closer-driven scan that skips a failed target run whole instead of retrying inside it — the memoization you described, and what @willem-bd suggested in the #5728 review ("all ]( closers inside one failed run share the same terminator ... the run can be memoized/skipped past without changing the extracted set").

Details, so you can compare it with your patch:

• The opener is the first [ after the last ] before the ]( (plus a leading !), which is where finditer reported the match; the search window starts where the previous closer's window ended, so opener search + target scan + title check are all amortized O(1) per character.
• On a failed run the position jumps to that run's terminator, so the ](-dense payload above now measures 0.004 s at k=16000 and scales linearly (32 KB → 0.011-0.012 s, 64 KB → 0.020 s, 128 KB → 0.030 s through analyze_skill_package).
• Equivalence with the old finditer implementation was checked the same way you did: 60,000 randomized and adversarial inputs, zero divergence in the extracted set — including inputs that place path tokens next to bracket constructs, because the residual blanking spans have to match too, not just the extracted refs.
• The regression test now covers both payloads; [closer-dense] fails at 11.1 s on the guard-only revision and passes here.

If your validated scanner differs in any spot — especially the title form or the leftmost-opener rule — please paste it and I'll fold it in. Your reproduction is what turned this into a complete fix.

[willem-bd]

Premise of the guard is sound and I verified it directly: _MARKDOWN_LINK_RE = !?\[[^\]]*]\(([^)\s]+)(?:\s+"[^"]*")?\) has no alternation or optional branch before \( — after [^\]]* greedily consumes up to the next ], the next pattern element is a bare \(, and [^\]]* cannot itself match ], so the position of that ] is fixed for a given start. The only way any match completes is a literal ]( pair, so if "](" in content skips exactly the zero-match cases and residual is byte-identical in both versions (residual = content when no match runs). I also confirmed nothing else scans skill content for links: _MARKDOWN_LINK_RE occurs exactly once repo-wide and both entry points (the review CLI in cli.py:39 and the review_skill_package tool) go through analyze_skill_package -> build_resource_graph -> _extract_references. The other two passes are genuinely linear on analogous pathological input — 32 KiB of backticks takes 0.0005s and 64 KiB of path-like chars 0.0001s — because [^\]+fails immediately when the next char is a backtick and_PATH_TOKEN_RE` has no trailing element to backtrack into. My concerns are that the shipped regression test does not actually go red without the fix, and that no CHANGELOG entry accompanies a fix with user-visible impact.

[willem-bd]

No CHANGELOG entry ships with this one, while the neighbouring skill-review fixes did: #5841 ("skills: Strip section anchors from code-span resource references...") and the merged #5893 each added CHANGELOG.md and CHANGELOG_zh.md entries. This change is user-visible in the same way — a 19s review hang on adversarial SKILL.md content, with the review_skill_package agent tool as one of the two entry points — and the repo's documentation-update policy asks for docs to stay in sync in the same change set. A short entry under the skills/performance bullets in ## [Unreleased] (plus the zh twin and the [#5884] link reference) would match the sibling PRs.

Worth folding into the entry, since it bounds how bad the remaining hazard is: PackageLimits.max_file_bytes is 64 MiB and the resource graph scans every kind == "text" member, so a single large SKILL.md is squarely in scope for this code path.

[willem-bd]

I mutation-tested this assertion and it does not go red without the fix. With only the if "](" in content: guard reverted (everything else at head, worktree pinned to 0c22d8ff), test_resource_graph_scan_of_unmatched_brackets_stays_linear passes: pytest reports 0.91s for the whole call against the assert elapsed < 2.0 bound. That is consistent with the PR description's own numbers — the RED evidence was recorded at 256 KiB (19.20s), while the shipped test ships 64 KiB, where the unguarded cost is ~1.2s on the author's host and 0.90s on mine. A guard with ~2.2x headroom on one host is a coin-flip on another, so as written this is roughly a 50/50 regression signal rather than a pin.

Measured on an M-series Mac, unguarded vs guarded, _extract_references("[" * n):

            8 KiB    16 KiB    32 KiB   64 KiB    128 KiB
unguarded  0.0143s  0.0567s  0.2253s  0.8993s   3.7114s
guarded    0.0001s  0.0002s  0.0003s  0.0006s   0.0013s

Three suggestions, in order of value:

1. Assert the shape, not an absolute budget. The quadratic/linear distinction is ~6x per doubling unguarded vs ~2x guarded, which no CI machine can confuse. Timing 32 KiB and 64 KiB and requiring t64 / t32 < 3 (plus t32 < 1.0 to bound the absolute cost) goes red on any host that reintroduces the backtracking, and stays green even if the runner is 10x slower than the author's. That makes the assertion load-independent instead of right at the boundary.
2. Measure _extract_references, not analyze_skill_package. I confirmed the dilution is small here — 0.9102s of the 0.9257s total is inside _extract_references (~98%) — but wrapping the full analysis makes the bound host-load-dependent for no benefit, since skillscan, digest and the eval pass all add variance the regression is not about. A direct unit-level measurement is both tighter and cheaper.
3. If you prefer keeping an end-to-end number, 128 KiB unguarded is 3.7s here — comfortably above a 2.0s bound even on a fast host, so it would actually have teeth.

[willem-bd]

The head moved from 0c22d8f to 29ebc8d part-way through my review, so this supersedes part of my earlier comment on this PR: the if "](" in content guard is gone, replaced by the closer-driven scanner below, and the new parametrization adds the closer-dense payload. The performance side of that rewrite is genuinely good — I measured 0.003s for 256 KiB of [ (vs ~15s projected for the old regex) and 0.009s for the closer-dense payload at 64k repetitions (vs ~140s projected), so both #5714 shapes are now linear, and the code-span and bare-path passes are still linear on their own adversarial inputs. The problem is that the rewrite is not semantically equivalent to the finditer it replaces: differential-fuzzing the base implementation (imported verbatim from fc9fb2d) against this one, 248 of 80,000 directed random inputs disagree, and the disagreement is user-visible through analyze_skill_package. Details and a suggested direction below. Two smaller items: the new _quoted_title_end branch has no test coverage anywhere in the suite, and I still don't see a CHANGELOG.md / CHANGELOG_zh.md entry (my earlier comment on that stands, as does the point that PackageLimits.max_file_bytes is 64 MiB per file, which is what makes this path worth pinning down).

[willem-bd]

The opener search is not bounded by position: index starts at closer - 1 and walks back to the previous ] with no lower bound, so the "opener" it picks can sit inside the construct the previous iteration already consumed. finditer can never do that — its matches are non-overlapping — so this scan is not equivalent to the regex it replaces.

I differential-fuzzed the base implementation (module loaded verbatim from fc9fb2d) against this one: 248 of 80,000 directed random inputs disagree, and both directions occur.

[x]([)y](z)                       base={'['}                                      head={'[', 'z'}
a [x]([) references/notes.md b](x)  base={'[', 'references/notes.md'}               head={'[', 'references/notes.md', 'x'}

The first is user-visible. A package whose SKILL.md body is [x]([)y](z) yields resource.missing evidence ['[', 'z'] at head versus ['['] at base — an extra bogus "Referenced resource does not exist: z" warning on a deterministic review report (the [ one is pre-existing).

Suggested direction, and a caveat: bound the walk-back at position (the same bound the closer search already has) and reject an opener before position. I tried that variant — it takes the divergence count from 248/80k to 14/200k, so it fixes the case above but not everything; the last few need the opener position tracked independently of position, because finditer restarts at last_match_end or start + 1 after a failure and can still use a segment that starts before position. Driving the loop by openers (the regex engine's own start-position scan) with the failed-run skip kept is the shape that reproduces finditer exactly.

Separately, please pin this with tests rather than my fuzz: [x]([)y](z) (extra bogus ref) and a [x]([) references/notes.md b](x) (same) are both one-line packages, and [x]([)y](z) [)y](z) is the blanking case below.

[willem-bd]

Two issues with this line, both consequences of the overlap above:

1. It can blank the wrong text. content[start:end] is only still intact in residual if nothing overlapped it. Once the opener can start inside a previous construct, the span may be half blanked, so str.replace either silently no-ops or replaces an unrelated later occurrence of the same text. For a SKILL.md body of [x]([)y](z) [)y](z), link 1 blanks [x]([, so the residual is y](z) [)y](z); link 2's span [)y](z) no longer exists in it, and this call blanks the second copy instead of the one it meant to. Blanking by span — residual[:start] + " " * (end - start) + residual[end:] — is exact and immune.

2. It is O(n·m) in link count (pre-existing, not a regression — 0.748s at base for the same input): each match replaces over the whole residual, so 16,000 links in a ~600 KB SKILL.md cost 0.71s here. Collecting (start, end) spans and blanking them once after the loop fixes this too — my span-based variant of this same loop runs that payload in 0.29s.

Both fall out of the same change, so it's worth doing even if the opener question above is deferred.

[willem-bd]

No test in the suite exercises this branch. grep -F '](' backend/tests/test_skill_review_core.py finds only four link payloads — [guide](references/guide.md), [config](references/config.yaml), [FAQ](references/faq.md#pricing), [missing](references/missing.md) — all title-less, and test_review_skill_package_tool.py has no link payload at all. So the hand-rolled title parser ships with zero coverage, which is the part of this rewrite most likely to drift (it replaces a regex whose behaviour is easy to reason about).

I checked it against the base implementation myself on [a](foo/bar.md "Title"), a ) inside the title ([a](foo/bar.md "Title with ) paren")), an empty title, a tab separator, an unterminated title, and ![a](foo/bar.png "Logo") — all six agree — but that's my verification, not the suite's. A parametrized test pinning those six cases would be cheap, and the )-inside-the-title case is the one that specifically distinguishes this content.find('"', quote + 1) from the regex's [^"]*.

[willem-bd]

Superseding my earlier comment on this test (the head moved under me mid-review): with the pre-PR implementation restored, closer-dense fails at 8.60s against the 2.0s bound, so the parametrization as a whole does go red — that's the teeth I was worried about, and it's there.

What's still true is that unmatched-brackets on its own carries no signal: the same pre-PR implementation passes it at 0.91s, i.e. ~2.2x of headroom, so it only goes red on a host roughly twice as slow as the author's. Two ways to make it load-independent instead: time two sizes and assert the ratio (unguarded grows ~4x per doubling, linear ~2x, so t64 / t32 < 3 separates them on any machine), or measure _extract_references directly — I measured 0.910s of the 0.926s total inside _extract_references, so the whole-analyze_skill_package wrapper adds machine-dependent variance (skillscan, digest, eval passes) without adding signal.

Given closer-dense already covers this code path more aggressively, dropping this param is also fine.

[willem-bd]

Hi @Shxiao101 — thanks for the focused work here, and for the repro you confirmed against @JY-M666's finding on the earlier revision.

One item is blocking before this can merge, and it is new to the current head rather than something you have had a chance to see: the closer-driven scanner added at resource_graph.py:148 is not equivalent to the regex it replaces.

Minimal repro, one line:

[x]([)y](z)

_MARKDOWN_LINK_RE.finditer matches [x]([) and stops, so the only reference extracted is [. The new scan then finds ]( at index 7, walks its opener search back to the previous ] at index 2 — which lies inside the construct the previous iteration already consumed — and picks the [ at index 4 as its opener, emitting z. So a SKILL.md containing that line now produces a resource.missing finding for z that current released behaviour does not produce.

I differential-fuzzed the base implementation (module loaded verbatim from fc9fb2d) against this one across 80,000 directed inputs: 248 disagree, in both directions. The full detail, a second repro, and a suggested fix direction are in the inline comment at resource_graph.py:148. The short version: bounding the opener walk-back at position fixes the majority, but the last few need the loop driven by openers (the regex engine's own start-position scan) with the failed-run skip kept, because finditer restarts at last_match_end or start + 1 and can still use a segment starting before position.

Two things worth knowing when you pick this up:

• The head force-pushed from the "](" in content guard to this scanner while I was reviewing, so my comment at test_skill_review_core.py:310 is superseded by the one at :298. The current closer-dense parametrisation does go red (8.6 s), but the unmatched-brackets param alone still passes at 0.91 s against a 2.0 s bound.
• _quoted_title_end at :98 has no test coverage at all — the only link payloads in the suite are four title-less ones. Worth a case per title form (present, ) inside the title, empty title, tab separator, unterminated).

No rush on the rest, but the equivalence question is the one that changes behaviour for existing packages, so it is the piece I would want settled before merge.