fix(insights): preserve revenue currency through publication - #740
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
The latest updates on your projects. Learn more about Unkey Deploy
|
|
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 preserves currency-scoped revenue identities across detection, signal preparation, remeasurement, and publication validation, while expanding detector, investigation-flow, and quality-evaluation coverage.
Confidence Score: 4/5The PR is not yet safe to merge because legacy unscoped revenue subjects can publish evidence for an arbitrary currency, and the explicit repository requirements must also be satisfied. Currency preservation and remeasurement are coherent, but the publication bypass treats bare Files Needing Attention: apps/insights/src/agent.ts, apps/insights/src/investigation-flow.test.ts, apps/insights/src/evals/quality.ts Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[revenue_overview current and previous rows] --> B[Match canonical currencies]
B --> C[DetectedSignal subjectKey revenue:CURRENCY]
C --> D[prepareInvestigation preserves signalKey]
D --> E[Currency-filtered remeasurement]
E --> F[Structured total_revenue evidence]
F --> G{Signal key matches evidence currency?}
G -->|Scoped key| H[Permit product-loss publication]
G -->|Legacy unscoped key| I[Currently also permitted]
G -->|Wrong currency or subject| J[Reject publication]
Reviews (1): Last reviewed commit: "fix(insights): preserve revenue currency..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
f29b1b9 to
11a4cd1
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found and verified against the latest diff
Confidence score: 3/5
- In
apps/insights/src/agent.ts, native revenue signals classified asmeasurement_coverageoruser_experiencecan bypass the website guard even when structured gross-revenue rows do not prove collection or implementation, weakening signal validation; require collection/implementation evidence before applying the exemption. - In
apps/insights/src/evals/quality.ts, module loading now runs the real detector and can throw when no synthetic revenue signal is found, so importingqualityCasesmay break evaluation or test tooling; move detection into an explicit execution path or otherwise avoid throwing during import.
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/agent.ts">
<violation number="1" location="apps/insights/src/agent.ts:992">
P2: When a native revenue signal is classified as `measurement_coverage` or `user_experience`, this exemption bypasses the website guard even though structured gross-revenue rows are not collection/implementation evidence or a downstream experience measurement. Restrict the exemption to `product_outcome` findings, or add equivalent classification validation before skipping the guard.</violation>
</file>
<file name="apps/insights/src/evals/quality.ts">
<violation number="1" location="apps/insights/src/evals/quality.ts:1424">
P2: The real detector now runs as a top-level side effect at module load: importing quality.ts to reuse `qualityCases` executes `detectSignals` and throws (`if (!revenueSignal) throw`) if the synthetic revenue signal is not emitted, and `prepareInvestigation` runs before any case executes. This couples module import to detector execution and makes any non-main consumer (e.g., a test importing `qualityCases`) fail at import time rather than at evaluation. Compute these once per run and guard them, or document that the module is not safely importable.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 4 files (changes from recent commits).
Shadow auto-approve: would require human review.
Re-trigger cubic
1 participant
Native revenue detections lost their currency and became generic website signals. Swapping unchanged USD/EUR result rows could fabricate a revenue change, while the traffic publication guard hid a successfully verified gross-revenue decline.
Match canonical currency rows across both windows, retain
revenue:USD-style identities through preparation and rechecks, and let successful structured gross-revenue evidence satisfy publication for that exact revenue subject. Missing currency rows and legacy unscoped rechecks remain inconclusive. The signal context now states that a snapshot alone is not publication evidence, avoiding conflict with the general snapshot-reuse instruction.The eval suite now runs the real detector and signal preparation with synthetic reads and records that setup alongside every observable model step. All 51 fresh runs / 171 steps were manually reviewed, including intermediate variants. On the final source head, nine matched native cases change verified-decline publication from 0/3 to 3/3 and keep stale/unavailable findings private 6/6. Turns fall 38 → 29, corrections 7 → 0, and input tokens 313,452 → 228,875. Summed model-run time was 136.8s → 91.2s; the small synthetic sample does not establish production latency. Manual review still rejects one final summary for implying that refunds reduced gross revenue; two unavailable runs add unnecessary discovery. Earlier attribution controls omit depth or exceed the word target. These are retained quality failures, not a claim of product readiness.
Review corrections restrict the exemption to product_outcome for the exact scoped currency, reject legacy unscoped revenue, type the recorded setup, flatten scenario metadata conditionals, and run detector setup only in the standalone eval process. Static fixture imports do not execute detection.
Validation: root lint, all 33 type-check/build tasks, and 378 insights tests pass; the pre-push hook also passed all 27 monorepo test/build tasks. Regression cases cover row order, two independent currencies, currency-preserving remeasurement, missing/invalid identity, and publication rejection for wrong subjects, wrong currency, refund-only selections, snapshot prose and unrelated finding categories.
Scope: investigation revenue detection/publication and its evaluation coverage. Based on merged #737; no outstanding dependency or overlapping open product PR. No schema, new agent, lifecycle, notification or deployment changes. Remaining limitations: independent refund/attribution detection with stable gross, absent-currency measurement, and semantic usefulness beyond these synthetic cases. AI-assisted implementation and review under maintainer authorization.