refactor(ai): generate the utm builder family from one definition - #743
Conversation
* fix(insights): preserve revenue currency through publication * fix(insights): reject unscoped revenue publication * fix(insights): limit revenue exemption to product outcomes
* feat(rpc): measure saved conversion journeys by cohort * fix(rpc): scope cohort reads to measured endpoints
* feat(insights): discover hidden commercial and conversion changes * perf(insights): bound parallel source reads and cancel native queries * fix(insights): use native source groups for referrer investigations * fix(insights): make unfiltered cohort reads explicit to the model * fix(insights): discover refunds from native signed revenue
The five utm_* builders were 314 lines of config differing in a single column name. Each restated the same fields, percentageOf, groupBy, orderBy, limit, timeField and a ten-entry allowedFilters list. All five now come from utmDimension(). Generated SQL and params are byte-identical for every one of them, verified against a pre-change capture. The drift the duplication was hiding is now visible as two flags rather than buried in prose: utm_terms and utm_content add IS NOT NULL and skip sessionAttribution, which the other three apply. Both behaviours are preserved as-is. The per-builder allowedFilters difference turned out to be uniform once expressed as "include your own column". Two metadata normalisations, neither affecting SQL: utm_mediums gains the title and output_fields it was missing, and utm_campaigns' percentage description matches the other four.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR consolidates the five UTM traffic builders into one configurable definition while preserving their SQL behavior. It also carries broader Insights work that adds currency-scoped commercial signals, bounded funnel-referrer discovery, cohort-aware goal and funnel reads, and stricter evidence and verification handling.
Confidence Score: 4/5The behavioral changes appear sound, but the explicit repository requirement against No behavioral regression was established in the UTM factory, cohort analytics, commercial detection, or referrer lifecycle; the remaining finding is a concrete repository-rule violation in the new commercial row helpers. Files Needing Attention: apps/insights/src/detection.ts Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
Telemetry[Website telemetry] --> Detection[Signal detection]
Detection --> Commercial[Currency-scoped commercial signals]
Detection --> Conversion[Goal and funnel signals]
Conversion --> Referrer[Bounded referrer cohort discovery]
Commercial --> Portfolio[Coverage portfolio]
Referrer --> Portfolio
Portfolio --> Investigation[Evidence-backed investigation]
Investigation --> NativeReads[Native revenue and cohort reads]
NativeReads --> Validation[Evidence and saved-definition validation]
Validation --> Outcome[Persisted investigation outcome]
QueryRequest[UTM query request] --> UTMFactory[Shared UTM builder factory]
UTMFactory --> SQL[Preserved parameterized SQL]
Reviews (1): Last reviewed commit: "refactor(ai): generate the utm builder f..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| current: Record<string, unknown>, | ||
| previous: Record<string, unknown>, |
There was a problem hiding this comment.
The new commercial helpers accept rows as Record<string, unknown> rather than using an explicit commercial overview row type. This violates the repository directive that prohibits unknown and requires proper explicit types. The same unrestricted type is repeated in commercialSignals; this repository requirement must be satisfied before merging.
Context Used: Basic guidelines for the project so vibe coders do... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 24 files
Confidence score: 3/5
- In
apps/insights/src/agent.ts, structured native revenue evidence can let ameasurement_coveragefinding bypass the website publication guard, potentially publishing findings that should remain blocked; restrict the exception toproduct_outcome. - In
apps/insights/src/evals/quality.ts, the native-decline check can pass formeasurement_coverageoruser_experiencebecause it validates only the publish boolean, allowing the regression to go undetected; also assertoutcome.findingKind === "product_outcome". - In
apps/insights/src/coverage-planner.ts, referrer signals are excluded from funnel/goal grouping, so each referrer gets its own group and the funnel aggregate may no longer include them; verify the intended grouping and aggregate behavior.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/insights/src/coverage-planner.ts">
<violation number="1" location="apps/insights/src/coverage-planner.ts:84">
P2: This new branch opts referrer signals out of the funnel/goal grouping, so every referrer signal (subjectKey `funnel:<id>:referrer:<referrer>`) now yields a group unique to that referrer, and the funnel's own aggregate signal keeps group `conversion:funnel:<id>`. Previously all of a funnel's referrers plus its aggregate shared one group, so `planCoveragePortfolio` selected at most one of them. Now a single funnel with several declining referrers can fill most or all of the 5-slot manual portfolio (and the 2-slot scheduled one) and crowd out unrelated signals. The change is untested (no referrer case in `coverage-planner.test.ts`) and unexplained in the PR. Please confirm this de-duplication change is intentional and add a test locking in the intended grouping.</violation>
</file>
<file name="apps/insights/src/agent.ts">
<violation number="1" location="apps/insights/src/agent.ts:1027">
P2: When an attribution signal has structured native revenue evidence, this condition lets a `measurement_coverage` finding bypass the website publication guard. Restrict the native-evidence exception to `product_outcome`; attribution coverage must remain guarded.
(Based on your team's feedback about revenue guard scope.) .</violation>
</file>
<file name="apps/insights/src/evals/quality.ts">
<violation number="1" location="apps/insights/src/evals/quality.ts:1584">
P2: When the native decline publishes with `measurement_coverage` or `user_experience`, this check passes because it only compares the publish boolean. Assert `outcome.findingKind === "product_outcome"` for `revenue-native-decline` so the evaluator catches a regression in the native revenue publication contract.
(Based on your team's feedback about revenue publication finding kinds.)</violation>
</file>
Architecture diagram
sequenceDiagram
participant Agent as Insight Agent
participant Detector as Detection Engine
participant FunnelDet as Funnel/Goal Detection
participant Cohort as Cohort Schema
participant Tools as AI Tools
participant RPC as Analytics RPC (funnels/goals)
participant DB as ClickHouse
Note over Agent,DB: Commercial Signals (Revenue/Refund/Attribution)
Detector->>DB: revenue_overview (both windows)
DB-->>Detector: rows per currency
Detector->>Detector: normalizeCurrencyCode + mapRowsByStringField
alt Valid currency + commercial fields present
Detector->>Detector: makeRevenueSignal / commercialSignals
Detector-->>Agent: signal (revenue:USD, refund_amount:USD, attribution_rate:USD)
else Missing/invalid currency
Detector-->>Agent: no signal (never zero-fill)
end
Note over Agent,DB: Scoped Currency Verification
Agent->>Tools: get_data (revenue_overview, currency filter)
Tools->>RPC: readByCurrency
RPC->>DB: query with currency = USD
DB-->>RPC: rows
RPC-->>Tools: scoped result
Tools-->>Agent: evidence
alt signalKey starts with attribution_rate
Agent->>Agent: require native attributed_revenue + total_revenue evidence
end
Note over Agent,DB: Funnel Referrer Discovery
Detector->>FunnelDet: detectFunnelGoalSignals()
FunnelDet->>DB: funnelConversion (core)
DB-->>FunnelDet: aggregate rates
alt aggregate stable and no signals
FunnelDet->>DB: funnelReferrers (up to 3 funnels, 2 windows)
DB-->>FunnelDet: referrer cohort rows
FunnelDet->>FunnelDet: statistical filter (diff >= 10%, margin >= 3*error)
FunnelDet-->>Detector: referrer signals (funnel:f1:referrer:direct)
end
opt optional probe failure
FunnelDet->>FunnelDet: log failedProbes, continue with goal
end
Note over Agent,DB: Cohort Reads (new capability)
Agent->>Tools: get_funnel_analytics(cohort: {browser: "Safari"})
Tools->>Tools: validate cohort (analyticsCohortSchema)
alt invalid cohort (tenant/step selectors)
Tools-->>Agent: reject
else valid
Tools->>RPC: getAnalytics (funnelId, dates, cohort)
RPC->>DB: load definition
DB-->>RPC: savedDefinition (unfiltered)
RPC->>DB: processFunnelAnalytics (saved filters + cohort)
DB-->>RPC: measurement (definition includes cohort)
RPC-->>Tools: {savedDefinition, measurement, cohort}
Tools-->>Agent: evidence
end
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
| } | ||
|
|
||
| function signalGroup(signal: DetectedSignal): string { | ||
| if (signal.subjectKey?.includes(":referrer:")) { |
There was a problem hiding this comment.
P2: This new branch opts referrer signals out of the funnel/goal grouping, so every referrer signal (subjectKey funnel:<id>:referrer:<referrer>) now yields a group unique to that referrer, and the funnel's own aggregate signal keeps group conversion:funnel:<id>. Previously all of a funnel's referrers plus its aggregate shared one group, so planCoveragePortfolio selected at most one of them. Now a single funnel with several declining referrers can fill most or all of the 5-slot manual portfolio (and the 2-slot scheduled one) and crowd out unrelated signals. The change is untested (no referrer case in coverage-planner.test.ts) and unexplained in the PR. Please confirm this de-duplication change is intentional and add a test locking in the intended grouping.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/insights/src/coverage-planner.ts, line 84:
<comment>This new branch opts referrer signals out of the funnel/goal grouping, so every referrer signal (subjectKey `funnel:<id>:referrer:<referrer>`) now yields a group unique to that referrer, and the funnel's own aggregate signal keeps group `conversion:funnel:<id>`. Previously all of a funnel's referrers plus its aggregate shared one group, so `planCoveragePortfolio` selected at most one of them. Now a single funnel with several declining referrers can fill most or all of the 5-slot manual portfolio (and the 2-slot scheduled one) and crowd out unrelated signals. The change is untested (no referrer case in `coverage-planner.test.ts`) and unexplained in the PR. Please confirm this de-duplication change is intentional and add a test locking in the intended grouping.</comment>
<file context>
@@ -81,6 +81,9 @@ export function coveragePortfolioLimit(
}
function signalGroup(signal: DetectedSignal): string {
+ if (signal.subjectKey?.includes(":referrer:")) {
+ return signal.subjectKey;
+ }
</file context>
| !isVital && | ||
| (!hasNativeRevenueEvidence || | ||
| outcome.findingKind !== | ||
| (signalKey.startsWith("attribution_rate:") |
There was a problem hiding this comment.
P2: When an attribution signal has structured native revenue evidence, this condition lets a measurement_coverage finding bypass the website publication guard. Restrict the native-evidence exception to product_outcome; attribution coverage must remain guarded.
(Based on your team's feedback about revenue guard scope.) .
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/insights/src/agent.ts, line 1027:
<comment>When an attribution signal has structured native revenue evidence, this condition lets a `measurement_coverage` finding bypass the website publication guard. Restrict the native-evidence exception to `product_outcome`; attribution coverage must remain guarded.
(Based on your team's feedback about revenue guard scope.) .</comment>
<file context>
@@ -984,10 +1009,24 @@ function validateAgentOutcome(
!isVital &&
+ (!hasNativeRevenueEvidence ||
+ outcome.findingKind !==
+ (signalKey.startsWith("attribution_rate:")
+ ? "measurement_coverage"
+ : "product_outcome")) &&
</file context>
| ? ["Invented a payment cause or repair"] | ||
| : []), | ||
| ...(outcome.publish | ||
| ...(outcome.publish === shouldPublish |
There was a problem hiding this comment.
P2: When the native decline publishes with measurement_coverage or user_experience, this check passes because it only compares the publish boolean. Assert outcome.findingKind === "product_outcome" for revenue-native-decline so the evaluator catches a regression in the native revenue publication contract.
(Based on your team's feedback about revenue publication finding kinds.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/insights/src/evals/quality.ts, line 1584:
<comment>When the native decline publishes with `measurement_coverage` or `user_experience`, this check passes because it only compares the publish boolean. Assert `outcome.findingKind === "product_outcome"` for `revenue-native-decline` so the evaluator catches a regression in the native revenue publication contract.
(Based on your team's feedback about revenue publication finding kinds.) </comment>
<file context>
@@ -1503,17 +1576,22 @@ for (const attributionLoss of [false, true]) {
? ["Invented a payment cause or repair"]
: []),
- ...(outcome.publish
+ ...(outcome.publish === shouldPublish
? []
- : ["Hid a measured paid-outcome or attribution finding"]),
</file context>
1 participant
The five
utm_*builders were 314 lines of near-identical config differing in a single column name. Each restated the samefields,percentageOf,groupBy,orderBy,limit,timeFieldand a ten-entryallowedFilterslist.traffic.tsgoes 468 → 296 lines (-172).Verification
Generated SQL and params are byte-identical for all five, checked against a capture taken before the change and re-checked after the formatter ran.
packages/aitests: 653 pass, 0 fail. Typecheck clean.Drift the duplication was hiding
Now visible as two flags instead of buried across 314 lines. All preserved as-is, not "fixed":
IS NOT NULLsessionAttributionThat second column is a real behavioural difference (those two compile to ~420 chars of SQL versus ~1600), so it deserves a deliberate decision rather than a silent normalisation. Worth a follow-up.
The per-builder
allowedFiltersdifference turned out not to be drift at all: it is uniformly "include your own column", which is how it now reads.Metadata changes
Two, neither touching SQL:
utm_mediumsgains thetitleandoutput_fieldsit was missing, andutm_campaigns' percentage description now matches the other four.Not done
devices.tshas 12 builders of similar shape, but they are genuinely heterogeneous (mixed single/multi-columngroupBy, limits of 20/25/100, some withoutpercentageOf, plus near-duplicate pairs likebrowser_namevsbrowsers). One factory over those would need six-plus optional knobs and would add complexity rather than remove it.Also carries #740, #741 and #742, already reviewed.
Summary by cubic
Refactors the five
utm_*traffic builders into oneutmDimension()factory and carries three already-reviewed changes: revenue currency preservation, cohort-scoped funnel and goal measurement, and broader commercial and conversion change detection.UTM refactor
traffic.tsdrops from 468 to 296 lines; generated SQL is byte-identical for all five builders.utm_termsandutm_contentaddIS NOT NULLand skipsessionAttribution, which the other three apply.utm_mediumsgains thetitleandoutput_fieldsit was missing;utm_campaigns' percentage description now matches the other four.Carried changes
cohortfilter restricted to context fields, with cohort reads returning the saved definition so the model can verify offline.refund_amountandattribution_ratesignals; referrer reads are cancellable and grouped by native source groups.investigationObjectivethrough planning and generation so evidence distinguishes the objective from human requests.Written for commit 36e7bbe. Summary will update on new commits.