fix: normalize keyword numeric IDs before hex rule - #91
Conversation
leo-aa88
left a comment
There was a problem hiding this comment.
REQUEST CHANGES
The diagnosis of the fragmentation bug is correct, but the fix trades it for a worse, undetected regression. normalize_message applies rules sequentially, feeding each rule's output into the next, so moving the keyword-ID rule from position 10 to position 3 doesn't just move it ahead of the hex rule — it moves it ahead of six rules it used to run after: hex, token, IPv4, IPv6, email, and the request_id= KV rule. The PR only reasoned about the interaction with hex.
I built both rule lists (main vs. this PR) and ran them against realistic log lines:
main (before): 'user 192.168.1.1 accessed order 12345678' -> 'user <ip> accessed order <hex>'
this PR (after): 'user 192.168.1.1 accessed order 12345678' -> 'user <id>.168.1.1 accessed order <id>'
'user 192.168.1.5 logged in' -> 'user <id>.168.1.5 logged in'
'session 10.0.0.1 established' -> 'session <id>.0.0.1 established'
'account 172.16.5.9 suspended' -> 'account <id>.16.5.9 suspended'
The keyword-ID regex matches greedily up to the first non-digit after the keyword — for an IP address that's just the first octet, since \d+\b stops at the dot. Running before the IPv4 rule, it eats "192" and leaves ".168.1.1" behind, a fragment the IPv4 rule can no longer recognize (it needs all 4 octets), so the mangled remainder reaches the final normalized message unfixed. See inline comment for detail, including why this is a worse bug than the one being fixed (a new false-merge across genuinely distinct IPs, not just corrupted text) and a narrower fix that avoids it entirely.
Also: this PR converts the entire file to CRLF line endings alongside the real 5-line fix (confirmed by diffing both versions with line endings normalized — main is LF/ASCII, this branch is CRLF/UTF-8). AGENTS.md is explicit that this repo doesn't want purely-cosmetic-reformatting bundled into functional PRs; here it's not purely cosmetic since there's a real fix inside it, but the line-ending conversion inflated the diff to 89/86 lines and obscured the actual 5-line change — I had to strip \r myself to find it. Please re-save with LF line endings before merge, independent of the regression above.
| # otherwise an 8+ digit decimal ID (e.g. "user 12345678") is swallowed | ||
| # by the hex rule and normalized to <hex>, fragmenting equivalent lines | ||
| # ("user 12345678 not found" vs "user 4567 not found") across clusters. | ||
| (re.compile(r"\b(user|account|order|transaction|session|job|task|worker|tenant|customer|invoice)\s+(?:id\s+)?#?(\d+)\b", re.IGNORECASE), r"\1 <id>"), |
There was a problem hiding this comment.
BLOCKING. This regex now runs before the IPv4/IPv6/token/email/request_id= rules (it used to run after all of them), and matches greedily up to the first non-digit after the keyword — for an IP address, that's just the first octet (\d+\b stops at the dot). That silently corrupts any keyword + IP address message and merges genuinely distinct source IPs into one fingerprint:
'user 192.168.1.1 accessed order 12345678' -> 'user <id>.168.1.1 accessed order <id>' (this PR)
'user 192.168.1.1 accessed order 12345678' -> 'user <ip> accessed order <hex>' (main, before this fix)
By the time the IPv4 rule runs, the first octet is already <id> and the remaining .168.1.1 fragment no longer matches the 4-octet IPv4 pattern, so it passes through unfixed into the normalized message shown as evidence. Worse than the fragmentation bug being fixed: two log lines with different first octets but the same last three (user 192.168.1.5 vs user 10.168.1.5) now normalize identically and silently merge into one cluster — a false merge across genuinely different source IPs, not just a display glitch.
tests/unit/test_normalization.py has a keyword-ID case and a standalone-IP case, but no case where a keyword is immediately followed by an IP — the exact interaction this reorder breaks, which is why the full suite still passes on this branch despite the regression.
A narrower fix avoids this entirely and doesn't require reasoning about the other five rules this reorder now runs ahead of: the real root cause is that the hex pattern ([0-9a-fA-F]{8,}) also matches pure-decimal runs, since 0-9 are valid hex characters — a decimal ID was never actually hex. Require at least one real hex letter instead of reordering:
(re.compile(r"\b(?!\d+\b)[0-9a-fA-F]{8,}\b"), "<hex>"),Verified this resolves the original bug without the regression:
'user 12345678 not found' -> 'user <id> not found'
'user 4567 not found' -> 'user <id> not found'
'user 192.168.1.1 accessed order 12345678' -> 'user <ip> accessed order <id>'
'commit abcdef1234567890 pushed' -> 'commit <hex> pushed'
Please also add a keyword + IP address regression test alongside the existing keyword-ID and standalone-IP cases so this interaction has coverage either way.
Summary
Fixes the clustering fragmentation bug where numeric IDs of 8+ digits normalize to
<hex>instead of<id>.Root cause: in
src/core/normalization/patterns.py, the generic hex rule (�[0-9a-fA-F]{8,}�) ran before the keyword-ID rule. Because the hex pattern also matches all-decimal strings, any numeric ID with 8+ digits became<hex>, while shorter IDs became<id>— so equivalent lines split into separate clusters:Fix: moved the keyword-ID rule (
user|account|order|... <digits>→� <id>) to run before the hex rule. Now:Real hex strings (e.g.
commit abcdef1234567890 pushed) are unaffected and still normalize to<hex>.Changes
src/core/normalization/patterns.py: reordered rules (keyword-ID before hex)Verification
user 12345678 not found/user 987654 not found/user 4567 not found→ all<id>(same fingerprint)session 12345678 expired→<id>;job id 12345 done→<id>;order #99999→<id>commit abcdef1234567890 pushed→<hex>(unchanged)