feat(insights): workflow intelligence (corrections, time-to-first-edit, file churn, coverage) + dash wire-up#756
Conversation
…t, file churn, coverage) + dash wire-up
… headline The carry-forward headline (#755) serves totals from the durable day set and merges only session-derived fields from the scan. The new workflow, topReworkedFiles, and pricingCoverage fields were computed by the scan but never propagated, so they silently defaulted on the all-provider path (pricingCoverage rendered a dishonest 1.0). Propagate them like the other scan-only fields and pin it with a carried-headline regression test.
|
Reviewed against post-merge-train main. The feature is sound (median math, correction patterns, privacy claims, and add-only payload all verified, two behaviors mutation-tested), but the three new payload fields were silently dropped on the default all-provider path: the carry-forward headline (#755) serves totals from the durable day set and merges only listed scan-derived fields, and workflow/topReworkedFiles/pricingCoverage were not on the list. On real data they read 0/null/[] and pricingCoverage defaulted to a dishonest 1.0. Fixed in f8c9a6a (propagation plus a carried-headline regression test); verified on a cache copy: corrections 14, TTFE ~5.5 min, 8 reworked files, coverage 0.947, and the headline still equals the history sum. One optional nit left to author discretion: the revert pattern accepts bare 'revert the ...' so ordinary task requests match ('revert the migration'); the sibling undo pattern is tighter. |
ozymandiashh
left a comment
There was a problem hiding this comment.
Ran an in-depth review of this PR (four independent reviewer passes plus my own; every finding below was verified on the code at the cited lines before inclusion). The new code is clean and well tested in isolation, but three issues are blocking.
F1 (HIGH, merge semantics). The branch is cut from a pre-#755 main. The merge with current main is textually clean, but on the default unscoped all-provider path currentData now comes from the daily-cache authority (src/usage-aggregator.ts:296-320 on main) and only estimatedCostUSD, unpricedModels, sessions and the per-model estimated markers are grafted from the scan. The new workflow, topReworkedFiles and pricingCoverage fields are computed only inside buildPeriodData, so after the merge they are dropped on that path and the payload falls back to corrections: 0, correctionRate: null, medianTimeToFirstEditMs: null, topReworkedFiles: [], pricingCoverage: 1. The headline feature reports empty plus a false "100% priced" exactly on the default surface. CI stays green because no test asserts these fields on the cache-authority path. Fix: rebase onto current main and graft the three fields from scanData in the isAllProviders branch (they are derivable solely from surviving sessions, the same rationale the existing comment gives for unpricedModels), plus a test asserting them on that path.
F5 (MED). The correction scanner contradicts its own stated scope. The doc comment and the PR text scope it to user follow-ups, but scanUserCorrections counts every turn with a non-empty userMessage, including the session-opening prompt (src/workflow-insights.ts:40-48), and the revert the... / undo the... patterns (:19-20) match ordinary opening task requests such as "revert the migration we shipped". corrections and correctionRate overstate systematically, and the coaching note keyed on them fires for users who never corrected anything. The false-positive guard list in the tests happens to avoid exactly this case ("undo the migration when done" passes only because "migration" is not in the alternation). Fix: skip the first prompt-bearing turn per session and tighten those two patterns.
F2 (MED). pricingCoverage: current.pricingCoverage ?? 1 (src/menubar-json.ts:478) turns "not computable" into "100% coverage". Make the payload field number | null and default to null.
F6 (MED). The pricingCoverage denominator counts calls of local/free models that findUnpricedModels deliberately excludes (src/models.ts:724-735). A user with 95% ollama calls and 5% unpriced cloud calls reads 0.95 coverage while 100% of their genuinely cost-bearing calls are unpriced. Use the same exclusions for the denominator.
F3 (MED). sessionTimeToFirstEditMs (src/workflow-insights.ts:63-71) skips an unparseable timestamp on the FIRST edit call and returns the time to the first parseable one, which measures "time to first parseable edit". Return null for the session instead, as already happens when turns[0].timestamp is invalid.
F4 (MED). File churn is keyed on the raw model-emitted path (src/parser.ts:1196) with no separator or relative-path normalization, while relativizePath and the payload basename hardcode / (src/workflow-insights.ts:96-103, src/menubar-json.ts:418-420). On Windows the privacy redaction is a no-op: the full absolute path, username included, ships in a payload whose own comment says it can leave the machine. Mixed absolute/relative emissions also split one file's churn across map entries. The parser already normalizes backslashes for project paths (src/parser.ts:60-67), so the same treatment fits here.
Non-blocking notes: buildCoachingNotes re-hardcodes the editTurns >= 5 threshold that worstOneShotCategory already applies; the still failing pattern matches CI logs pasted into prompts; the three new payload fields ship with zero consumers in the repo today (disclosed as add-only, just noting it).
Also verified and fine, to save you the trip: usd() is null-safe, findUnpricedModels skips <synthetic>, avgCostPerSession exists on the payload, modelEfficiency nulls are filtered before the dash, and the deliberately excluded unanswered-sessions metric is fairly disclosed.
…ce (#763) Five verified findings from the #756 review, each with a regression test: Corrections now count only follow-up prompts. The session-opening prompt cannot be correcting this assistant, however correction-shaped its task text reads, and the revert pattern no longer matches ordinary 'revert the <noun>' requests (undo was already guarded; the doc claimed both were). Time-to-first-edit returns null when the first edit's timestamp is unparseable instead of silently measuring to the first parseable one. File churn normalizes backslashed paths before keying, relativizing, and basenaming. Previously the payload's basename-only privacy redaction was a no-op on Windows and shipped the full absolute path, username included; mixed separators also split one file across churn entries. Pricing coverage excludes deliberately-free models (local-looking, local-savings mapped, zero-rate overrides) from the denominator via the same exclusions findUnpricedModels applies, so a mostly-ollama user no longer reads high coverage while every cost-bearing call is unpriced. An absent coverage value now reaches the payload as null, never as a fabricated 100%. Also shares the one-shot minimum-edit-turns threshold between worstOneShotCategory and the coaching gate so they cannot drift.
|
Full disposition of the review, finding by finding. F1 was fixed before merge in f8c9a6a (the three fields are grafted in the isAllProviders cache-authority branch, with a carried-headline regression test). F2 through F6 all verified valid against the merged code and fixed in #763: corrections now skip the session-opening prompt and the revert pattern matches demonstratives only (real-data corrections dropped from 14 to 5, all nine removed being exactly the opener/task-phrasing false positives you predicted); TTFE returns null on an unparseable first edit; churn normalizes backslashes so the Windows basename redaction actually redacts; the coverage denominator uses a new isExpectedFreeModel mirroring findUnpricedModels' exclusions; pricingCoverage is number | null with null for not-computable. The threshold duplication note is also fixed (shared constant). Remaining non-blocking notes (the 'still failing' CI-paste pattern, zero payload consumers) are acknowledged and left as-is. Thanks for the depth on this one; the Windows privacy catch in particular would have shipped. |
Workflow-intelligence metrics for the CLI plus wiring three typed-but-never-rendered fields into the web dash.
New metrics (all computed at report time from existing cached shapes; no parse-version bump, no daily-cache changes)
scanSelfCorrectionspointed at user follow-ups ("that's not what I", "you missed", "revert that", "still broken"...). Conservative pattern list; bare "wrong"/"undo" excluded to avoid false positives in ordinary task text. Count + rate over prompt-bearing turns.current.workflow,current.topReworkedFiles,current.pricingCoverage.Deliberately NOT shipped: "sessions ending without an assistant reply" — the parser drops trailing user-only turns before they reach the cache, so a faithful version needs new parse-time capture + a global cold re-parse for one noisy count (tool-results ride user-role messages). The optional payload slot stays unset.
Dash
avgCostPerSessioncolumn on Top projects, Top models list upgraded to a table exposingsavingsUSD, new Model-efficiency table (costPerEdit,oneShotRate) — all three were in the data contract but rendered nowhere.Verification
CLI suite 2011 passed; 25 new tests (phrase guards, median edges, churn ties, coverage edges, note thresholds). Dash typecheck + build green. Real-machine run: corrections 8 (rate 0.004), median TTFE 5.8 min, coverage 99.99%, churn top = the files this repo's own recent work kept touching. No overlap with #755 (daily-cache untouched by design).