feat(transform)!: match aw-server-rust's merge_events_by_keys, split_url_events and tag by ErikBjare · Pull Request #173 · ActivityWatch/aw-core

ErikBjare · 2026-09-26T21:48:36Z

Implements the decision on ActivityWatch/activitywatch#1466: aw-core adopts aw-server-rust's output shape and semantics for these transforms, and chunk_events_by_key is deprecated in both servers (Rust side: ActivityWatch/aw-server-rust#758).

merge_events_by_keys

Each merged event keeps the first event's timestamp and whole data, not only the merge keys.
Events missing any key are dropped.
[] keys return [].
Groups use the JSON values, like Rust, so 1 and 1.0 differ and list values such as $category work.
Output is in first-seen order. fix(transform): return merge_events_by_keys groups in first-seen order aw-server-rust#757 makes Rust deterministic in the same order; it currently returns HashMap order.

split_url_events

$domain is the host without port, userinfo or leading www.. It's lowercased for http(s)/ws(s)/ftp/file, IPv6 keeps its brackets, and it falls back to the scheme when there's no host (about:blank, file:///x).
$path includes any ;params segment, and is / for special schemes with an empty path.
$params is the query string. $options and $identifier are gone.
Non-URLs, relative URLs, http:// without a host, and non-string url values are left unchanged.
URLs are parsed like the URL Standard, the same as aw-server-rust's url crate, for all schemes:
- special schemes: any slashes or backslashes before the authority (http:example.com), hosts normalized (IPv4 forms like 127.1, compressed IPv6), dot segments resolved, path and query percent-encoded;
- file: URLs: localhost is no host, and Windows drive letters are kept;
- other schemes (chrome://, mailto:, foo://): opaque hosts and paths percent-encoded, IPv6 without the port;
- invalid ports or hosts leave the event unchanged, and one odd URL can never abort the transform.
Non-ASCII hosts go through UTS 46 (non-transitional) with the idna package, a new dependency: pure Python, BSD, and already in the ActivityWatch bundle via requests. Compared against aw-server-rust's own split_url_event over 218 URLs (all of the above plus every review case, including bidi and joiner rules), all fields match, on Python 3.8 and 3.11.
Fields the event already has, including old $options/$identifier, are kept, as in Rust.

tag (and categorize, for malformed rules)

A malformed rule (not a [name, dict] pair) is now a QueryFunctionException instead of an AttributeError crash.
Tag names must be strings. tag(events, [[["Work"], rule]]) now fails with a QueryFunctionException, as in Rust. Category-style list names belong to categorize.
$tags is sorted and deduplicated.

chunk_events_by_key

DeprecationWarning plus a docstring note, and the query function logs a warning once per process. The notice says there is no drop-in replacement: merge_events_by_keys merges across gaps and has no subevents. Behavior is unchanged.

First-party consumers

aw-webui (queries.ts), aw-client (queries.py): already written against aw-server-rust's semantics. They read $category/$domain from merged events, and comments in the Android and multidevice queries rely on events without title being dropped. On aw-server they got uncolored top titles/URLs until now. Their split_url_events use only reads $domain and url. No tag/chunk_events_by_key use.
quantifiedme, aw-research: their $tags come from aw-research's own classifier, not aw_transform.tag. They don't call the other three transforms. (aw-research's queries/aw-development.awq calls merge_events_by_keys(events, "app", "title"), which was never a valid call.)
Breaking for hand-written queries on aw-server that used $options, $identifier or the old $params, relied on merge_events_by_keys keeping events without the keys, or passed list names to tag. They now get what aw-server-rust returns.

Tests and parity

Updated and added tests for each change, including a query-level test for list tag names. 306 passed.

Parity suite (ActivityWatch/activitywatch#1467) against aw-server-rust e7c5439, starting from the 451 known failures in ActivityWatch/activitywatch#1470:

This PR alone: 221 flip, none new.
With fix(query): don't modify events that other query variables still hold #172 (query variables modified in place, found while measuring): 285 flip.
The 166 that remain are 52 timestamp-precision cases (by design), 46 chunk_events_by_key (deprecated, kept as is), 46 tag_list_names (both servers now reject list names, so the suite should mark that case expect_error; I'll do that in the regeneration PR), and 22 multidevice-webui-android-merge (the Rust HashMap order, fixed by aw-server-rust#757).

ErikBjare · 2026-09-26T21:48:38Z

@greptileai review

chatgpt-codex-connector · 2026-09-26T21:48:48Z

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-27T01:40:30.770543Z	`9558a55`	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-26T21:51:17Z

Confidence Score: 5/5

[Medium risk] Refactors event transformation functions to match Rust implementation.

The PR appears safe to merge, with a non-blocking URL-grouping discrepancy for invalid encoded internationalized hosts.

Findings

Encoded hosts bypass bidi checks ▶

Summary

The PR aligns event merging, URL splitting, and tagging with aw-server-rust, improves malformed-rule errors, and deprecates chunking without changing its behavior.

The latest changes add bidi-host validation and reject non-special URLs with credentials or a port but no host.
Bidi validation still misses invalid right-to-left content encoded as an A-label.

_{Reviews (5) · Last reviewed commit: "fix(transform): CheckBidi, leading joine..."}

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e157bd4413

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

TimeToBuildBob · 2026-09-26T22:38:28Z

🤖 AI code review

This PR aligns aw-core's merge_events_by_keys, split_url_events, tag, and categorize with aw-server-rust's semantics. It rewrites merge_events_by_keys to group by JSON-serialized values, keep the first event's full data, and drop events missing keys; rewrites split_url_events with a WHATWG-style parser using the new idna dependency; makes tag names require strings and sorts/dedupes $tags; and deprecates chunk_events_by_key with a one-time log warning. Tests are updated to match the new behavior.

Needs a look — P2 only

Confidence 4/5

⚠️ 1 generated file excluded and NOT reviewed: poetry.lock (+16/-1 lines, 1KB). Machine-generated output is removed from the diff before review — from what the model sees and from the size gate alike — so nothing below says anything about it.

1 finding · ⚠️ 1 P2

⚠️ P2 medium — aw_transform/split_url_events.py:221

In _split_special, the authority is found by scanning for the first of '/', '', '?', '#'. For a URL like 'http://example.com?x/y', the authority becomes 'example.com' and the remainder is '?x/y', which is correct. But for a URL with a userinfo containing a '?' or '#' (e.g. 'http://user?pw@host/'), the scan stops at '?' inside the userinfo, so authority becomes 'user' and the rest '?pw@host/'. The host is then parsed as 'user', which is not a valid host (contains '?'? Actually 'user' is valid), so the event gets $domain 'user' and $path '/' and $params 'pw@host/' — wrong. The URL Standard treats '?' and '#' as ending the authority only after the host, but userinfo can contain them? Actually the URL Standard forbids '?' and '#' in userinfo? Let's check: the URL Standard's authority state ends at '/', '?', '#', or end. Userinfo is parsed before the host, and '?' or '#' would end the authority, so 'http://user?pw@host/' would have authority 'user' and then '?pw@host/' as path/query? Actually the URL Standard would parse 'http://user?pw@host/' as host 'user', path '/', query 'pw@host/'. So the code matches the URL Standard here. So not a bug. Let's find a real bug.

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

aw_query/functions.py — Adds _parse_rules for malformed rule handling, requires string tag names, and logs chunk_events_by_key deprecation once per process.
aw_transform/chunk_events_by_key.py — Adds CHUNK_DEPRECATION constant and emits a DeprecationWarning from chunk_events_by_key.
aw_transform/classify.py — Adds _matching_tags helper that sorts and dedupes matching tag names; tag() and _tag_one use it.
aw_transform/merge_events_by_keys.py — Rewrites merge_events_by_keys to group by JSON values, keep first event's full data, drop missing-key events, and return [] for empty keys.
aw_transform/split_url_events.py — Replaces urlparse-based splitting with a WHATWG-style parser producing $protocol/$domain/$path/$params, with UTS 46 host handling.
pyproject.toml — Adds idna >=3.0 as a runtime dependency for UTS 46 host processing.
tests/test_query2.py — Adds tests for tag string-name requirement, malformed rules as query errors, and one-time chunk deprecation logging.
tests/test_transforms.py — Updates merge_events_by_keys and split_url_events tests for new semantics; adds URL-standard and tag sorting/dedup tests.

Previous review passes

commit	score	findings	engine	when
`e157bd44130e`	4/5	3	llm	2026-09-26 22:34 UTC
`415dbe1eb2f6`	2/5	3	llm	2026-09-27 00:24 UTC

_{Reviewed 9558a55a8d0c · openrouter/deepseek/deepseek-v4-flash-0731 · llm engine · 29s · 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-26T22:38:29Z

+            continue
+        # Group by the JSON values, like aw-server-rust (so 1 and 1.0 differ,
+        # and list values such as categories work).
+        composite_key = json.dumps(values, sort_keys=True, default=str)


⚠️ P2 — In merge_events_by_keys, the composite key is built with json.dumps(values, sort_keys=True, default=str). The default=str fallback means that any value that is not JSON-serializable (e.g. a datetime object, a set, or a custom object) is converted to its string representation. This causes distinct values that share the same string representation to be merged together. For example, two events with data values datetime(2020,1,1) and the string '2020-01-01 00:00:00' would both serialize to the same string and be grouped, even though they are different values. More importantly, the old code used a tuple of raw values, so 1 and 1.0 were distinct (as the new test asserts), but the new code's json.dumps also distinguishes 1 and 1.0 (json.dumps(1) is '1', json.dumps(1.0) is '1.0'), so that part is fine. The real issue is the default=str fallback: it silently coerces non-serializable values, which can cause incorrect merging of events that should be separate. This is a correctness bug for events with non-JSON-serializable data, which can occur in aw-core since Event.data is arbitrary. The observable consequence is that distinct events get merged and their durations summed incorrectly, producing wrong aggregated results.

Fixed in 415dbe1. Event data from the datastore is always JSON, like in aw-server-rust, so this only affects the Python API. Non-JSON values are now tagged with their type in the group key, so a datetime and its lookalike string stay separate (test_merge_events_by_keys_non_json_values_dont_collide). 1 and 1.0 still differ, as in Rust.

TimeToBuildBob · 2026-09-26T22:38:29Z

+    # Tag names are strings, like in aw-server-rust. Category-style list names
+    # belong to categorize (ActivityWatch/activitywatch#1466).
+    for entry in classes:
+        if not isinstance(entry, list) or len(entry) != 2:


⚠️ P2 — In q2_tag, the new validation loop checks that each entry is a list of length 2 and that entry[0] is a string. However, it does not check that entry[1] is a dict. The subsequent try block does classes = [(_cls, Rule(rule_dict)) for _cls, rule_dict in classes], and Rule(rule_dict) will raise a TypeError (not ValueError) if rule_dict is not a dict, e.g. if it's a string or an int. The except clause only catches ValueError, so a TypeError propagates out of q2_tag as an unhandled exception, breaking the query with a traceback instead of a QueryFunctionException. This is a robustness issue: a malformed tag rule (e.g. tag(events, [['name', 'not_a_dict']]) ) will crash the query server. The old code had the same issue (it also only caught ValueError), but the new validation loop is a good place to add a check. This is a real defect because the PR adds validation for tag names but not for rule dicts, and the error handling is incomplete.

Real, fixed in 415dbe1: tag([], [["Work", "not a rule"]]) raised AttributeError ('str' object has no attribute 'get'). categorize had the same bug. Both now go through _parse_rules, which raises QueryFunctionException for entries that aren't [name, dict] pairs, the Python counterpart of aw-server-rust's query error. The new test is test_query2_malformed_rules_are_query_errors.

TimeToBuildBob · 2026-09-26T22:38:29Z

+    assert result[0].data == {"app": "x", "title": "t1", "$category": ["Work"]}
+    assert result[0].timestamp == events[0].timestamp
+    assert result[0].duration == timedelta(seconds=15)
+    assert result[0].id is None


⚠️ P2 — In the test test_merge_events_by_keys_keeps_first_payload, the assertion assert result[0].id is None is checking that the merged event has no id. However, the Event constructor in aw_core.models may assign a default id (e.g. a UUID) if not provided. The test passes currently, but it is a fragile assertion that depends on the Event model's default behavior. If aw_core changes to auto-generate ids, this test will fail. This is a test-defect because it pins an implementation detail that is not part of the contract being tested. The test is not vacuous, but it is brittle. The review criteria say to report defects in tests. This is a minor test-defect.

Keeping this one: a merged event having no id is the contract, not an implementation detail. aw-server-rust's merge_events_by_keys sets event.id = None explicitly, because a merged event doesn't correspond to a stored event, and Python matches it. If Event ever auto-generated ids, the test failing would be the point.

…url_events and tag Decided in ActivityWatch/activitywatch#1466: aw-core adopts aw-server-rust's output shape and semantics for these transforms, and chunk_events_by_key is deprecated in both servers. - merge_events_by_keys keeps the first event's whole data (not only the merge keys) and drops events missing a key; [] merges into []. aw-webui relies on both: top titles are colored by $category and top URLs/browser titles by $domain, and its multidevice/Android queries expect events without a title to be dropped. - split_url_events: $domain is the host without port, userinfo or leading www., $path includes ;params, $params is the query string, $options and $identifier are gone, and non-URLs are left unchanged. - tag: names must be strings (a query with category-style list names is rejected), and $tags is sorted and deduplicated. - chunk_events_by_key: DeprecationWarning and a query log warning; behavior unchanged. BREAKING CHANGE: hand-written queries on aw-server that used $options, $identifier or the old $params, relied on merge_events_by_keys keeping events without the keys, or passed list names to tag, get different results (the same as aw-server-rust).

…lit_url_events

- split_url_events: parse http(s)/ws(s)/ftp URLs like the URL Standard (aw-server-rust's url crate): any slashes or backslashes before the authority (http:example.com), invalid ports and hosts leave the event unchanged, hosts normalized (punycode, IPv4 forms, compressed IPv6), dot segments resolved and path/query percent-encoded. 117/117 URLs give the same fields as aw-server-rust's split_url_event. - merge_events_by_keys: tag non-JSON values (only possible through the Python API) with their type, so they can't merge with a lookalike string. - tag/categorize: a malformed rule is a QueryFunctionException instead of an AttributeError crash, like aw-server-rust's query error. - chunk_events_by_key: log the deprecation once per process, and don't suggest merge_events_by_keys as a replacement (it isn't one).

…ted replacement Queries run repeatedly (dashboard refreshes), so log the deprecation warning once per process. merge_events_by_keys isn't a drop-in replacement (it merges across gaps), so say so instead of recommending it. Same wording as aw-core (from the review of ActivityWatch/aw-core#173).

ErikBjare · 2026-09-26T23:57:16Z

@greptileai review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 415dbe1eb2

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

TimeToBuildBob · 2026-09-27T00:35:09Z

+    # Any number of slashes or backslashes may precede the authority.
+    rest = rest.lstrip("/\\")
+    end = len(rest)
+    for sep in "/\\?#":


❌ P1 — In _split_special, the authority is extracted by finding the first occurrence of any of '/', '\', '?', '#' in the remainder after stripping leading slashes/backslashes. However, the URL Standard's special-scheme authority parsing treats a '?' or '#' as terminating the authority only if it appears before any '/', but the current code uses min() over all separators, so a '?' or '#' that appears before a '/' will cut the authority at that point, even though the standard would include the '/' in the path. Concretely, 'http://example.com?x/y' should have authority 'example.com' and path '/?x/y' (or similar), but this code sets authority='example.com?x' and remainder='y', then _parse_host('example.com?x') fails because '?' is in _FORBIDDEN_HOST, returning None and leaving the event unchanged. This is a correctness bug for URLs with a query before a slash, which are valid per the URL Standard. The test suite does not cover this case. The consequence is that such URLs are not split at all, so $domain/$path/$params are missing for events that should have them.

Checked, and this matches the standard as is: ? and # do end the authority in the URL Standard, and aw-server-rust gives $domain example.com, $path /, $params x/y for http://example.com?x/y (verified with its split_url_event). Python returns the same, and there is now a test for it. No change needed.

TimeToBuildBob · 2026-09-27T00:35:09Z

+        return None
+

 def split_url_events(events: List[Event]) -> List[Event]:


❌ P1 — In split_url_events, the function now only sets $protocol, $domain, $path, and $params, and no longer sets $options and $identifier. This is a breaking change for any existing queries or consumers that relied on $options or $identifier. The PR description explicitly says '$options and $identifier are gone', and the first-party consumers are said to not use them. However, this is a public API change for aw-core's transform, and any external users of aw-core (e.g., plugins or custom queries) that used $options or $identifier will silently get missing fields. The PR description acknowledges this is intentional to match aw-server-rust, but it is still a contract break. The severity is P1 because it breaks existing callers, but the PR explicitly intends this. I'll report it as a contract finding.

Intentional: removing $options and $identifier is what was decided in ActivityWatch/activitywatch#1466 (Python adopts aw-server-rust's shape). The commit is marked BREAKING CHANGE, and the PR description lists who is affected (hand-written queries on aw-server). No first-party consumer reads them.

TimeToBuildBob · 2026-09-27T00:35:09Z

-        if composite_key not in merged_events:
+        try:
+            values = [event.data[key] for key in keys]
+        except KeyError:


❌ P1 — In merge_events_by_keys, the function now drops events that are missing any of the keys. This is a breaking change from the previous behavior, which kept events missing keys (they were grouped under an empty composite key). The PR description says this matches aw-server-rust, and the first-party consumers rely on it. However, this is a contract change for any existing aw-core users who used merge_events_by_keys with events that may lack some keys. The test test_merge_events_by_keys_1 was updated to reflect this. This is intentional, but it is a breaking change. I'll report it as a contract finding.

Intentional, same decision (ActivityWatch/activitywatch#1466): aw-webui's Android and multidevice queries rely on events without the keys being dropped, as aw-server-rust does. Marked BREAKING CHANGE and described in the PR.

…s like the URL Standard From the second review round of #173: - A port must be ASCII digits: "²" crashed int() and aborted the whole transform, and full-width digits were accepted. Any ValueError now leaves the event unchanged instead of failing the query. - Non-ASCII hosts use UTS 46 non-transitional processing (as the URL Standard does) instead of Python's IDNA 2003 codec, so faß.de is xn--fa-hia.de, not fass.de; zero-width joiners follow CheckJoiners. - file: URLs follow the URL Standard: "localhost" is no host, Windows drive letters stay in the path (file:c:/foo, file:C|/x), and ".." never removes the drive. 155/155 URLs give the same fields as aw-server-rust's split_url_event.

ErikBjare · 2026-09-27T00:43:36Z

@greptileai review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 064f21ea89

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

…ial URLs From the third review round of #173: - Hosts go through UTS 46 with the idna package (new dependency, pure Python, BSD, already in the ActivityWatch bundle via requests): mapping removes soft hyphens and rejects disallowed code points, and the URL Standard's label checks reject leading combining marks, bad joiners and invalid A-labels (xn--zz, or ones decoding to disallowed text). ASCII hosts are checked too. - Non-special schemes (foo://, mailto:, chrome://, data:) follow the URL Standard instead of urlsplit: ports validated (invalid ones leave the event unchanged), IPv6 hosts without the port, opaque hosts and paths percent-encoded, dot segments resolved, queries percent-encoded. 191/191 URLs give the same fields as aw-server-rust's split_url_event, on Python 3.8 and 3.11.

ErikBjare · 2026-09-27T01:16:58Z

@greptileai review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8e4c709c0e

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

…rl_events From the fourth review round of #173: - Hosts with right-to-left text must satisfy RFC 5893 bidi rules in every label (UTS 46 CheckBidi, via idna.check_bidi), as in the URL Standard. - A joiner at the start of a label is invalid; the check read the label's last character for i == 0. - In non-special URLs, credentials or a port with an empty host (foo://:80/a, foo://@/a) are invalid and leave the event unchanged. 208/208 URLs give the same fields as aw-server-rust's split_url_event.

ErikBjare · 2026-09-27T01:26:26Z

@greptileai review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0842e699da

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

From the fifth review round of #173: - CheckJoiners uses the full RFC 5892 CONTEXTJ rule (idna.valid_contextj), so a ZWNJ in a joining context (Persian hostnames) is accepted, as in aw-server-rust; the virama-only approximation rejected them. - CheckBidi looks at the Unicode form of every label, so an encoded mixed-direction label (xn--x-zhcd) is rejected too. 218/218 URLs give the same fields as aw-server-rust's split_url_event.

ErikBjare · 2026-09-27T01:37:40Z

Stopping the review rounds here, as decided. The remaining risk is exotic or malformed URLs that browsers don't send. Replacing the hand-written URL parsing with a WHATWG parser (ada-url) once the Python floor reaches 3.10 is tracked in #174, which includes the Rust comparison harness (218/218 URLs match aw-server-rust's split_url_event as of 9558a55).

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9558a55a8d

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

chatgpt-codex-connector · 2026-09-27T01:40:28Z

+
+# Non-special schemes: opaque hosts and paths keep their case, and only the
+# URL Standard's C0 control percent-encode set applies to opaque paths.
+_OPAQUE_HOST_FORBIDDEN = _FORBIDDEN_HOST - {"%"}


Percent-encode controls in opaque hosts

For a non-special URL whose authority contains a control that survives preprocessing, such as foo://a\u000bb/p, this set inherits every C0 character and DEL from _FORBIDDEN_HOST, so the URL is rejected at the host check. The URL-standard opaque-host parser instead percent-encodes these characters (a%0Bb here), as does the parser used by aw-server-rust; consequently this transform leaves a valid event unsplit rather than assigning its derived fields. Keep only truly forbidden opaque-host characters in this set and allow the subsequent percent-encoding step to serialize the others.

Useful? React with 👍 / 👎.

TimeToBuildBob · 2026-09-27T02:37:23Z

+        idx = rest.find(sep)
+        if idx != -1:
+            end = min(end, idx)
+    authority, remainder = rest[:end], rest[end:]


⚠️ P2 — In _split_special, the authority is found by scanning for the first of '/', '', '?', '#'. For a URL like 'http://example.com?x/y', the authority becomes 'example.com' and the remainder is '?x/y', which is correct. But for a URL with a userinfo containing a '?' or '#' (e.g. 'http://user?pw@host/'), the scan stops at '?' inside the userinfo, so authority becomes 'user' and the rest '?pw@host/'. The host is then parsed as 'user', which is not a valid host (contains '?'? Actually 'user' is valid), so the event gets $domain 'user' and $path '/' and $params 'pw@host/' — wrong. The URL Standard treats '?' and '#' as ending the authority only after the host, but userinfo can contain them? Actually the URL Standard forbids '?' and '#' in userinfo? Let's check: the URL Standard's authority state ends at '/', '?', '#', or end. Userinfo is parsed before the host, and '?' or '#' would end the authority, so 'http://user?pw@host/' would have authority 'user' and then '?pw@host/' as path/query? Actually the URL Standard would parse 'http://user?pw@host/' as host 'user', path '/', query 'pw@host/'. So the code matches the URL Standard here. So not a bug. Let's find a real bug.

greptile-apps Bot reviewed Sep 26, 2026

View reviewed changes

Comment thread aw_transform/split_url_events.py Outdated

Comment thread aw_transform/split_url_events.py

Comment thread aw_transform/chunk_events_by_key.py Outdated

Comment thread aw_query/functions.py Outdated

chatgpt-codex-connector Bot reviewed Sep 26, 2026

View reviewed changes

Comment thread aw_transform/split_url_events.py Outdated

TimeToBuildBob reviewed Sep 26, 2026

View reviewed changes

ErikBjare added 3 commits September 27, 2026 01:54

fix(transform): parse special-scheme URLs like the URL Standard in sp…

bd534e3

…lit_url_events

ErikBjare force-pushed the feat/rust-query-shapes branch from e157bd4 to 415dbe1 Compare September 26, 2026 23:56

chatgpt-codex-connector Bot reviewed Sep 27, 2026

View reviewed changes

Comment thread aw_transform/split_url_events.py Outdated

Comment thread aw_transform/split_url_events.py Outdated

Comment thread aw_transform/split_url_events.py Outdated

greptile-apps Bot reviewed Sep 27, 2026

View reviewed changes

Comment thread aw_transform/split_url_events.py Outdated

Comment thread aw_transform/split_url_events.py Outdated

TimeToBuildBob reviewed Sep 27, 2026

View reviewed changes

chatgpt-codex-connector Bot reviewed Sep 27, 2026

View reviewed changes

Comment thread aw_transform/split_url_events.py Outdated

Comment thread aw_transform/split_url_events.py Outdated

Comment thread aw_transform/split_url_events.py Outdated

greptile-apps Bot reviewed Sep 27, 2026

View reviewed changes

Comment thread aw_transform/split_url_events.py Outdated

greptile-apps Bot reviewed Sep 27, 2026

View reviewed changes

Comment thread aw_transform/split_url_events.py

chatgpt-codex-connector Bot reviewed Sep 27, 2026

View reviewed changes

Comment thread aw_transform/split_url_events.py Outdated

Comment thread aw_transform/split_url_events.py

Comment thread aw_transform/split_url_events.py

greptile-apps Bot reviewed Sep 27, 2026

View reviewed changes

Comment thread aw_transform/split_url_events.py Outdated

chatgpt-codex-connector Bot reviewed Sep 27, 2026

View reviewed changes

Comment thread aw_transform/split_url_events.py

Comment thread aw_transform/split_url_events.py

ErikBjare mentioned this pull request Sep 27, 2026

split_url_events: switch to a WHATWG parser (ada-url) when the Python floor reaches 3.10 #174

Open

chatgpt-codex-connector Bot reviewed Sep 27, 2026

View reviewed changes

TimeToBuildBob reviewed Sep 27, 2026

View reviewed changes

		return None


		def split_url_events(events: List[Event]) -> List[Event]:

Uh oh!

Conversation

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

Uh oh!

First-party consumers

Tests and parity

Uh oh!

ErikBjare commented Sep 26, 2026

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

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

Needs a look — P2 only

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ErikBjare commented Sep 26, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ErikBjare commented Sep 27, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ErikBjare commented Sep 27, 2026

Uh oh!

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