feat: reuse agentic URL classification for referral-traffic topic/category enrichment#2641
Open
cwjwisse wants to merge 4 commits into
Open
feat: reuse agentic URL classification for referral-traffic topic/category enrichment#2641cwjwisse wants to merge 4 commits into
cwjwisse wants to merge 4 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
MysticatBot
reviewed
Jun 11, 2026
There was a problem hiding this comment.
Hey @cwjwisse,
Verdict: Comment - well-structured feature with thorough documentation and tests; a few minor observations below.
Changes: Reuses agentic CDN-logs regex classification rules to enrich both weekly and daily referral-traffic reports with topic/category columns, via a new shared JS classifier module and a consolidated spreadsheet-safety module (15 files).
Non-blocking (4): minor issues and suggestions
- nit:
escapeCsvValueinsrc/common/spreadsheet-safe.jswill prefix a single quote on any value whose string form starts with-(the formula trigger list includes-). No current CSV field in these reports carries negative numbers, so this is theoretical - but worth a comment or guard (!isNaN(value)skip) if a future column could hold negative deltas. -src/common/spreadsheet-safe.js:79 - nit:
site.getSiteId()changed tosite.getId()is an unrelated API migration mixed into the feature PR without mention in the description. Consider noting it in the PR body for reviewability. -src/llmo-referral-traffic/handler.js:133 - suggestion: Add a brief note in
toRegExpthat the RE2J source dialect (Trino) inherently excludes constructs (backreferences, lookaheads) that could bypass the ReDoS heuristics - makes the implicit security argument explicit for future maintainers. -src/common/agentic-url-classification.js:165 - suggestion: Consider adding timing logs around
fetchAgenticUrlClassificationRulesin both handlers - the Postgres fetch latency directly impacts Lambda execution time and is the first thing needed during latency investigations. -src/llmo-referral-traffic-daily/handler.js:228
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 1m 16s | Cost: $5.77 | Commit: 1644b39625848d215d84e73197462797415ad5e8
If this code review was useful, please react with 👍. Otherwise, react with 👎.
Contributor
|
This PR will trigger a minor release when merged. |
…egory enrichment Add topic and category enrichment to both weekly and daily LLMO referral-traffic audits by reusing the existing agentic (CDN-logs) two-tier regex-rule classifier. No per-URL LLM calls; rules are authored once per site and cached in Postgres. - New shared JS classifier src/common/agentic-url-classification.js (JS twin of the Athena/Trino SQL CASE/COALESCE): createClassifier compile-once factory plus classifyTopic/classifyPageType parity surface. Rules-present gate skips enrichment entirely when no rules. - ReDoS defense: compile-time REDOS_HEURISTICS screen plus 2048-char input cap; bad/unsafe patterns dropped (rule -> 'Other'), one aggregated log.warn. No new dependency. - Formula-injection defense: shared src/common/spreadsheet-safe.js (sanitizeSpreadsheetValue, escapeCsvValue, serializeCsv) used by both CSV (daily) and Excel (weekly) paths. - Daily handler: stable 14-column CSV always; topic/category empty when classifier absent. Weekly handler: explicit WEEKLY_COLUMNS, topic/category always present. - Export query-builder SQL helpers for JS-twin parity tests. - ADR docs/decisions/001 and spec docs/specs/referral-traffic-topic-category-enrichment.md. 100% line/branch/statement coverage maintained. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The agentic-daily-export and referral-daily-export specs created esmock module mocks on every test but never purged them. Unpurged esmock registrations leak into the global module registry and cause nondeterministic cross-file pollution when the full suite runs in a single process — surfacing in CI as "log.warn is not a function" and reading ".args" of null in these specs. Track each esmocked module and esmock.purge() it in afterEach, matching the existing convention in handler.test.js, cdn-analysis, preflight, and llmo-config-utils specs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
cwjwisse
force-pushed
the
feat/referral-traffic-topic-category-enrichment
branch
from
7d152fe to
703ad5a
Compare
| const cappedPath = capUrlPath(urlPath); | ||
| return { | ||
| topic: applyTopic(cappedPath, topicCompiled), | ||
| category: applyPageType(cappedPath, pageCompiled), |
Contributor
There was a problem hiding this comment.
category: applyPageType(cappedPath, pageCompiled),
this is actually pagetype and topic is actually category here
the naming are kept due to old convention
- Time the agentic classification-rule fetch in both the weekly and daily referral handlers; the Postgres fetch latency feeds Lambda runtime and is the first signal to check during latency investigations. - Document why a leading '-' is deliberately neutralized in CSV cells (no negative-numeric column today; quoting kept as the safe default). - Note in toRegExp that the RE2J/Trino source dialect excludes backreferences and lookaround, making the implicit ReDoS-safety argument explicit. Non-blocking feedback from MysticatBot on PR #2641. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…t classify() akshaymagapu flagged that the classify() return maps `category` ← page-type and `topic` ← the agentic category rules — the output keys are the inverse of the source-rule names. The module header documents this, but the call site had no inline note. Add one explaining the legacy convention is kept for backward compatibility with downstream report columns. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Reuse the agentic (CDN-logs) URL classification rules to enrich referral-traffic reports with
topic+categorycolumns — both the weekly (llmo-referral-traffic) and daily (llmo-referral-traffic-daily) audits. No page-intent replacement; no new per-URL LLM calls.How
src/common/agentic-url-classification.js— JS twin of the existing SQLCASE WHEN REGEXP_LIKE(...)logic.createClassifier(rules, { log })compiles all patterns once and returns{ classify(urlPath) }, ornullwhen no rules are present.hasClassificationRules()false (null/error/empty) ⇒ enrichment skipped entirely (rows are not tagged'Other').src/common/spreadsheet-safe.jsneutralizes formula-injection cells for both the weekly Excel and daily CSV paths.WEEKLY_COLUMNS, daily 14-colCSV_COLUMNS; topic/category always present, empty''when not classifying.Hardening (from prior review rounds)
toRegExprejects non-string / >1000-char / catastrophic-backtracking patterns (5 heuristics);classifycaps input to 2048 chars. Guards are a screen, not a proof (see ADR caveats).Docs
docs/decisions/001-reuse-agentic-url-classification-for-referral-traffic.mddocs/specs/referral-traffic-topic-category-enrichment.mdCaveats
url; referral classifies onpathonly — host/scheme-dependent patterns won't match.Testing
test/common/agentic-url-classification.test.js,test/common/spreadsheet-safe.test.js.🤖 Generated with Claude Code
Review notes
src/llmo-referral-traffic/handler.jsswitchessite.getSiteId()→site.getId()(the current data-access accessor). Unrelated to the topic/category feature but included here; called out for reviewability per PR feedback.afc04d06, all non-blocking): timing logs around the agentic rule fetch in both referral handlers; doc note on the deliberate CSV-neutralization; doc note that the RE2J/Trino rule dialect excludes backreferences/lookaround (explicit ReDoS-safety rationale).