Uh oh!
There was an error while loading. Please reload this page.
fix(executor): make markdown linkification linear - #407
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes worst-case quadratic behavior in executor.linkify_markdown, preventing pathological markdown (e.g., long runs of backticks/brackets/punctuation) from stalling the async event loop that drives the dashboard, concurrency, and timeout enforcement.
Changes:
- Makes fenced-code detection avoid pathological backtracking by tightening the opener match (possessive quantifier + newline requirement).
- Replaces the existing-link regex with a forward-only scanner to identify
[text](url)and[text][ref]spans in linear time. - Replaces per-character path punctuation trimming with
str.lstrip()/str.rstrip(), and adds regression/performance coverage (including reference links and near-linear scaling checks).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/conductor/executor/linkify.py | Removes the regex-based existing-link scan, hardens fenced-code detection against backtracking, and makes path trimming linear-time. |
tests/test_executor/test_linkify.py | Adds reference-link preservation coverage and performance-regression tests for pathological inputs. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Jason Robert (jrob5756)
left a comment
There was a problem hiding this comment.
The performance work here is solid, and two of the three rewrites are provably behaviour-preserving: I fuzzed _existing_link_spans against the regex it replaces over 200k inputs, and the lstrip/rstrip trimming over another 200k, with zero divergences in both. The monotonic search_start argument behind the memo holds up. Measured win on 20k backticks: 1400ms down to 0.74ms.
Three things I would want resolved before merge.
The possessive quantifier on the fence regex changes behaviour as well as complexity, and it silently stops protecting a code block whose opening fence is longer than its closing one. That reaches real agent output through the gate and dialog paths. Details and a verified drop-in are inline.
The regression test is marked performance, which CI deselects, so nothing guards the fix. And the punctuation parameter passes against origin/main as written, which leaves _try_linkify_path with no working coverage at all.
Two things sit outside the diff, so I could not anchor them:
_wrap_url (linkify.py:194-204) still has the exact character-at-a-time pattern this PR removes from _try_linkify_path. _URL_RE admits ., so "https://a.example/x" + "." * n drives the loop: I measured 16.5ms, 50.4ms, 189ms and 946ms at 25k/50k/100k/200k, roughly quadrupling per doubling. Applying this PR's own assertion to that input passes, so the new test does not see it. The ) branch is unreachable, since _URL_RE's character class excludes ) and a Wikipedia URL arrives already truncated at Foo_(bar, so the whole loop collapses to url = matched.rstrip(".,;:!?"). Worth doing here, given the title claims linearity and closes #395.
There is also no CHANGELOG.md entry. This changes gate and dialog rendering, and recent user-facing fixes such as #393 and #387 added one under Unreleased.
| # Fenced code block (``` or ~~~, with optional language tag) | ||
| _FENCED_CODE_RE = re.compile(r"^(`{3,}|~{3,}).*?^\1", re.MULTILINE | re.DOTALL) | ||
| _FENCED_CODE_RE = re.compile(r"^(`{3,}+|~{3,}+)[^\n]*\n.*?^\1", re.MULTILINE | re.DOTALL) |
There was a problem hiding this comment.
Making the opener possessive removes a behaviour along with the backtracking: a 4-backtick opener can no longer close on a 3-backtick fence, so that block stops being protected and its contents get rewritten.
input: "````\nSee src/x.py\n```"
main: "````\nSee src/x.py\n```"
this: "````\nSee [src/x.py](src/x.py)\n```"
Across 60k fence-shaped inputs, 87,237 characters that main protected are now unprotected. CommonMark says a short closer does not close a longer fence, so the renderer still shows the region as code and the injected link syntax appears literally inside it. Models wrap nested code blocks in longer fences routinely, and that text reaches gates/dialog.py:623 and gates/human.py:299.
Adding CommonMark's unterminated-fence fallback keeps the possessive quantifier and restores the protection. I measured zero characters under-protected against main on the same corpus, still linear at 0.10/0.18/0.37ms for 20k/40k/80k backticks, ruff-clean at 96 characters, and tests/test_executor plus tests/test_gates pass (91 tests).
| _FENCED_CODE_RE=re.compile(r"^(`{3,}+|~{3,}+)[^\n]*\n.*?^\1", re.MULTILINE|re.DOTALL) | |
| _FENCED_CODE_RE=re.compile(r"^(`{3,}+|~{3,}+)[^\n]*\n(?:.*?^\1|.*)", re.MULTILINE|re.DOTALL) |
The comment above this line is worth updating too. The + in {3,}+ is the entire fix (161ms to 0.03ms on 8k backticks, while [^\n]*\n alone stays quadratic), and as written it reads like a typo someone will delete.
| """Find existing ``[text](url)`` and ``[text][ref]`` links.""" | ||
| spans: list[tuple[int, int]] = [] | ||
| search_start = 0 | ||
| unavailable_closers: set[str] = set() |
There was a problem hiding this comment.
This memo is what keeps "[a](" * n linear (6.7ms versus 16.8ms at 80k without it), and its correctness depends on search_start never moving backwards. That invariant is not written down anywhere, so a later change that backtracks would quietly stop protecting existing links, with no error and no test to catch it.
The "]" half is also never read back: once it is recorded, the next text.find("]", start + 1) returns -1 and the loop breaks before reaching the check. Confirmed over an exhaustive enumeration of []()x up to length 10.
| unavailable_closers: set[str] =set() | |
| # Closers proven absent from the rest of the text. Sound only because | |
| # search_start never moves backwards: once find(closer, i) misses, every | |
| # later start is > i and misses too. Without it, text full of unclosed | |
| # "[a](" candidates rescans the tail per candidate, which is quadratic. | |
| # Only ")" is ever read back; with no "]" left, the label scan breaks first. | |
| unavailable_closers: set[str] =set() |
| stripped = token.lstrip("([\"'") | ||
| prefix = token[: len(token) - len(stripped)] | ||
| path = stripped.rstrip(")]\"'.,;:!?") | ||
| suffix = stripped[len(path) :] | ||
| stripped = path |
There was a problem hiding this comment.
stripped holds two different values four lines apart, and path is dead after the last line. Dropping line 238 during a later cleanup leaves stripped un-rstripped and everything still runs, so this is easy to break silently.
The comment can carry the part that is not obvious instead: that the trimmed characters are reattached rather than discarded.
| stripped=token.lstrip("([\"'") | |
| prefix=token[: len(token) -len(stripped)] | |
| path=stripped.rstrip(")]\"'.,;:!?") | |
| suffix=stripped[len(path) :] | |
| stripped=path | |
| # Split off surrounding punctuation; prefix and suffix are reattached below | |
| # so "(see foo/bar.md)." round-trips. The trailing set is wider because | |
| # sentence punctuation can only follow a path, never precede it. | |
| unprefixed=token.lstrip("([\"'") | |
| prefix=token[: len(token) -len(unprefixed)] | |
| stripped=unprefixed.rstrip(")]\"'.,;:!?") | |
| suffix=unprefixed[len(stripped) :] |
| assert "[../../../etc/passwd.txt]" not in result | ||
| @pytest.mark.performance |
There was a problem hiding this comment.
CI deselects this marker: both .github/workflows/ci.yml:120 and release.yml:108 run -m "not real_api and not performance", and I confirmed 3 tests deselected locally. So the only regression guard for this fix never executes on a pull request.
Excluding the marker makes sense for ratio-based microbenchmarks, so the test's shape is the thing to change rather than the marker. The separation is wide enough that an absolute ceiling will not flake: 40k backticks is 1.5ms on this branch against 4.3s on main, and even the cheapest case at 320k is around 10ms against 1s.
| @pytest.mark.performance | |
| # Deliberately unmarked: CI runs `-m "not performance"`, so a marked test cannot | |
| # guard this fix. The ceiling below is absolute and sits far above the fixed-code | |
| # cost, so it can only fire on a return to quadratic behaviour (#395). |
| [ | ||
| pytest.param(lambda size: "`" * size, id="backticks"), | ||
| pytest.param(lambda size: "[" * (size // 2) + "]" * (size // 2), id="brackets"), | ||
| pytest.param(lambda size: '"' * size, id="punctuation"), |
There was a problem hiding this comment.
This parameter passes against origin/main, so it cannot catch the regression it exists for. Measured on the unfixed code: 20k takes about 5ms and 40k about 17ms, and 5 * 3 + 10ms is 25ms, so it clears comfortably. The + 0.01 epsilon is larger than this case's entire pre-fix cost.
That leaves the lstrip/rstrip change with no coverage at all: reverting _try_linkify_path to the character loops keeps the suite green. Raising this case to around 320k separates the two cleanly (about 10ms fixed against about 1s broken), which needs size parametrized alongside make_text rather than fixed in the loop body.
A fourth case for _wrap_url would be worth adding at the same time, once that loop is fixed:
pytest.param(lambdasize: "https://example.com/x"+"."*size, id="url-punctuation"),| assert linkify_markdown(text) == text | ||
| elapsed.append(time.perf_counter() - start) | ||
| assert elapsed[1] < elapsed[0] * 3 + 0.01 |
There was a problem hiding this comment.
The 10ms epsilon is doing most of the work in both directions. On the fixed code I saw ratios up to 3.19, above the threshold of 3, so the epsilon is what keeps it green; on the unfixed code it is what lets punctuation through. The effective assertion is already an accidental 12ms ceiling, just not a chosen one.
An explicit per-case ceiling would say what it means and match tests/test_performance.py, which uses absolute bounds throughout. Either way the failure message should carry the timings, since a bare ratio prints nothing to diagnose from.
| assertelapsed[1] <elapsed[0] *3+0.01 | |
| assertelapsed[1] <elapsed[0] *3+0.01, ( | |
| f"doubling input grew runtime from {elapsed[0] *1e3:.1f}ms to {elapsed[1] *1e3:.1f}ms" | |
| ) |
The docstring above says "must not quadruple" while the assertion allows 3x; worth aligning whichever number survives.
Closes#395.
What
linkify_markdowncould take seconds to process gate and dialog text containing long runs of backticks, brackets, or path punctuation.Because linkification runs synchronously, a pathological prompt blocked the event loop along with dashboard updates, concurrent agents, and timeout enforcement.
Why
Three paths repeatedly processed the remaining input:
[characters from every position.On the original implementation, doubling a backtick run from 10,000 to 20,000 characters increased local runtime from 0.49s to 1.97s.
How
[text](url)and[text][ref].str.lstrip()andstr.rstrip().The change stays inside the shared linkification helper, so all gate and dialog callers use the same fix.
Validation
pytest tests/test_executor/test_linkify.py— 33 passedruff check src tests— passedruff format --check src tests— passedty check src/conductor/executor/linkify.py— passedThe full local Windows suite is not reported as passing because platform-specific tests outside this module failed. The focused linkification tests and all checks listed above pass.