fix(insights): bind verification to exact measurements - #731
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 9 files
Architecture diagram
sequenceDiagram
participant Agent as Insights Agent
participant Prompt as Agent Prompt
participant Verifier as Verification Checker
participant Analytics as Analytics Tools (get_funnel_analytics)
participant DB as Insights DB (analyticsInsights)
participant Reply as Reply/Scheduled Flow
participant Eval as Eval Runner
Note over Agent,Verifier: Repair with Structured Verification
Agent->>Prompt: Define repair with next.check
Prompt-->>Agent: Goal/funnel check: metric, dates, min entrants, threshold
Agent->>Verifier: CHANGED: Submit verification check
Verifier->>Analytics: Read exact definition with UTC dates
Analytics-->>Verifier: Measured count, entrants, conversion rate
alt Measurement matches saved check
Verifier->>Verifier: CHANGED: Compute verdict (passed/failed/inconclusive)
Verifier-->>Agent: Code-generated summary (not model prose)
else Legacy no structured check
Agent->>Agent: Use prose condition (no invented check)
end
Note over Agent,DB: Persistence with Shared Projection
Agent->>DB: CHANGED: Persist outcome + verification status
DB-->>Reply: CHANGED: Single caseValues projection
Reply->>Reply: CHANGED: Reuse same projection (no duplicate update)
alt Verification passed
Reply->>DB: Label "recovered" only if passed
else Failed or inconclusive
Reply->>DB: Keep "resolved/open" (not recovered)
end
Note over Agent,Eval: Eval with Source Snapshots
Eval->>Eval: NEW: Copy agent.ts, insights.ts, quality.ts to output dir
Eval->>Agent: Run verification scenarios (passed/failed/small-sample/unfinished-window)
Agent-->>Eval: Outcome with verification status matching scenario
Eval->>Eval: Validate code verdict vs expected, not model summary
Note over Agent,DB: Multi-Source Evidence
Agent->>Prompt: CHANGED: Evidence refs may be array of sources
Prompt-->>Agent: Include all contributing periods/populations/mechanisms
Agent->>Agent: Validate every cited source in combined claim
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
|
The latest updates on your projects. Learn more about Unkey Deploy
|
Greptile SummaryThis PR introduces structured goal and funnel recovery checks and binds verdicts to exact analytics metadata rather than model-authored claims.
Confidence Score: 4/5The PR is not yet safe to merge because goal repairs inspected with temporary filters can never satisfy their subsequently generated verification read, and an explicit repository typing requirement remains violated. The verification flow stores request-level goal filters as part of the expected persisted definition but omits them from the later exact read, forcing valid recoveries to remain inconclusive; the newly introduced Files Needing Attention: apps/insights/src/agent.ts, apps/insights/src/evals/quality.ts Important Files Changed
Sequence DiagramsequenceDiagram
participant Agent
participant Analytics
participant Store
participant Resume
Agent->>Analytics: Inspect goal/funnel definition and measurements
Analytics-->>Agent: Counts plus canonical measurement metadata
Agent->>Store: Save action and structured verification check
Resume->>Store: Load latest pending check
Resume->>Analytics: Read exact definition and UTC window
Analytics-->>Resume: Counts, actual dates, and measured population
Resume->>Resume: Validate identity, completeness, sample, and threshold
Resume->>Store: Persist passed, failed, or inconclusive verdict
Reviews (1): Last reviewed commit: "fix(insights): verify actual populations..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 10 files (changes from recent commits).
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 2 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 5 unresolved issues from previous reviews.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
1 participant
Repair follow-ups could claim recovery from an incomplete window, undersampled read, or changed population. This change saves optional goal/funnel verification conditions, binds them to the expected definition, and computes the verdict from an exact native analytics response. The model cannot supply the stored verdict or override its summary.
Existing single-goal/funnel analytics now return their actual measured dates and measured definition. Cache keys include that identity; clipped historic windows and changed filters remain inconclusive. One analytics read can also inspect the definition, removing the separate list lookup for ordinary verification. Failed/inconclusive case closure no longer receives a recovered marker. Native SDK callbacks prevent finishing alongside unseen reads, including when a read already has its own output/input hooks. Resolved website identity comes from the measured metadata, so valid domain aliases work.
Multi-source evidence citations preserve both periods/cohorts in one concise comparison. Scheduled/reply persistence uses one projection and removes duplicate update mappings. There is no new table, agent, replay conversation, or investigation lifecycle.
Scope: one verification/measurement-boundary slice against staging. No dependencies. File overlap with open PR #680 in goals.ts: this PR changes single getAnalytics and its output; #680 owns bulkAnalytics. That bulk path is untouched here.
Fresh evaluation: same 24 synthetic scenarios × 2 per revision through the real model gateway. Turns 108→95; reads 80→64; summed durations 442.6s→386.8s; rejected finishes 12→2. Five false recovery claims in the baseline check scenarios became zero; all 12 candidate machine verdicts and corresponding copy matched the measured condition. Two baseline small-sample cases also lost/misclassified the 300-entrant requirement. The baseline is not penalized merely for lacking the new stored field. Fourteen subsequent confirmations, a final 25-case sweep, three corrected native-funnel repeats and boundary tests cover later review fixes. All 336 exploratory/comparison runs and 738 observable steps were retained and reviewed locally. The first native fixture returned the current period for both period queries; it was corrected, retained and excluded from final native-funnel findings.
Limits: synthetic tool latency is not production latency. Input tokens increased 10%, and four published briefs exceeded 60 words on both sides. Initial goal investigations still mishandle contradictory measurements, and free-text recovery conditions remain weak. Runtime is net +310 lines for the new verification boundary; persistence is 38 lines smaller. This does not establish overall product quality or price fit.
Review dispositions: configured-target fixture evidence is retained at the prior action's original evidence index; current follow-up context is not substituted. Targets absent from all cited evidence remain invalid. Domain-alias and mixed read/finish findings are fixed with boundary tests. SPEC now folds impact into summary while counting legacy impact toward the brief budget. The population-drift fixture allows a new inspected filter repair rather than treating every action as a repeated route edit.
Validation: root lint; all 33 type/build tasks; all 27 test tasks including 337 insights tests; four RPC actual-window/population/cache tests; 38 local PostgreSQL/Redis integration tests. MCP JSON-schema serialization is covered by the existing full test suite. No customer data or notifications were used in evals.
Final review: the agent's production get_goal_analytics input (packages/ai/src/ai/tools/goals.ts) accepts only goalId, websiteId and the date range; its executor forwards exactly those fields. Request-level filters on the public RPC therefore cannot enter an agent repair snapshot through this tool. Existing verification guards still reject extra query filters. The callback now uses the native SDK contract, and the eval reuses insightMeasurementSchema, removing 11 duplicate lines.
Funnel review fixes: native measurements retain step names and stored conditions for repair validation; saved verification uses only evaluated population fields. Regression cases cover native-only repair, a native read after list, lost-condition rejection, published verification and cosmetic rename stability. Filter fields/operators reuse the existing edit constraints. Uninspected checks produce the existing actionable validation message. The goal RPC mock uses the actual goal signature and asserts the fourth argument.
The passed-domain boundary test is retained: native SDK toolResults carry parsed input even when a mock executor ignores it. The verifier previously compared that input directly and rejected the domain alias. This tests the verification boundary, not the unchanged domain resolver.
Final live findings: all six check verdicts/copy remain correct. All three native-funnel repeats preserved conditions and created an 82% check with a 200-entrant minimum in two turns, but none passed the complete usefulness rubric: one unnecessarily renamed a step, two exceeded the brief budget. The final sweep also reproduced the stale-zero story despite fetching 164 current visits, omitted Direct in the source comparison, and had three overlong briefs. These are documented remaining limitations, not reported as quality passes.
The final P3 suggestion to add a condition-only cache-key test is not a new runtime defect. The key already serializes the complete measurement and existing tests assert full condition retention plus invalidation on changed definitions. Native repair tests separately reject lost stored conditions. No condition-specific cache branch exists; another assertion of JSON serialization would duplicate that coverage. Conditions remain excluded from the recovery population comparison because current analytics does not evaluate them.