Skip to content

fix: harden linear token sequence matcher - #111

Open
jbachorik wants to merge 4 commits into
mainfrom
agent/logs-reggie-l1a
Open

fix: harden linear token sequence matcher#111
jbachorik wants to merge 4 commits into
mainfrom
agent/logs-reggie-l1a

Conversation

@jbachorik

Copy link
Copy Markdown
Collaborator

Summary

  • make cached LinearTokenSequenceMatcher execution state per-call and make bracket-tail scanning linear
  • admit the named-capture LTS route only when every Grok-observable named group has a capture-producing plan operation
  • add concurrency, projection, and JDK-differential coverage for the expanded Grok access-log fixtures

Why

Grok consumes named capture indexes only. The named-only projection preserves those indexes, but the LTS plan could previously select a route that did not populate every named group. This change declines that route and lets the normal strict compiler choose the implementation instead.

Validation

  • ./gradlew --rerun-tasks :reggie-codegen:test --tests '*LinearTokenSequencePlanTest' :reggie-runtime:test --tests '*LinearTokenSequenceMatcherTest' --tests '*LinearTokenSequenceAccessLogTest'
  • ./gradlew jacocoVerify build

@codecov-commenter

codecov-commenter commented Jul 16, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.87179% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.4%. Comparing base (f437ac8) to head (5d31eba).

Files with missing linesPatch %Lines
...ghq/reggie/runtime/LinearTokenSequenceMatcher.java95.0%0 Missing and 3 partials ⚠️
...ggie/codegen/analysis/LinearTokenSequencePlan.java93.7%0 Missing and 1 partial ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #111 +/- ##
=======================================
Coverage 84.3% 84.4% Complexity 1 1 =======================================
Files 147 147 Lines 42922 42960 +38 Branches 5845 5851 +6 =======================================
+ Hits 36213 36260 +47 + Misses 4919 4916 -3 + Partials 1790 1784 -6 
Files with missing linesCoverage Δ
.../com/datadoghq/reggie/runtime/RuntimeCompiler.java88.3% <100.0%> (+<0.1%)⬆️
...ggie/codegen/analysis/LinearTokenSequencePlan.java81.7% <93.7%> (+1.9%)⬆️
...ghq/reggie/runtime/LinearTokenSequenceMatcher.java76.9% <95.0%> (+4.7%)⬆️

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f437ac8...5d31eba. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

CopilotAI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR hardens the runtime Linear Token Sequence (LTS) matcher to be safer under concurrency and more correct for Grok-style “named-only” capture projection, while adding targeted regression/performance tests around these behaviors.

Changes:

  • Make LTS execution state per-invocation (removing shared scratch arrays) and make bracket-tail scanning linear-time.
  • Gate runtime LTS routing on whether the plan actually covers all named capture indexes (avoid selecting an LTS route that can’t populate Grok-observable named groups).
  • Add new correctness, JDK-differential, and concurrency tests for named-only routing and bracket scanning edge cases.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file
FileDescription
reggie-runtime/src/main/java/com/datadoghq/reggie/runtime/RuntimeCompiler.javaRefines LTS routing by requiring plans to cover all named capture indexes before selecting the LTS matcher.
reggie-runtime/src/main/java/com/datadoghq/reggie/runtime/LinearTokenSequenceMatcher.javaRemoves shared scratch state, introduces per-call workspace, and replaces bracket scanning with a linear pass.
reggie-codegen/src/main/java/com/datadoghq/reggie/codegen/analysis/LinearTokenSequencePlan.javaAdds coversCaptureIndexes(...) to verify a plan has capture-producing ops for required group indexes (including inside optional sequences).
reggie-codegen/src/test/java/com/datadoghq/reggie/codegen/analysis/LinearTokenSequencePlanTest.javaAdds unit tests validating capture-coverage semantics (optional nesting, named-group coverage, invalid indexes).
reggie-runtime/src/test/java/com/datadoghq/reggie/runtime/LinearTokenSequenceMatcherTest.javaAdds routing regression test for named-only coverage plus new bracket-scanning edge/perf tests.
reggie-runtime/src/test/java/com/datadoghq/reggie/runtime/LinearTokenSequenceMatcherConcurrencyTest.javaIntroduces concurrent-use stress tests for cached LTS matchers and nested optional rollback behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@jbachorik
jbachorik marked this pull request as ready for review August 14, 2026 09:53

@chatgpt-codex-connectorchatgpt-codex-connectorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit:6d47a27d3f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Restores the allocation-free contract for matches()/find()/findFrom()
by reusing a ThreadLocal<MatchWorkspace> instead of allocating per call.
Result-returning methods (match()/findMatchFrom()) copy arrays before
returning, preserving thread-safety without aliasing.
@jbachorik
jbachorik requested review from Zenithar and removed request for ZenitharAugust 14, 2026 14:47
Sign up for freeto 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.

3 participants

@jbachorik@codecov-commenter