fix(core): per-workflow metrics read zero on a renamed board - #2866
Conversation
aggregateWorkflowAnalytics filtered in SQL on t."column" = 'done' and IN ('in-progress','in-review').
On a renamed board those match nothing, so tasksCompleted / tasksInProgress / tasksInReview come back
ZERO for every workflow while the board is busy. Nothing errors.
Same shape and same fix as the team-analytics one: resolve per PROJECT via
resolveProjectColumnsForRoles, bind an IN list, thread the store from the single Command Center
caller so the parameter has a supplier immediately.
WHAT THE TEST CAUGHT THAT I HAD NOT. The renamed case still failed WITH the query fixed. The bucketing
at lines 296-297 already uses isWipColumnRole / isReviewColumnRole, but those need
`query.columnFlagsByName`, which production supplies and my fixture did not. So the SQL decides which
rows come back and the trait map decides which bucket each lands in — both halves have to be right,
and testing only the query half would have shipped a conversion that still reported zero. That is the
partial-conversion shape this program keeps re-finding, caught here by asserting tasksInReview
alongside tasksCompleted rather than the completed count alone.
MEASURED: reverted, only the renamed case fails; the default-vocabulary control, the hold-lane
negative, and the omitted-store legacy case all stay green. With the fix, 4 passed.
The sync SQLite arm keeps its literals: it throws in backend mode and has no production caller.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Greptile SummaryThis PR updates PostgreSQL workflow analytics to resolve renamed completion and active lanes.
Confidence Score: 4/5The PR is not yet safe to merge because shared lane IDs can still inflate completion metrics or erase valid active and review counts across workflows. The completed query applies a project-wide lane union before grouping by workflow, while conflicting active lanes are removed from the query because the project-level classifier cannot represent workflow-specific traits; both previously reported metric failures therefore remain reachable. Files Needing Attention: packages/core/src/workflow-analytics.ts and packages/dashboard/src/routes/register-command-center-routes.ts
|
| Filename | Overview |
|---|---|
| packages/core/src/workflow-analytics.ts | Adds project-resolved PostgreSQL lane filters, but the two previously reported multi-workflow metric errors remain reachable. |
| packages/dashboard/src/routes/register-command-center-routes.ts | Supplies the scoped store to workflow analytics while continuing to omit conflicting column traits from the project-level classifier map. |
| packages/core/src/tests/postgres/workflow-analytics-renamed-lanes.pg.test.ts | Covers renamed and legacy vocabularies plus hold-lane exclusion, but does not exercise conflicting lane roles across workflows. |
| scripts/lib/sql-column-literals-baseline.json | Reduces the expected workflow-analytics SQL literal count to match the converted queries. |
| .changeset/workflow-analytics-renamed-lanes.md | Adds the required patch changeset with labeled operator and developer-facing details. |
Reviews (4): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile
|
Warning Review limit reached
Next review available in: 6 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
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 |
…t conflicting ids #2866 review, second finding — a self-contradiction rather than an imprecision. `resolveProjectColumnsForRoles` unions every column any workflow gives the role, so an id two workflows reuse with DIFFERENT traits stays in the SQL filter. `columnFlagsByName` deliberately DROPS such an id as conflicting (its own note, #2803 review: a merged entry double-counts). Together they SELECT rows and then classify them by nothing — the flags lookup misses, both role predicates fall back to legacy ids, `checking` matches neither, and the count evaporates. Worse than either half: the filter says the lane counts, the classifier says it cannot say, and the operator sees zero. Aligned on the classifier's answer, which is the conservative direction: a column it refuses to judge is not selected, rather than selected and discarded. UNCOVERED, AND INHERENTLY SO. Mutating the filter away leaves the renamed-lane suite green because the counts were already zero on both sides; the only observable difference is whether a workflow with no other rows appears with zeros or is absent. Pinning that would assert an artifact of `ensureSummary`, not the invariant. The FIRST finding — a project union admitting one workflow's rows into another's completed count — is real and NOT fixed here. Unlike team-analytics these rows carry a workflow id, so a per-workflow lane map is feasible and is the right fix; it changes this function's contract with its caller, so it is recorded on the PR rather than folded into a review response. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Both real. The second is fixed (004b913); the first is real, feasible, and deliberately not folded into a review response. Conflicting lanes lose active counts — fixedThis one is a self-contradiction, not an imprecision. Together they select rows and then classify them by nothing: the flags lookup misses, Aligned on the classifier's answer, which is the conservative direction: a column it refuses to judge is not selected at all, rather than selected and discarded. This ships uncovered, and that is inherent rather than an omission. Mutating the filter away leaves the renamed-lane suite green — the counts were already zero on both sides. The only observable difference is whether a workflow with no other rows appears with zeros or is absent, and a test pinning that would be asserting an artifact of Project-wide lanes inflate completions — real, not fixed hereConfirmed, and unlike I did not do it in a review-response commit because it changes this function's contract with its caller: the lane resolution has to be keyed by workflow rather than by project, which moves work across the Recorded at the call site so it reads as known debt with a named fix, not as an oversight. Core |
This branch removes legacy comparisons, and the gate (#2841, merged after this was opened) fails on a LOWERED count so a migrated slot cannot be silently regrown into. Without this the branch turns main red on merge, which is what #2864 did. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Added a baseline re-record for the SQL column-literal gate (#2841), which merged after this PR was opened. That gate fails on a lowered count — deliberately, so a migrated site cannot leave an unused allowance behind for the surface to regrow into. This branch removes legacy comparisons, so without the re-record it turns Baseline entries are per file and this PR only touches its own, so it can merge in any order relative to the other analytics conversions. The coupling was mine to spot when I opened five PRs against a gate that did not exist yet, and I did not. |
**`pnpm test:gate` and both `pretest` hooks fail on `main` right now.** Merge this first. ## What happened #2841 (the SQL gate) merged, then #2864 merged. #2864 removed three legacy comparisons from `team-analytics.ts`, but its baseline entry still allows six — and this gate **fails on a lowered count by design**, so a migrated slot cannot be silently regrown into later. Baseline 30 → 28. ## This is my sequencing error The four analytics conversions were branched and reviewed **before** the gate existed, so none of them carries a baseline update. The gate then landed first, which means **each of them breaks `main` as it merges**. I opened all five without thinking about the order they would land in. The three still open — #2866, #2870, #2871 — will each do this again. I am adding baseline updates to them next so they land clean. ## Note on the downward check The "count went down" failure looks like pedantry until it fires. It exists so a migrated site cannot leave an unused allowance behind for the surface to regrow into — the same rot as an allow-list entry for a deleted function. The real cost is that a conversion and its gate have to land in a known order, which is a coupling I created and did not plan for. ## Verification `pnpm test:gate` green with the re-recorded baseline · lint 0 · `node scripts/check-sql-column-literals.mjs` exit 0. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…enamed board (#2870) Third of the 14 lane-bound SQL sites from #2839, after #2864 and #2866. Independent of both. ## The defect `aggregateProductivityAnalytics` filtered its duration query on `"column" = 'done'`. On a renamed board that matches nothing, so the entire task-duration distribution — median, p90, average, total — is computed from an **empty row set** and reports zeros while the project ships work. Nothing errors. Same shape and fix as the previous two: resolve per **project** via `resolveProjectColumnsForRoles`, bind an `IN` list, thread the store from the single Command Center caller so the parameter has a supplier immediately rather than becoming an inert seam. ## Measured Reverted, only the renamed case flips: ``` ✓ default vocabulary: a finished task contributes to the duration stats × renamed vocabulary: a task in the RENAMED complete lane contributes ✓ renamed vocabulary: a task still in the WIP lane does NOT contribute ✓ without a lane store, the legacy id still answers Tests 1 failed | 3 passed (4) ``` ## The negative asserts the median, not just the count This fix's failure mode is **worse than the bug it fixes**. Resolving too many lanes would pull unfinished work into the distribution and produce a plausible-but-wrong median — a number nobody questions — where the bug produces an obvious zero. So the WIP-lane case asserts `medianMs` is null as well as `completedTasks` being 0. ## A fixture error worth naming My first version asserted `taskDuration.count`. `TaskDurationSummary` exposes `completedTasks`. Every case failed with `expected undefined to be 1` — **including the controls** — which reads exactly like a broken product until you notice the control is failing too. A control that fails is a fixture bug, not a finding; that asymmetry is the fastest way to tell them apart. ## Scope The sync SQLite arm keeps its literal: it throws in backend mode and has no production caller, the same dead-arm conclusion as `cleanupStaleMergeQueueRowsImpl` on #2839. ## Verification `pnpm test:gate` green · both Command Center analytics suites 8/8 · `tsc` core 0, dashboard 0 · lint 0 · changeset included. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
# Conflicts: # scripts/lib/sql-column-literals-baseline.json
) Fourth and last of the lane-bound analytics sites from #2839, after #2864, #2866 and #2870. ## The defect `aggregateGithubIssueAnalytics` and its GitLab twin filtered their resolved-issue query on `"column" = 'done'`. On a renamed board that matches nothing, so `fixed` is **zero**, the resolved-issue list is empty, and `net` reports every filed issue as still outstanding — while the team closes issues all week. Nothing errors. Same fix as the previous three: resolve per **project** via `resolveProjectColumnsForRoles`, bind an `IN` list, thread the store from each Command Center caller so the parameters have suppliers immediately. ## Both providers in one change, deliberately These two files are **copies** — same query, only the provider literal differs — and a copy is exactly what gets half-fixed. Converting one and not the other type-checks, passes that provider's test, and leaves the second silently broken with no signal anywhere. The suite runs every case against both, so the pair cannot drift. ## Measured Reverted, exactly the two renamed cases fail — **one per provider** — while both default-vocabulary controls, both WIP-lane negatives, and both omitted-store legacy cases stay green: ``` ✓ github: default vocabulary counts a resolved issue × github: renamed vocabulary counts a resolved issue ✓ github: renamed vocabulary does NOT count an issue still in the WIP lane ✓ github: without a lane store, the legacy id still answers ✓ gitlab: default vocabulary counts a resolved issue × gitlab: renamed vocabulary counts a resolved issue ✓ gitlab: renamed vocabulary does NOT count an issue still in the WIP lane ✓ gitlab: without a lane store, the legacy id still answers Tests 2 failed | 6 passed (8) ``` That the failures are symmetric is itself the check on the copy-paste risk. ## Scope The sync SQLite arms keep their literals: they throw in backend mode and have no production caller, the same dead-arm conclusion as `cleanupStaleMergeQueueRowsImpl` on #2839. ## Verification `pnpm test:gate` green · Command Center + GitLab issue analytics suites 10/10 · `tsc` core 0, dashboard 0 · lint 0 · changeset included. --- **This closes the lane-bound half of #2839.** All 14 sites the hand-review identified as genuinely vocabulary-bound are now converted across four PRs. What remains there is the 11 `!= 'archived'` exclusions, which are probably correct as literals — archiving writes `task.column = 'archived'` unconditionally as a state rather than a lane — plus one dead SQLite arm. Those need per-site judgment, not conversion. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…NXC gate (#2949) Two things, and the second is why the first does not ship alone. ## The opt-in `resolveProjectColumnsForRoles` gains `untraitedProject: "declared-columns"`. When **no** workflow in the project expresses **any** lifecycle trait, every declared column id joins the answer. This is the three-state rule at **project** scope — the last item on the deferred list, recorded at three self-healing call sites (#2869, #2876). A board that renames its lanes and declares no traits contributes nothing today, so its cards are **absent from every role-keyed query**, and the correct per-card fallback downstream never runs for them. A fallback cannot rescue a card the query never returned. **Not "no workflow declares this role."** A project that expresses traits and has no review lane has *answered*; widening there would invent lanes it deliberately lacks. Mutation-verified both directions — widening unconditionally fails 1 of 12, making the option a no-op fails 1 of 12. **Opt-in, not default**, because the safe direction differs per caller — the finding in `project-union-versus-per-task-lanes.md`: | caller | over-inclusion costs | |---|---| | sweep | nothing — the per-card check discards the extra rows | | aggregator | an inflated number an operator reads (#2864, #2866) | | action site | a card routed or notified under a vocabulary that is not its own (#2852, #2891) | Making it the default moves all three at once, in the one direction two of them must not. Verified byte-identical without the option, so this lands with **no caller changes** and each site adopts it on its own reasoning. ## Main was red, and my own gate caught me first I dated the new comments `2026-07-31` while today is `2026-07-30` — **the exact defect `check-fnxc-future-dates` exists to prevent, committed while writing the feature.** The gate I added yesterday failed my own commit. Correcting mine surfaced that the merged sentinel batch, #2947, and three engine test files carried future-dated stamps too, so **the gate was failing on `main` for everyone**, not just here. All corrected to real dates rather than raising the ceiling. The stamps were simply wrong, and a baseline bump would have recorded the error as permitted — which is the failure mode that ratchet exists to prevent. Core and engine `tsc` clean, `pnpm lint` clean, census `--strict` 0, FNXC gate 0 (469 known, none added), gate green (161/487/13/71). Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Second of the 14 lane-bound SQL sites from #2839, after #2864. Independent of it — different file, different caller argument.
The defect
aggregateWorkflowAnalyticsfiltered in SQL ont."column" = 'done'andIN ('in-progress','in-review'). On a renamed board those match nothing, sotasksCompleted,tasksInProgressandtasksInReviewcome back zero for every workflow while the board is busy. Nothing errors.Same shape and same fix as #2864: resolve per project via
resolveProjectColumnsForRoles, bind anINlist, and thread the store from the single Command Center caller so the parameter has a supplier immediately rather than becoming an inert seam.What the test caught that I had not
The renamed case still failed with the query fixed.
The bucketing at lines 296–297 already uses
isWipColumnRole/isReviewColumnRole— correctly converted — but those readquery.columnFlagsByName, which production supplies and my fixture did not. So:Both halves have to be right. Fixing only the query would have shipped a "conversion" that still reported zero on a renamed board, and the file would have scored as converted twice over. That is exactly the partial-conversion shape this program keeps re-finding — caught here only because the test asserts
tasksInReviewalongsidetasksCompleted, since those two paths take different resolved sets (complete vs wip+human-review). Asserting the completed count alone would have left the second conversion unproven.Measured
Reverted, only the renamed case flips:
The hold-lane negative is there so resolving real lanes cannot degrade into "every column counts" — trading an undercount for an overcount is harder to notice than the original bug.
Scope
The sync SQLite arm in the same file keeps its literals: it throws in backend mode and has no production caller, the same dead-arm conclusion reached for
cleanupStaleMergeQueueRowsImplon #2839.Verification
pnpm test:gategreen · both Command Center analytics suites 8/8 ·tsccore 0, dashboard 0 · lint 0 · changeset included.🤖 Generated with Claude Code