Skip to content

feat(classify): add regex_fields whole-field AND rule type - #670

Open
TimeToBuildBob wants to merge 1 commit into
ActivityWatch:masterfrom
TimeToBuildBob:feat/category-regex-fields
Open

TimeToBuildBob wants to merge 1 commit into
ActivityWatch:masterfrom
TimeToBuildBob:feat/category-regex-fields

Conversation

@TimeToBuildBob

Copy link
Copy Markdown
Contributor

Summary

Adds a new regex_fields rule variant for category/tag classification that lets users constrain multiple fields simultaneously (logical AND) with whole-field matching semantics.

This addresses the use-case from ActivityWatch/aw-webui#939: when two apps (e.g. mstsc.exe for RDP, winbox.exe for Winbox) share the same window title, the existing regex rule cannot distinguish them because it OR-tests fields. A regex_fields rule with both app and title patterns resolves this exactly.

New rule format

{
  "type": "regex_fields",
  "fields": {
    "app": "mstsc\\.exe",
    "title": "office\\.example\\.com"
  },
  "ignore_case": false
}

All named fields must be present in the event data, be strings, and fully satisfy their pattern (\A(?:PATTERN)\z whole-field anchoring, unaffected by embedded newlines).

Backward compatibility

  • Existing regex rules with OR semantics are completely unchanged.
  • The new variant is strictly opt-in (requires "type": "regex_fields").
  • Rollout hazard guard: the variant explicitly rejects any rule that also contains a regex or select_keys member (old Python silently reads a stale regex member on unknown types).
  • ignore_case uses (?i) prefix (as in RegexRule) since RegexBuilder::case_insensitive is unsupported by fancy_regex.

What changed

  • aw-transform/src/classify.rs: New RegexFieldsRule struct + RuleTrait impl; new Rule::RegexFields variant.
  • aw-query/src/datatype.rs: TryFrom<&DataType> for Rule now dispatches on type == "regex_fields" and builds the rule from a DataType::Dict of field→pattern pairs.
  • 6 new tests covering AND semantics, whole-field anchoring, ignore_case, missing fields, empty-fields error, and embedded newlines.

Testing

cargo test --package aw-transform --package aw-query — 55 tests pass.

Companion PR for aw-core (Python server): ActivityWatch/aw-core#154 (pending)

Closes ActivityWatch/aw-webui#939 (partial — webui editor wiring is a follow-up once capability is advertised)

@greptile-apps

greptile-apps Bot commented Sep 10, 2026

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

The PR appears safe to merge, with no outstanding correctness or repository-rule violations identified.

Summary

  • Parses and validates field-to-pattern dictionaries through the shared query rule conversion.
  • Compiles anchored per-field regular expressions with optional case-insensitive matching.
  • Applies logical AND semantics while rejecting missing and non-string fields.
  • Removes the unrelated datastore compression prototype present at the previous review SHA.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
    A[Rule dictionary] --> B{type = regex_fields}
    B --> C[Validate fields and ignore_case]
    C --> D[Compile anchored regex per field]
    D --> E[Read event data]
    E --> F{Every field exists as a string and matches}
    F -->|Yes| G[Category or tag rule matches]
    F -->|No| H[Rule does not match]
Loading

Reviews (2) · Last reviewed commit: "feat(classify): add regex_fields whole-f..."

Comment thread aw-datastore/src/compress.rs Outdated
Introduce Rule::RegexFields(RegexFieldsRule) alongside the existing
Rule::Regex. The new variant matches events by testing each named field
against its own compiled regex, requiring ALL fields to match (logical
AND). Each pattern is anchored to the entire field value via \A(?:...)\z
so that partial-string matches are rejected without requiring the caller
to add explicit anchors.

Motivation: mstsc.exe and winbox.exe can display the same title string
when connected to the same host. Title-only regexes cannot distinguish
them. Cross-field matching (app AND title) is the minimal change that
lets a user create mutually exclusive RDP vs Winbox categories.

Contract (mirrors aw-core Python implementation):
- type: 'regex_fields', fields: {<field>: <pattern>, ...}, ignore_case: bool
- All named fields must exist as strings in event.data
- Each pattern must match the full field value (not a substring)
- 'regex' and 'select_keys' members are rejected at parse time to guard
  against the identified rollout hazard (old Python reads a stray 'regex')
- Empty fields map is a validation error

DataType parser in aw-query/src/datatype.rs extended to deserialise the
new rule shape; 6 new unit tests added to aw-transform/src/classify.rs.

Git-Session-Id: 3f81
@codecov

codecov Bot commented Sep 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 36.20690% with 37 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.02%. Comparing base (656f3c9) to head (479359f).
⚠️ Report is 101 commits behind head on master.

Files with missing lines Patch % Lines
aw-query/src/datatype.rs 3.03% 32 Missing ⚠️
aw-transform/src/classify.rs 80.00% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #670      +/-   ##
==========================================
+ Coverage   70.81%   79.02%   +8.20%     
==========================================
  Files          51       67      +16     
  Lines        2916     5893    +2977     
==========================================
+ Hits         2065     4657    +2592     
- Misses        851     1236     +385     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

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

@TimeToBuildBob
TimeToBuildBob force-pushed the feat/category-regex-fields branch from adeba0a to 479359f Compare September 10, 2026 06:36
@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

@greptileai review

@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

Greptile is now 5/5 on the current head, every CI check passes, and the prior compression finding is resolved on its thread. No further code change or re-trigger is needed; this is waiting only for maintainer review/merge.

@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

CI-green and mergeable (Greptile 5/5) — waiting only on a maintainer click.

This PR is ready to merge, but the bot has pull-only access to this repo and can't self-merge — surfacing it here so it isn't lost. The monitoring loop will stop re-flagging it now that this note is posted.

@TimeToBuildBob

TimeToBuildBob commented Sep 16, 2026

Copy link
Copy Markdown
Contributor Author

🤖 AI code review

Safe to merge — no P0/P1 findings on latest review

Updated after inline dispositions on finding threads — this is the current state; the verdict below is frozen at review time and is kept as the historical record of that pass.

Finding disposition
Finding Severity State
aw-transform/src/classify.rs:15 P2 accepted-tradeoff

Adds a new regex_fields rule variant to the classification system, enabling multiple fields to be matched with logical AND and whole-field anchoring via a new RegexFieldsRule struct and Rule::RegexFields enum variant. Extends the TryFrom<&DataType> for Rule parser in aw-query/src/datatype.rs to handle the new type, including rejection of stale regex/select_keys members, and adds 6 tests in aw-transform/src/classify.rs.

Needs a look — P2 only

Confidence 4/5

1 finding · ⚠️ 1 P2

⚠️ P2 mediumaw-transform/src/classify.rs:15

Adding a new RegexFields variant to the public Rule enum (without #[non_exhaustive]) is a breaking change for any external crate or code that matches on Rule exhaustively. Previously only None and Regex existed; now exhaustive matches without a wildcard arm will fail to compile. The PR description does not mention this compatibility break. While it is a deliberate feature addition, it violates the API contract for existing callers who rely on the enum being closed. The fix is to mark the enum #[non_exhaustive] or document the change as a breaking release.

Add #[non_exhaustive] to the enum to allow future variants without breaking external matches, or document the breaking change.

How this was verified: Checked the diff and current file: the Rule enum has no #[non_exhaustive] attribute, and the new variant is added without any compatibility note. Searched the repo for exhaustive matches on Rule; all in-repo matches are updated, but external callers are not.

1 advisory finding (summary-only, not scored)

These P2 guard, heuristic, trade-off, or documentation claims are retained for judgment without opening review threads.

⚠️ P2 mediumaw-transform/src/classify.rs:91

RegexFieldsRule::new compiles each pattern directly with Regex::new(&anchored)? and bypasses the REGEX_CACHE that RegexRule::new uses. In a long-running server, the same pattern strings appearing in many rules are recompiled on every rule construction, whereas RegexRule deduplicates via a 512-entry LRU cache. For query-heavy workloads this adds avoidable CPU and allocation overhead, and the lack of a shared cache means the same anchored pattern is stored as a separate Regex object per rule, increasing memory use. This is a trade-off rather than a correctness bug, but it diverges from the existing caching strategy for regex rules.

How this was verified: Compared with RegexRule::new on lines 135-148 of the same file, which uses REGEX_CACHE to reuse compiled Arc<Regex>. RegexFieldsRule::new has no access to the cache and recompiles every pattern.

Files changed (2) — the diff as I read it
  • aw-transform/src/classify.rs — Adds RegexFieldsRule struct with whole-field anchored patterns, Rule::RegexFields variant, RuleTrait implementation, and 6 new tests.
  • aw-query/src/datatype.rs — Adds parsing for regex_fields rule type, building a field-to-pattern map, handling ignore_case, and rejecting stale regex/select_keys members.

Reviewed 479359fae97a · openrouter/deepseek/deepseek-v4-flash-0731 · llm engine · 575s · about this reviewer

Maintainer commands

@TimeToBuildBob review (own line) — fresh review · @TimeToBuildBob fix — a worker acts on the findings. Once per comment; 👀 = received.

Comment thread aw-transform/src/classify.rs
@ErikBjare

Copy link
Copy Markdown
Member

@TimeToBuildBob Does this have a parallel aw-core implementation? Is this a finalized design? Is it really needed/a good idea? We already added something like this (select_keys?)

@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

Answers in order:

Parallel aw-core implementation: yes — ActivityWatch/aw-core#155 is the Python twin with the same rule format (re.fullmatch per field, same type: "regex_fields" payload, same rollout guard). Both are open and parked together.

select_keys is not equivalent: it only narrows which keys a single shared regex searches (values = [e.data.get(k) for k in select_keys], then regex.search — any hit wins). It can't express two different patterns ANDed across fields, which is exactly the aw-webui#939 case: app: mstsc\.exe AND title: office\.example\.com. Sharing one pattern across app+title would false-match (a title containing "mstsc" or an app matching the domain fragment). Whole-field anchoring is the other gap — the existing regex rule is a search, so title: .*office.* style rules over-match.

Needed / good idea?: it's the only way to express the RDP-vs-Winbox disambiguation in rules today without a rule-execution-order hack, and it's strictly opt-in (type: "regex_fields"), so no existing rule changes behavior. That said, the design is yours to judge — if you'd rather generalize differently (e.g. a per-field pattern map under the existing regex type), I'm happy to rework rather than land a second rule type. Not finalized until you sign off; both PRs are holding on that decision.

@ErikBjare

Copy link
Copy Markdown
Member

without a rule-execution-order hack

explain?

a per-field pattern map under the existing regex type

yeah something like that?

Just not fully convinced that this is the design we want to settle into, seems pretty marginal and adds a whole rule type. I was hoping that the existing type could somehow be leveraged to accomplish stuff like this. I feel like there's a lot of prior work and discussion here, not just that one issue you came from. It's maybe worth collating all of that prior information and discussion to actually see what needs we have or what gaps there are in the category rules system.

@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

"Without a rule-execution-order hack" — explained: today, two apps sharing a title (mstsc.exe RDP and winbox.exe both showing office.example.com) can only be separated by exploiting rule ordering: put a narrower rule earlier in the list that claims the RDP events before the generic title rule matches them. Correctness then depends on list position, which is invisible in the UI and silently breaks if rules are reordered, edited, or merged from a preset. That's the hack I meant.

Collation of prior work on the category rules system (what keeps recurring is "which field(s) does this pattern apply to"):

  • aw-webui#286 (2021, merged) — origin of select_keys: match on a specific field (domain/url), not every string field. Field-targeting was the need from day one.
  • aw-core#100 (open) — matching url/editor-payload keys is problematic; rules are really written against app & title. More evidence that field scoping is the core axis, not an edge case.
  • aw-webui#823aw-webui#840 (merged) — exposed select_keys + a field selector in the category editor.
  • aw-server-rust#604 / fix(query): support select_keys in regex rules #605 (merged) — Rust backend parity for select_keys.
  • aw-webui#619 (open) — users want to target a new field (editor project). Recurring pattern: new field, same rule machinery.

The gap select_keys doesn't close: it shares one pattern across the selected keys and OR-matches them (search, any hit wins). It can't express two different patterns ANDed across fields (app: mstsc\.exe AND title: office\.example\.com), and unanchored search over-matches.

Proposal, converging on your instinct: drop the separate regex_fields type and extend the existing regex rule with an optional per-field map:

{"type": "regex", "fields": {"app": "mstsc\\.exe", "title": "office\\.example\\.com"}}

If that shape works for you, I'll rework both PRs to it.

@ErikBjare

Copy link
Copy Markdown
Member

fullmatch semantics, which is how users think about "app equals X"

@TimeToBuildBob But is not how the current regex rules work? What's the most intuitive "anchoring" or "fullmatch" or whatever you call it here?

@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

No — current regex rules are unanchored search (Regex::is_match / re.search over the event's string values, or the select_keys subset). So fullmatch would be new semantics, and that's exactly why I'd drop it in this shape.

For the per-field fields map I'd keep search semantics, same as the existing rule:

  • One mental model per rule type. If type: "regex" means "search" when written bare but "fullmatch" when written as a fields map, the same pattern string behaves differently depending on which syntax it happens to sit under. That's the least intuitive option, and it's the kind of thing that produces rules that "work in the editor, break in production."
  • Explicit anchoring is the regex convention. Users who want exact match write ^mstsc\.exe$ — visible in the rule, standard everywhere, and works per-field for free. Implicit fullmatch hides that behavior and actually blocks legitimate partial patterns (a title fragment like office\.example\.com would never fullmatch a real window title).
  • The real gap is field scoping, not anchoring. The over-match problem from the RDP case was one shared pattern hitting the wrong field. Per-field patterns with search fix that; app: mstsc\.exe via search is already precise.

So the most intuitive anchoring here is: no implicit anchoring — inherit the existing rule's search semantics, let users anchor explicitly with ^/$ when they mean exact. Fullmatch only made sense back when this was a separate regex_fields type whose whole contract was "match the field exactly"; folded into regex as a fields map, search is the consistent default.

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.

Different interpretation of regex in the Activity and Timeline sections

2 participants