Skip to content

refactor(mcp): use the shared pattern predicate for drill-downs - #3663

Open
alex-fedotyev wants to merge 3 commits into
alex/FR-385-shared-pattern-predicatefrom
alex/FR-385-mcp-shared-pattern-predicate
Open

alex-fedotyev wants to merge 3 commits into
alex/FR-385-shared-pattern-predicatefrom
alex/FR-385-mcp-shared-pattern-predicate

Conversation

@alex-fedotyev

@alex-fedotyev alex-fedotyev commented Nov 13, 2026 •

Copy link
Copy Markdown
Contributor

Switches clickstack_event_patterns to the shared pattern predicate from #3662 and deletes the MCP copy of templateToRegex, buildPatternPredicate and MAX_PREDICATE_TEMPLATE_LENGTH. Part of FR-385.

Stack

Part 2 of 4. Each PR is based on the one before it, so its diff shows only its own step. Review in order:

  1. feat(common-utils): share the Event Patterns drill-down predicate #3662 Share the pattern predicate in common-utils
  2. refactor(mcp): use the shared pattern predicate for drill-downs #3663 MCP uses the shared predicate (this PR)
  3. feat(common-utils): add a compact form of the pattern predicate #3664 Compact form of the predicate, short enough for a URL
  4. feat(app): filter for or exclude a pattern from Event Patterns #3665 Filter for or exclude a pattern from Event Patterns

Please merge in order: once a PR merges, the next one is retargeted to main. CI and the tier label only run on PRs whose base is main, so PRs 2 to 4 get them at that point. Until then the test plan lists what I ran locally.

Summary

Pure refactor. MCP mines without extra delimiters, so the whereSnippet it returns for each pattern is unchanged. PATTERN_MATCH_SEMANTICS, andSql and userWhereAsSql stay in patternDrilldown.ts, since only MCP uses them.

Test plan

  • unit: the existing patternDrilldown.test.ts tests import from common-utils and pass as they are
  • Compared the deleted MCP functions with the shared ones on 5,000 generated templates (wildcards, punctuation, quotes, backslashes, tabs, newlines, non-breaking spaces, CJK): no difference in the regex or the SQL, and a template over the length limit still gets no predicate
  • jest src/mcp in packages/api (29 suites, 578 tests), tsc --noEmit, eslint on the changed files, knip

clickstack_event_patterns now imports templateToRegex,
buildPatternPredicate and MAX_PREDICATE_TEMPLATE_LENGTH from
@hyperdx/common-utils/dist/drain, and its own copy is deleted. MCP mines
without extra delimiters, so the whereSnippet it returns is unchanged. The
existing patternDrilldown tests import from common-utils and pass as they
are.

Part of FR-385.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@vercel

vercel Bot commented Nov 13, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
hyperdx-oss Ready Ready Preview Nov 13, 2026 11:45pm UTC
hyperdx-storybook Ready Ready Preview Nov 13, 2026 11:45pm UTC

Request Review

@changeset-bot

changeset-bot Bot commented Nov 13, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: eda26c9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@hyperdx/api Patch
@hyperdx/app Patch
@hyperdx/otel-collector Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@@ -1,11 +1,11 @@
import { minePatterns } from '@hyperdx/common-utils/dist/drain';

import {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 minor — Tests for the common-utils predicate now sit in the api package, so common-utils' own suite doesn't cover them

Move the templateToRegex and buildPatternPredicate cases into packages/common-utils/src/__tests__/pattern-predicate.test.ts and keep only the andSql tests here. Today that file only tests extra delimiters and parsePatternPredicate. These cases cover escaping, quotes and backslashes, Unicode whitespace, empty templates and MAX_PREDICATE_TEMPLATE_LENGTH. Because they import the built @hyperdx/common-utils/dist/drain, changing pattern-predicate.ts and running the common-utils tests can't catch a regression in any of them. They also only run against whatever dist was last built.

Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keeping these here: they pin the exact whereSnippet this tool returns, and the common-utils suite now also covers the length limit and the no-option round trip (04c8a76).

@github-actions

Copy link
Copy Markdown
Contributor

PR Review

If you are a coding agent acting for the author, read this first.
Nothing below blocks merge automatically; a maintainer decides. What they will expect:

  • 🔴 critical or 🟠 major in code this PR adds or changes: fix before asking for review.
  • 🔵 minor in code this PR changes: your call. Fix if small, otherwise reply.
  • Anything about surrounding code, or asking you to widen the change (hoist a helper,
    dedupe with another file, fix other call sites): never fix here, whatever the
    severity. Reply with one sentence; if it is critical, say so plainly so a human sees it.

One commit per review round. After two rounds, stop and ask a maintainer to review
scope rather than addressing more automated findings. Full rule: AGENTS.md.

1 finding(s): 🔴 0 critical · 🟠 0 major · 🔵 1 minor

1 posted as inline comment(s) on the changed lines.


Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides.

@github-actions

Copy link
Copy Markdown
Contributor

Deep Review

✅ No critical issues found.

This is a behavior-preserving refactor. The MCP clickstack_event_patterns tool now sources templateToRegex, buildPatternPredicate, and MAX_PREDICATE_TEMPLATE_LENGTH from @hyperdx/common-utils/dist/drain and deletes its local copies. Multiple reviewers independently verified the equivalence mechanically: the shared templateToRegex(template, options?) with options omitted makes separatorClass(undefined) collapse to exactly the old JS_WHITESPACE class ('\s\x{0B}\pZ\x{FEFF}'), so the generated regex and match(...) SQL are byte-identical to the deleted implementation. Both call sites — buildPatternPredicate(bodyColumn, p.pattern) at runEventPatterns.ts:468 and minePatterns(...) at runEventPatterns.ts:314 — pass no extra delimiters, so tokenizer and predicate separators agree. The removed escapeSqlString import is no longer referenced, the retained local exports (andSql, userWhereAsSql, PATTERN_MATCH_SEMANTICS) are all still consumed, and the test import split is consistent. No actionable findings at any severity.


Reviewers (7): correctness, testing, maintainability, project-standards, kieran-typescript, agent-native, learnings-researcher.

Notes (non-blocking, no action required):

  • Equivalence depends on both call sites invoking the shared functions with no options; a future caller that passes extraDelimiters to minePatterns without matching buildPatternPredicate would diverge, but no such caller exists in this diff.
  • After this diff, the only direct assertions of the no-options buildPatternPredicate path (match wrapping, SQL escaping, MAX_PREDICATE_TEMPLATE_LENGTH boundary) live in packages/api/src/mcp/tools/query/__tests__/patternDrilldown.test.ts; common-utils's suite mostly covers the with-delimiters path. Coverage is preserved here, but a later "dedup" of that file should not assume redundancy.
  • Prior PR comments were automated only (Vercel, changeset bot); nothing required verification.

…mcp-shared-pattern-predicate

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…mcp-shared-pattern-predicate

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

This branch was successfully deployed

2 active deployments
Preview – hyperdx-oss — eda26c96 Deployed Nov 13, 2026 by vercel[bot]
Preview – hyperdx-storybook — eda26c96 Deployed Nov 13, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant