fix(logs): match copilot log-grep patterns with RE2 (linear time) - #5987
Conversation
PR SummaryHigh Risk Overview In-process matching now goes through PII custom patterns (editor + API boundary) only validate syntax—backtracking screening is removed because those regexes run in Presidio, not Node. Guardrail Differential and timing tests cover parity and catastrophic patterns across logs, VFS, chunking, and guardrails. Reviewed by Cursor Bugbot for commit 9ba9e65. Configure here. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Greptile SummaryReplaces caller-controlled backtracking regex execution with RE2JS-backed linear matching.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/lib/core/security/linear-regex.ts | Introduces the shared RE2JS-backed matching interface, syntax translation, literal matching, and lookaround-aware splitting. |
| apps/sim/lib/logs/log-views.ts | Moves execution-trace grep to linear matching and returns a notice when unsupported syntax is treated literally. |
| apps/sim/lib/chunkers/regex-chunker.ts | Replaces native regex splitting and runtime safety probes with linear compilation and explicit unsupported-pattern errors. |
| apps/sim/lib/copilot/vfs/operations.ts | Moves VFS grep to reusable linear matchers while preserving plain-text and case-insensitive searches. |
| apps/sim/lib/guardrails/validate_regex.ts | Uses linear matching for in-process guardrails while limiting persisted Presidio custom-pattern validation to syntax checks. |
| apps/sim/lib/copilot/tools/server/workflow/query-logs.ts | Propagates log-grep pattern notices through the Copilot query response. |
| apps/sim/package.json | Adds the pure-JavaScript re2js runtime dependency. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Input[Caller-supplied pattern and text] --> Plain{Plain text?}
Plain -->|Yes| Literal[Native escaped literal matcher]
Plain -->|No| RE2[Compile with RE2JS]
RE2 -->|Supported| Linear[LinearRegex test/find/split]
RE2 -->|Unsupported| Policy{Caller policy}
Policy --> Logs[Log grep: literal fallback plus notice]
Policy --> VFS[VFS grep: literal fallback plus warning]
Policy --> Guardrail[Guardrail: fail closed with error]
Policy --> Chunker[Regex chunker: positive-lookaround decomposition or reject]
Reviews (9): Last reviewed commit: "test(security): pin engine parity with a..." | Re-trigger Greptile
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 2a721c8. Configure here.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 92084df. Configure here.
cba72da to
462aac8
Compare
|
@cursor review |
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 6a579ef. Configure here.
ffe77fd to
0380a0d
Compare
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 0380a0d. Configure here.
0380a0d to
6651434
Compare
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 6651434. Configure here.
`grepSpans` compiled a caller-supplied pattern with a bare `new RegExp`, whose only guard was a `catch` for syntax errors. Trace text is attacker-influenced — a workflow can emit arbitrarily long uniform runs into its own block outputs — and matching runs synchronously on the shared event loop, so an authenticated caller could choose both the pattern and the input and stall every request on the instance. Patterns are now matched as an escaped, case-insensitive literal. Screening was implemented first and abandoned, because each rule only excludes the shapes someone thought to enumerate: - `safe-regex2` (already used by the guardrails validator) documents itself as having false negatives. It screens star height only, so it passes `(a|a)*b`, measured >61s on V8 at 30 characters. - Rejecting quantified groups on top of that catches `(a|a)*b`, and every attack `safe-regex2` catches, but still passes `a*a*b` — adjacent quantifiers over overlapping character sets — measured 213s on JSC and 132s on V8 over a 10k-character run, well inside what a block output can hold. An escaped literal is linear on every engine, needs no dependency, and does not depend on which runtime serves the request. `safe-regex2` stays a dependency for its existing guardrails/PII callers; it is simply not relied on here. `query_logs` loses regex matching. Its catalog entry describes `pattern` as "greps" rather than promising regex, but it is generated from a contract in another repository and cannot be updated here, so a `patternNotice` is returned whenever a pattern contains regex syntax — the caller is told the pattern was taken literally instead of reading zero matches as "not in the trace". Two bounds are kept as backstops: a cumulative match-time budget charging only time inside `test`/`exec` (never the blob-store reads the scan awaits between matches, which would truncate slow-but-legitimate greps under load), and a total scanned-character cap.
…ated invariant Review follow-ups on the literal log-grep. The `patternNotice` fired on any regex metacharacter, which includes `.` — so ordinary literal searches (`example.com`, `file.pdf`, `v1.2.3`, `block_1.output`) were told their pattern "was not interpreted as a regex", inviting the agent to retry a search that had in fact worked. Measured 10 false notices across 19 realistic literal searches. `REGEX_INTENT` now keys on actual regex intent — escape classes, character class, group, alternation, a leading/trailing anchor, or a repetition quantifier — which drops that to 0/19 while still flagging all 10 regex attempts tested. `truncated` gains a TSDoc block stating its invariant: it reports that trace was left unread, not that a budget was reached. Every point that skips work goes through `done()`, which sets it, so a budget exhausted by the final match correctly reports `false`. Raised by Cursor Bugbot; behavior is right, the contract was just implicit. Also drops `snippetAround`'s `maxChars` parameter, which every caller passed from the state object it already receives.
… support Restores full regex support on copilot log-grep, which the previous commits in this PR had removed to close the ReDoS. Deleting the capability was the wrong trade: RE2JS is a port of RE2 that matches in time linear in the input, so no pattern can blow up regardless of the text, and callers keep their regexes. Measured through the real compile path, against a 100k-character adversarial input — 10x the run that took 213s on JSC / 132s on V8: (a+)+$ 13.0ms (\w+\s?)*$ 10.7ms (a|a)*b 6.7ms (x+x+)+y 4.0ms a*a*b 8.1ms ^(\d+)*$ 0.0ms Two escape hatches keep this honest: - A pattern with no regex metacharacter takes the built-in engine. Semantics are identical when there is nothing to interpret, and RE2JS costs ~100x more per byte (~25ms/MB), so the common plain-text search stays native. - Lookaround and backreferences are not in RE2. RE2JS rejects them at compile time, so those fall back to a literal and return a `patternNotice` naming the constructs — a narrow, reported gap rather than a silent behavior change. The match-time and scanned-character budgets stop being formalities: RE2JS's throughput is what they now bound, not backtracking. Also collapses the old test-then-exec double scan — `compilePattern` returns a single `find` returning a match index, which `recordIfMatch` passes straight to `snippetAround`, so each field is scanned once instead of twice. re2js@2.8.6 is MIT, pure JavaScript (no native addon, so nothing changes for the oven/bun image), server-only, and published 2026-07-05 — clearing the 7-day bunfig minimumReleaseAge gate without an exclusion.
…ime engine Extends the RE2 fix to the three other places that compiled a caller-supplied pattern and ran it on the shared event loop, behind one shared helper — `lib/core/security/linear-regex.ts` — rather than four local copies. - `lib/copilot/vfs/operations.ts` — copilot VFS grep took a caller pattern over caller-supplied file content with no screen at all. - `lib/guardrails/validate_regex.ts` (`validateRegex`) — a guardrail rule's pattern matched against LLM output, screened only by `safe-regex2`. - `lib/chunkers/regex-chunker.ts` — the KB chunker's split pattern. This one screened for catastrophic backtracking by running the pattern against six probes including `'a'.repeat(10000)` and rejecting anything over 50ms — but it measured elapsed time *after* the match returned, so the screen was the denial of service it existed to prevent (`a*a*b` on that probe: 213s). The probe is deleted rather than repaired. Deliberately unchanged: `validateRegexPattern`. Those patterns are persisted for Presidio, a separate service where a slow pattern times out instead of stalling this loop, and Presidio's Python engine accepts constructs RE2 does not — so narrowing it to the RE2 subset would reject patterns that work. Its TSDoc now says plainly that its `safe-regex2` screen is a courtesy, not a ReDoS defense, and points in-process callers at `compileLinearRegex`. `safe-regex2` therefore stays a dependency, used only there. Lookaround is the standard way to split while keeping the delimiter (`(?=#\s)`) and RE2 cannot represent it, so the chunker keeps the built-in engine for those patterns — the only remaining path that can backtrack, bounded by the existing 500-character cap. RE2JS `split` was verified identical to `String.split` across six representative chunking patterns. Adds 27 tests for the shared helper (every catastrophic pattern asserted linear over a 50k-character input) plus regression tests at each site, plus the first tests for `validateRegex`. Also removes the now-duplicated `escapeRegExp` and literal-finder locals from `log-views.ts`.
… engine Audit follow-ups, plus the Cursor round-4 finding. Removes `safe-regex2` entirely. Its last caller was `validateRegexPattern`, the PII custom-pattern boundary, where it was pure cost: it screens star height alone and passes `(a|a)*b` and `a*a*b`, while rejecting patterns that work — lookbehind and optional groups could not be saved as custom PII rules at all. Nor could any screen be sound there: those patterns execute in Presidio's Python engine, which backtracks on shapes RE2 accepts, so RE2-representability says nothing about their runtime. Validation is now syntax-only and the dependency is gone. Cursor flagged the chunker's lookaround fallback as reintroducing backtracking, which was correct. The fallback is removed. Verified first that no bounded probe can replace it: at a probe size small enough to survive an exponential pattern (24 chars), `(?=a*a*b)` completes in 0.0ms and passes, because polynomial blowup only shows on long input — small probes miss it and large probes hang, the same trap as the original guard. Instead `compileLookaroundSplit` puts the two idioms that actually need lookaround onto RE2: splitting on `(?=X)` is slicing at every match start of X, and on `(?<=X)` at every match end, neither of which needs the assertion. Both `(?=#\s)` and the pre-existing `(?<=</section>)` case keep working, now in linear time; anything else RE2 cannot represent is rejected with an actionable message rather than run on the backtracking engine. Also from the audit: - `literalRegex` recompiled its `RegExp` on every call to dodge `lastIndex` state from the `g` flag. VFS grep calls it per line, so that was a compile per line. It now keeps one non-global instance for `test`/`find` and a global one for `split`. - `validateRegex` and VFS grep log a warning when a pattern degrades. A guardrail rule that used lookaround now fails closed, which reads as the guardrail tripping on every input, and VFS grep's return shape has nowhere to report that a pattern was taken literally — both need to be findable in logs. Measured: RE2 costs 6-13x native on per-line matching (20k-line, 1.66MB file greps in 7ms), against ~100x on single multi-megabyte strings.
…tion CI caught this; my local runs only covered `lib/**` and missed the `.tsx` counterpart of removing `safe-regex2`. The editor asserted that `(a+)+$` renders "potentially unsafe". That screen is gone, so the assertion is inverted: the test now pins that `(a+)+$`, lookbehind and optional groups all render cleanly, which is the point of the removal — they are valid Presidio patterns that could not be saved before. Syntactically invalid patterns still surface "Invalid regex".
…ce semantics
Three independent reviewers with no shared context audited the RE2 migration.
Two found the same high-severity regression, and one found the divergence that
silently rewrites stored data. Both are fixed here.
**Lookaround with an affix threw.** `compileLookaroundSplit` only accepted a
pattern that was *entirely* one assertion, so `(?<=\.)\s+` — split after the
period — and `(?<=[.!?])\s+(?=[A-Z])` — the textbook sentence splitter — fell
through to the chunker's `throw`. Those are persisted KB configs, and the
compile runs in the constructor, so an existing knowledge base would stop
re-ingesting with a hard error. The TSDoc claiming the bare assertions "cover
the reason a split pattern reaches for lookaround" was simply wrong.
Fixed generally rather than by adding another special case: splitting never
needs the assertion, only the span the delimiter consumes, so `(?<=X)Y(?=Z)`
compiles to `(?:X)(Y)(?:Z)` and group 1's span is exactly what `String.split`
removes. One rule now covers every combination, and the four shapes above match
`String.prototype.split` output exactly.
**`\s` silently lost 14 codepoints.** RE2's `\s` is ASCII-only; ECMAScript's
also covers `\v`, NBSP, U+2028/9, U+202F, U+3000, U+FEFF and the rest of
`\p{Zs}`. A reviewer measured 9 of 35 realistic pattern/document pairs chunking
differently through the real `RegexChunker` — NBSP from PDF/DOCX/HTML
extraction, U+3000 in CJK, U+202F in French text. That rewrites stored chunk
boundaries and embeddings for a document that never changed, with nothing
thrown and nothing logged. `translateToRe2` now rewrites `\s`/`\S` to the
verified-equivalent RE2 class, and `\uXXXX` to `\x{XXXX}` (RE2 rejects the
former outright, turning a non-ASCII delimiter into a hard failure).
Also from the audit:
- `truncated`'s new TSDoc asserted an invariant the code violates — reaching
`maxMatches` sets it even when nothing was left unread. Reworded to what it
actually means.
- The chunker's rejection message blamed "backreferences and lookaround" for
repeat-count and `\uXXXX` rejections too.
- `escapeRegExp` was a byte-identical copy of `executor/constants.ts:472` and
exported for nobody; now module-private.
- The guardrails wand prompt told the LLM to emit "valid JavaScript regex",
which reliably produces `(?=.*\d)`-style patterns that now fail every check.
- The PII editor's TSDoc still described the removed backtracking screen.
- VFS grep's TSDoc omitted that invalid patterns now match literally.
Test gaps the audit exposed: the match-time budget's only mechanism was
unguarded (deleting the accumulation left every test green) — now covered by a
mutation-verified test; the escape test survived dropping `.` from the class;
the split-parity test dodged the one real divergence; and two test names
described the built-in engine that no longer runs.
…ition A fourth reviewer, given no context beyond "these scanners are new and unreviewed", differential-tested them against the built-in engine over ~62k comparisons and found three real defects in code added an hour earlier. All were reachable through RegexChunker with plausible patterns, and all silently produced different chunks rather than failing. 1. **Top-level alternation was reshaped into a different pattern.** `(?<=\.)\s+|\n\n` means `((?<=\.)\s+)|(\n\n)`, but decomposing it produced `(?:\.)(\s+|\n\n)` — demanding the period before *both* branches. It could lose boundaries (`"Heading\n\nAlpha beta.\nGamma delta."` split in 2, not 3) and invent them. No grouping recovers the original meaning, so patterns whose middle alternates at the top level are now declined outright, which also removes an asymmetry: `\n\n|(?<=\.)\s` already threw. 2. **A capturing group inside the lookbehind stole group 1.** `(?<=(a))b` cut at the assertion instead of the delimiter, and where the group did not participate `start(1)` was -1, so `find` and `test` contradicted each other. The middle is now captured by name, immune to numbering. 3. **Consuming the assertion text swallowed every other boundary.** The compiled form matched `behind + middle + ahead`, so any boundary whose lookahead doubles as the next one's lookbehind was skipped: `(?<=\w)\s+(?=[A-Z])` over `"A B C D"` split 2 of 3 gaps. Each iteration now resumes at the end of the delimiter rather than the end of the match. This was documented as affecting only self-overlapping delimiters; that was wrong and far too narrow. The reviewer confirmed `translateToRe2` and `closingParen` clean across exhaustive escape-state, character-class, and paren-matching probes, and that nothing added is superlinear. One divergence remains and is now documented rather than overstated: when delimiters themselves overlap (a single-character middle whose matches abut, as in `(?<=\w).(?=\w)`), a match starting behind the cursor is dropped instead of emitting the empty segment the built-in engine produces. Splitters that consume whitespace or punctuation between tokens do not overlap and match exactly.
6b13124 to
76d896d
Compare
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 76d896d. Configure here.
Every defect in this module has been a silent semantic divergence rather than a
crash, and each was found by comparing engines over a corpus in a throwaway
script. That comparison now lives in the suite: ~700 pattern/document pairs plus
match/index/ignore-case parity, checked against the built-in engine on every
run, with the accepted divergences enumerated and individually pinned.
It earned its place immediately by falsifying my own documentation. The
"self-overlapping delimiter" caveat was stale — fixing the boundary-advance in
the previous commit also made `(?=#{1,6}\s)`, `(?=aa)` and `(?=##)` exact, so
the entry asserting they still diverge failed. One real divergence remains and
is now stated precisely: a lookbehind whose body self-overlaps (`(?<=aa)` over
`aaaaa`). Making that exact means restarting the scan one character past each
match start, which is quadratic on a multi-megabyte document and forfeits the
linear guarantee — so it is a deliberate trade, not an oversight.
Mutation-verified against the three bugs that actually escaped review:
disabling the `\s` translation fails 12 tests, changing the boundary-advance
fails 2, and removing the top-level-alternation guard fails 3. That last one
initially passed, because the corpus had no alternation pattern in it — the gap
is closed and the mutation now fails as it should.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 9ba9e65. Configure here.
Summary
grepSpanscompiled a caller-suppliedpatternwith a barenew RegExp, guarded only by acatchfor syntax errors, then ran it synchronously against execution-trace text on the shared event loop. Trace text is attacker-influenced — a workflow can emit long uniform runs into its own block outputs — so one authenticated request could pick both the pattern and the input and stall every request on the instance.(a+)+$13.0ms ·(a|a)*b6.7ms ·a*a*b8.1ms ·(x+x+)+y4.0ms ·(\w+\s?)*$10.7ms.safe-regex2(used by the guardrails validator) documents itself as having false negatives and passes(a|a)*b; rejecting quantified groups on top of it still passesa*a*b. Every syntactic rule only excludes the shapes someone thought to enumerate — so the engine changed rather than the filter.Behavior
patternNoticenaming the constructs — a narrow, reported gap rather than a silent change. Everything else (\d+,^anchor,(a|b),\b,{n,m},.*) behaves exactly as before.Dependency
re2js@2.8.6— MIT, pure JavaScript (no native addon, so nothing changes for theoven/bunimage), server-only (no client bundle impact), published 2026-07-05 so it clears the 7-dayminimumReleaseAgegate without an exclusion.Type of Change
Testing
grepSpans; regex-syntax cases (character class, anchor, alternation, word boundary, bounded quantifier, wildcard) asserted to be interpreted, not matched literally.lib/logs+lib/copilot;tsc --noEmit,bun run lint:check,bun run check:api-validationall clean.Checklist