fix(transform): don't double-count overlapping filter events in filter_period_intersect by ErikBjare · Pull Request #166 · ActivityWatch/aw-core

ErikBjare · 2026-09-26T17:09:01Z

When filter events overlap, filter_period_intersect intersects an event with each of them in full, so the time they share is returned twice:

events  = [(0 s, 10 s)]
filters = [(0 s, 6 s), (4 s, 4 s)]
before: (0 s, 6 s), (4 s, 4 s)   <- 4-6 s counted twice
after:  (0 s, 6 s), (6 s, 2 s)

The two-pointer walk now only intersects the part of the event that hasn't been returned yet. aw-server-rust used to trim events this way, and ActivityWatch/aw-server-rust#749 (which ports this function to Rust for ActivityWatch/aw-server-rust#747) keeps doing so. This PR makes aw-core match. Codex found it in review of that PR.

Results for non-overlapping filter events, the usual case (not-afk events), are unchanged. That includes zero-duration events inside a filter event: they are still kept, boundaries included. I added a test pinning that, since aw-server-rust#747 was about Rust dropping them.

Parity

The parity suite (ActivityWatch/activitywatch#1467) has overlapping filter events in its random scenarios. With aw-server-rust#749 alone, 19 of the 75 filter_period_intersect xfails flip. With this branch as well, all 75 flip, and there are no new failures.

…r_period_intersect When filter events overlap, an event spanning both was intersected with each of them in full, so the shared time was returned twice: an event [0, 10] filtered by [0, 6] and [4, 8] gave [0, 6] and [4, 8]. Only intersect the part of the event that has not been returned yet, giving [0, 6] and [6, 8]. Results for non-overlapping filter events, the common case (not-afk events), are unchanged. Found in review of ActivityWatch/aw-server-rust#749, which ports this function to Rust.

chatgpt-codex-connector · 2026-09-26T17:09:13Z

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-26T17:21:25.751770Z	`34c8fee`	New commits

ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

greptile-apps · 2026-09-26T17:11:06Z

Confidence Score: 5/5

[Medium risk] Fixes double-counting in time-period filtering logic.

The PR appears safe to merge, with a non-blocking extra-event edge case at an overlapping filter boundary.

Findings

Extra zero-duration event ▶

Summary

The PR tracks the portion of each event already returned so overlapping filter events do not double-count duration, and adds overlap and zero-duration tests.

_{Reviews (2) · Last reviewed commit: "fix(transform): don't double-count overl..."}

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 43e68a3287

ℹ️ 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".

ErikBjare · 2026-09-26T17:13:56Z

@greptileai review

… sharing an end

TimeToBuildBob · 2026-09-26T17:48:45Z

🤖 AI code review

This PR modifies the two-pointer walk in _intersecting_eventpairs in aw_transform/filter_period_intersect.py to avoid double-counting overlapping filter events. It introduces a yielded_until variable that trims the current event's period to the part not yet yielded, and resets it when advancing the event pointer. It also adds two tests: one for overlapping filter events and one for zero-duration events within a filter event.

Not safe to merge — 1 P1 open

Confidence 3/5

2 findings · ❌ 1 P1 · ⚠️ 1 P2

❌ P1 high — aw_transform/filter_period_intersect.py:40

The new yielded_until logic can produce an empty intersection period when the current event's remaining portion is entirely before the next filter event, but the code still yields it. Specifically, when yielded_until is set to ip.end after a partial intersection, and the next filter event starts after the current event's end, the e1_p is trimmed to Timeslot(yielded_until, e1_p.end). If yielded_until equals e1_p.end (e.g., the previous intersection ended exactly at the event's end), then e1_p becomes a zero-duration timeslot. The subsequent intersection with a filter event that starts at or after that point will be empty, but the code still yields the pair with an empty ip. This results in a zero-duration event being emitted from filter_period_intersect, which is incorrect because the event's time was already fully covered by the previous filter event. The test test_filter_period_intersect_overlapping_filters covers a case where the second filter event ends at the same time as the first, but not a case where the second filter event starts after the first ends. For example, with to_filter = [Event(timestamp=now, duration=10s)] and filter_with = [Event(timestamp=now, duration=6s), Event(timestamp=now+6s, duration=4s)], the first intersection yields (0,6s), sets yielded_until=6s. The next iteration trims e1_p to (6s,10s), intersects with the second filter (6s,10s) yielding (6s,10s) correctly. But if the second filter were (now+6s, 0s) (zero duration), the intersection would be empty but still yielded. More realistically, if the second filter starts at now+10s (after the event), the trimmed e1_p is (6s,10s) and the intersection is empty, but the code still yields because ip is truthy? Actually Timeslot.intersection returns a Timeslot; an empty Timeslot is falsy? Need to check. The code checks if ip: so empty intersection is not yielded. However, the issue is that when yielded_until is set to ip.end and the next filter event is entirely after the event's end, the e1_p is trimmed to a zero-length period, and the intersection with the next filter event might be non-empty if the filter event starts exactly at yielded_until? Let's trace: if yielded_until equals e1_p.end, then e1_p becomes Timeslot(e1_p.end, e1_p.end) which is zero-duration. If the next filter event starts at e1_p.end and has positive duration, the intersection is a zero-duration timeslot at that point. The code yields it, producing a zero-duration event. This is a bug because the event's time was already fully covered, so no additional event should be emitted. The test test_filter_period_intersect_zero_duration only tests zero-duration events in the to_filter list, not zero-duration filter events. The PR description says zero-duration events inside a filter event are kept, but this is about filter events with zero duration. The new logic can create spurious zero-duration events when a filter event ends exactly at the end of the event and another filter event starts at that same point. For example, to_filter = [Event(timestamp=now, duration=10s)], filter_with = [Event(timestamp=now, duration=6s), Event(timestamp=now+6s, duration=4s)] works, but if the second filter is (now+6s, 0s) (zero duration), the intersection of the trimmed (6s,10s) with (6s,6s) is (6s,6s) which is zero-duration and truthy? Timeslot with start==end is likely truthy? Need to check timeslot library. The code yields it, producing a zero-duration event at 6s. This is wrong because the time 6s is already covered by the first filter. The fix would be to skip yielding when ip.duration == 0 and the event's original period was fully covered, or to not trim to a zero-length period. But the PR's intent is to avoid double-counting, and this edge case is not covered by tests. This is a real bug in the new logic.

Before yielding, check that `ip.duration > timedelta(0)` or that the intersection is not a zero-length point when the event was fully covered. Alternatively, when `yielded_until >= e1_p.end`, advance `e1_i` without yielding.

_{How this was verified: Traced the logic: when yielded_until equals e1_p.end, the trimmed e1_p becomes zero-length. If the next filter event starts at that same point, the intersection is a zero-length Timeslot, which is truthy in the timeslot library (it represents a point), so it is yielded. This produces a spurious zero-duration event. Checked the timeslot library behavior: Timeslot with start==end is considered non-empty (it has zero duration but is a valid point). The code's if ip: check passes for zero-length intersections.}

⚠️ P2 medium — tests/test_transforms.py:168

The new test test_filter_period_intersect_overlapping_filters does not cover the case where a filter event starts exactly at the end of a previous filter event (adjacent, non-overlapping filters). The PR's fix is specifically about overlapping filters, but the yielded_until mechanism also affects adjacent filters. For events1=[(0,10)], events2=[(0,6),(6,4)], the expected output should be (0,6) and (6,4) — no double counting and no gap. The algorithm: first yield (0,6), yielded_until=6; second e1_p trimmed to (6,10), e2_p=(6,10), yield (6,4). This works. But the test only uses overlapping filters (0,6) and (4,4) which overlap on [4,6]. The adjacent case is not tested, but the algorithm handles it. The zero-duration test only tests zero-duration events in events1, not in events2. The missing test for zero-duration filter events is a coverage gap, but per the three-clause rule, I cannot report it as a test-coverage finding because it does not cross a boundary this PR does not own. The zero-length intersection bug is a real defect, so I report that instead.

_{How this was verified: Reviewed the added tests: they cover overlapping filters and zero-duration events in events1. They do not cover zero-duration events in events2, which is where the empty-intersection bug manifests. This is a test gap, but the underlying defect is already reported.}

Files changed (2) — the diff as I read it

aw_transform/filter_period_intersect.py — Adds yielded_until tracking to trim already-yielded portions of the current event when intersecting with overlapping filter events.
tests/test_transforms.py — Adds tests for overlapping filter events and zero-duration events within a filter event.

_{Reviewed 34c8feeef3e2 · openrouter/deepseek/deepseek-v4-flash-0731 · llm engine · 377s · about this reviewer}

_{Maintainer commands}

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

TimeToBuildBob · 2026-09-26T17:48:46Z

        e2 = events2[e2_i]
        e1_p = _get_event_period(e1)
+        if yielded_until is not None and yielded_until > e1_p.start:
+            e1_p = Timeslot(yielded_until, e1_p.end)


❌ P1 — The new yielded_until logic can produce an empty intersection period when the current event's remaining portion is entirely before the next filter event, but the code still yields it. Specifically, when yielded_until is set to ip.end after a partial intersection, and the next filter event starts after the current event's end, the e1_p is trimmed to Timeslot(yielded_until, e1_p.end). If yielded_until equals e1_p.end (e.g., the previous intersection ended exactly at the event's end), then e1_p becomes a zero-duration timeslot. The subsequent intersection with a filter event that starts at or after that point will be empty, but the code still yields the pair with an empty ip. This results in a zero-duration event being emitted from filter_period_intersect, which is incorrect because the event's time was already fully covered by the previous filter event. The test test_filter_period_intersect_overlapping_filters covers a case where the second filter event ends at the same time as the first, but not a case where the second filter event starts after the first ends. For example, with to_filter = [Event(timestamp=now, duration=10s)] and filter_with = [Event(timestamp=now, duration=6s), Event(timestamp=now+6s, duration=4s)], the first intersection yields (0,6s), sets yielded_until=6s. The next iteration trims e1_p to (6s,10s), intersects with the second filter (6s,10s) yielding (6s,10s) correctly. But if the second filter were (now+6s, 0s) (zero duration), the intersection would be empty but still yielded. More realistically, if the second filter starts at now+10s (after the event), the trimmed e1_p is (6s,10s) and the intersection is empty, but the code still yields because ip is truthy? Actually Timeslot.intersection returns a Timeslot; an empty Timeslot is falsy? Need to check. The code checks if ip: so empty intersection is not yielded. However, the issue is that when yielded_until is set to ip.end and the next filter event is entirely after the event's end, the e1_p is trimmed to a zero-length period, and the intersection with the next filter event might be non-empty if the filter event starts exactly at yielded_until? Let's trace: if yielded_until equals e1_p.end, then e1_p becomes Timeslot(e1_p.end, e1_p.end) which is zero-duration. If the next filter event starts at e1_p.end and has positive duration, the intersection is a zero-duration timeslot at that point. The code yields it, producing a zero-duration event. This is a bug because the event's time was already fully covered, so no additional event should be emitted. The test test_filter_period_intersect_zero_duration only tests zero-duration events in the to_filter list, not zero-duration filter events. The PR description says zero-duration events inside a filter event are kept, but this is about filter events with zero duration. The new logic can create spurious zero-duration events when a filter event ends exactly at the end of the event and another filter event starts at that same point. For example, to_filter = [Event(timestamp=now, duration=10s)], filter_with = [Event(timestamp=now, duration=6s), Event(timestamp=now+6s, duration=4s)] works, but if the second filter is (now+6s, 0s) (zero duration), the intersection of the trimmed (6s,10s) with (6s,6s) is (6s,6s) which is zero-duration and truthy? Timeslot with start==end is likely truthy? Need to check timeslot library. The code yields it, producing a zero-duration event at 6s. This is wrong because the time 6s is already covered by the first filter. The fix would be to skip yielding when ip.duration == 0 and the event's original period was fully covered, or to not trim to a zero-length period. But the PR's intent is to avoid double-counting, and this edge case is not covered by tests. This is a real bug in the new logic.

Checked: this is the intended half-open behaviour, not a bug. With filters (0,6),(6,0) on event (0,10) the result is [(0,6),(6,0)]. A zero-duration filter is a point. Point 3 inside [0,6) would be dropped as already covered, but point 6 isn't in [0,6), so it's emitted, the same as a lone zero filter at 6. It adds 0 s and only changes the event count. aw-server-rust with ActivityWatch/aw-server-rust#749 gives the same result for this case and for (0,10),(10,0), which gives [(0,10)] on both since 10 is outside the event's [0,10). That holds both with and without this PR. Pinned as unit tests in #170 and as parity scenarios in ActivityWatch/activitywatch#1468.

Correction to my reply above about (0,10),(10,0): the result there is [(0,10)] only because the walk has already moved past the event after the full-length filter, not because 10 is outside [0,10). A lone zero filter at 10 gives [(10,0)], so zero-duration pieces at the event's bounds are kept. Both servers do the same (checked against aw-server-rust master), so parity holds. The (0,6),(6,0) explanation stands. Tests updated in #170.

Thanks for the clear explanation. The half-open boundary semantics are correct — a zero-duration filter at point 6 isn't in [0,6) so it emits as a point, matching aw-server-rust behaviour. The P1 finding was a false positive; the tests added in #170 confirm this. Marking resolved.

TimeToBuildBob · 2026-09-26T17:48:46Z

    assert filtered_events[1].duration == timedelta(minutes=15)


+def test_filter_period_intersect_overlapping_filters():


⚠️ P2 — The new test test_filter_period_intersect_overlapping_filters does not cover the case where a filter event starts exactly at the end of a previous filter event (adjacent, non-overlapping filters). The PR's fix is specifically about overlapping filters, but the yielded_until mechanism also affects adjacent filters. For events1=[(0,10)], events2=[(0,6),(6,4)], the expected output should be (0,6) and (6,4) — no double counting and no gap. The algorithm: first yield (0,6), yielded_until=6; second e1_p trimmed to (6,10), e2_p=(6,10), yield (6,4). This works. But the test only uses overlapping filters (0,6) and (4,4) which overlap on [4,6]. The adjacent case is not tested, but the algorithm handles it. The zero-duration test only tests zero-duration events in events1, not in events2. The missing test for zero-duration filter events is a coverage gap, but per the three-clause rule, I cannot report it as a test-coverage finding because it does not cross a boundary this PR does not own. The zero-length intersection bug is a real defect, so I report that instead.

Added the adjacent-filters case (0,6),(6,4) -> [(0,6),(6,4)], 10 s total, in #170 (a separate test file, so it doesn't conflict with this PR), plus a parity scenario in ActivityWatch/activitywatch#1468. Both servers agree on it.

Good — the adjacent-filters case being covered in #170 and the parity scenarios in ActivityWatch/activitywatch#1468 close this gap. P2 resolved.

…duration events (#749) * fix(transform): port aw-core's filter_period_intersect filter_period_intersect skipped every zero-duration event, so a point event inside a filter event (e.g. a zero-duration window event during a not-afk period) was dropped, while aw-core keeps it. The two also differed at zero-duration filter events: aw-core emits a zero-duration part of an event containing one, Rust did not always. Both come from aw-core's use of timeslot's Timeslot.intersection, which treats intervals as closed when one of them has zero duration. Port aw-core's two-pointer algorithm and that intersection so both servers return the same events, and add the repro from the issue plus randomized invariant tests. TimeInterval::intersects is not used here and is unchanged. Fixes #747 * fix(transform): don't double-count overlapping filter events aw-core's two-pointer walk intersects an event with each overlapping filter event in full, so time covered by several filter events was returned twice. The previous Rust implementation trimmed the event instead. Keep that: only intersect the part of the event that has not been emitted yet. Same change as ActivityWatch/aw-core#166. * test(transform): pin filter_period_intersect edge cases with overlapping filters

Zero-duration filter at the end of another filter ((0,6),(6,0)), at the end of the event ((0,10),(10,0)), and adjacent filters ((0,6),(6,4)), from the review of ActivityWatch/aw-core#166. With ActivityWatch/aw-server-rust#749 both servers agree on all three (half-open filters: [(0,6),(6,0)], [(0,10)], [(0,6),(6,4)]); the pinned Rust drops the zero-duration piece, so those cases are RUST_747. The 36 new known failures are attributed with the same measured runs (each fix alone, all, all but one, and the combinations).

…#1468) * test(query-parity): attribute known failures to their measured causes known_issues.py labelled each case with the first matching id pattern, so a case was often blamed on its scenario (period clipping, equal timestamps) when a transform divergence was the real cause, or the other way round. For example period-clip-p1-flood was labelled #162 and does flip with the #162 fix alone, but identical-p0-flood needs both the #163 fix and the Rust flood() port. known_failures.txt now records the issues per case ("A,B" all needed, "A | B" either suffices), and the xfail reason lists them. The keys were measured with the new attribute.py from 23 runs: no fixes, each of the 10 open fix PRs alone (aw-core#161, #165-#169, aw-server-rust#744, #748-#750), all of them, and all but one. Cases that still fail with every fix keep only the causes that have no fix yet (#1466 shapes, ms timestamp precision). The id patterns remain as suggestions for new failures. * fix(query-parity): refuse to attribute from missing or incomplete results A mistyped results directory read as zero failures, and --write then emptied known_failures.txt. Require base.txt (with failures), all.txt and both result files per fix. * fix(query-parity): keep alternatives, flag underdetermined and unverified combinations - When pruning fixed keys from a case that still fails, prune within each alternative instead of flattening "A | B,C" into a conjunction. - A combination that reduces to a single key its solo run already disproves (or that an optional <KEY>+<KEY>.txt run shows is not enough) is reported as underdetermined, and --write refuses. - Reject all-<KEY>.txt without a matching <KEY>.txt. - Optional <KEY>+<KEY>.txt runs verify combinations; the script reports how many are only inferred from the leave-one-out runs. - Unit tests for attribute.py. All 153 combination specs in known_failures.txt are now confirmed by direct runs of the 11 distinct combinations; the list is unchanged. * fix(query-parity): resolve underdetermined cases from the combination runs The underdetermined diagnostic asked for <KEY>+<KEY>.txt runs but never used them. Fall back to the supplied runs that fix the case and contain every needed key, keeping the minimal ones as alternatives (A,B | A,C). * test(query-parity): add filter_period_intersect boundary scenarios Zero-duration filter at the end of another filter ((0,6),(6,0)), at the end of the event ((0,10),(10,0)), and adjacent filters ((0,6),(6,4)), from the review of ActivityWatch/aw-core#166. With ActivityWatch/aw-server-rust#749 both servers agree on all three (half-open filters: [(0,6),(6,0)], [(0,10)], [(0,6),(6,4)]); the pinned Rust drops the zero-duration piece, so those cases are RUST_747. The 36 new known failures are attributed with the same measured runs (each fix alone, all, all but one, and the combinations). * fix(query-parity): attribute every minimal sufficient set of fixes Rework how a case that some fixes flip gets its spec: collect every set of fixes known or inferred to be enough (single fixes, passing combination runs, the leave-one-out set), keep only the minimal ones as alternatives, and report a measured set that may carry an unneeded key. - A case fixed by A alone and by B+C (neither alone) is now "A | B,C"; the combination run used to be ignored when a single fix sufficed. - A passing superset whose extra key the leave-one-out runs show is not needed, and whose smaller run is missing, is reported instead of written. - Redundant alternatives go: "CORE_163 | CORE_161,CORE_163,RUST_744" is just "CORE_163" (4 entries in known_failures.txt). * fix(query-parity): reject duplicate keys and duplicate combination result files CORE_161+CORE_161.txt collapsed to the single fix, and A+B.txt next to B+A.txt silently kept one of them.

chatgpt-codex-connector Bot reviewed Sep 26, 2026

View reviewed changes

Comment thread aw_transform/filter_period_intersect.py

ErikBjare mentioned this pull request Sep 26, 2026

fix(transform): port aw-core's filter_period_intersect, keeping zero-duration events ActivityWatch/aw-server-rust#749

Merged

greptile-apps Bot reviewed Sep 26, 2026

View reviewed changes

Comment thread aw_transform/filter_period_intersect.py

test(transform): pin filter_period_intersect with overlapping filters…

34c8fee

… sharing an end

TimeToBuildBob reviewed Sep 26, 2026

View reviewed changes

This was referenced Sep 26, 2026

test(query-parity): attribute known failures to their measured causes ActivityWatch/activitywatch#1468

Merged

test(transform): pin filter_period_intersect boundary cases #170

Merged

ErikBjare merged commit 7b24386 into master Sep 26, 2026
4 checks passed

		assert filtered_events[1].duration == timedelta(minutes=15)


		def test_filter_period_intersect_overlapping_filters():

Uh oh!

Conversation

ErikBjare commented Sep 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Parity

Uh oh!

chatgpt-codex-connector Bot commented Sep 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codex Review Summary

Uh oh!

greptile-apps Bot commented Sep 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Confidence Score: 5/5

Findings

Summary

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

ErikBjare commented Sep 26, 2026

Uh oh!

Uh oh!

TimeToBuildBob commented Sep 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🤖 AI code review

Not safe to merge — 1 P1 open

Uh oh!

TimeToBuildBob Sep 26, 2026

Choose a reason for hiding this comment

Uh oh!

ErikBjare Sep 26, 2026

Choose a reason for hiding this comment

Uh oh!

ErikBjare Sep 26, 2026

Choose a reason for hiding this comment

Uh oh!

TimeToBuildBob Sep 26, 2026

Choose a reason for hiding this comment

Uh oh!

TimeToBuildBob Sep 26, 2026

Choose a reason for hiding this comment

Uh oh!

ErikBjare Sep 26, 2026

Choose a reason for hiding this comment

Uh oh!

TimeToBuildBob Sep 26, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ErikBjare commented Sep 26, 2026 •

edited

Loading

chatgpt-codex-connector Bot commented Sep 26, 2026 •

edited

Loading

greptile-apps Bot commented Sep 26, 2026 •

edited

Loading

TimeToBuildBob commented Sep 26, 2026 •

edited

Loading