feat(roadmap-planner): W2 — fold bot contributions into synthetic bot member#190
Conversation
| if bots.Size() > 0 { | ||
| res, err := contributions.ConsolidateBots(ctx, store, bots) | ||
| if err != nil { | ||
| logger.Warn("bot consolidation backfill failed", zap.Error(err)) |
There was a problem hiding this comment.
Warning (style/error-handling): ConsolidateBots errors are logged as warnings rather than returning a fatal error. If backfill fails, the server continues with potentially inconsistent state. Consider whether this should be fatal for production deployments, or add a startup health check to verify the backfill succeeded.
| placeholders := strings.Repeat("?, ", len(logins)) | ||
| placeholders = strings.TrimSuffix(placeholders, ", ") | ||
|
|
||
| rebind := func(q string) string { |
There was a problem hiding this comment.
Suggestion (docs/readability): The rebind function uses dialect.Placeholder(n) assuming 1-based indexing. Consider adding a brief comment explaining this assumption to improve maintainability.
🤖 AI Code Review
SummaryThis PR implements W2 of the roadmap-planner metrics audit follow-up: consolidating bot contributions (renovate, copilot, etc.) into a synthetic "bot" member to prevent bot activity from skewing human-first-review latency metrics. The implementation adds Review Statistics
Critical Issues
Warnings
Suggestions
Positive FeedbackThe implementation is well thought out:
Note: Since this PR is already merged, these comments serve as documentation for future reference. The merge conflict in config.example.yaml should have been caught before merge and requires immediate remediation. ℹ️ About this reviewThis review was automatically generated using the
|
… member Second workstream of the 2026-05-19 metrics audit follow-up. Resolves finding B4 (56% of pr_reviews rows are automated, dragging the team-wide first-review p50 to ~0.1h). Adds two new schema columns (migration 0005), an explicit team_analytics.bot_logins config knob, and the contributions/bot.go package that owns both the live predicate (BotSet) and the one-shot retroactive consolidation pass (ConsolidateBots). Live ingest changes: - github.Syncer.IsBotLogin / gitlab.Syncer.IsBotLogin predicates. When a PR's author_login matches, author_id becomes "bot". Same for review rows: reviewer_id="bot" and is_bot=1. - pull_requests.first_human_review_at is computed live as MIN(submitted_at) over non-bot reviews. Retroactive backfill at startup (idempotent): - pr_reviews: tag is_bot=1, rewrite reviewer_id to "bot" where login matches. - pull_requests: rewrite author_id to "bot" where login matches; populate first_human_review_at from non-bot reviews on each PR. NetworkDensity now: - Reads first-review latency from first_human_review_at (so renovate's instant comments stop collapsing p50 to ~0.1h). - Joins with rv.is_bot=0 in the cross-pillar review query. Activation: - Empty bot_logins -> no behaviour change (BotSet.Size()==0 is a no-op). - Populated list -> live ingest reclassifies + startup pass updates historical rows. Tests: - BotSet truth table (empty, exact, case-fold). - ConsolidateBots: human PR with bot+human reviews -> is_bot tagged, reviewer_id rewritten, first_human_review_at = human's review time (not the earlier bot review). Bot-authored PR -> author_id="bot". Idempotency: re-running consolidates nothing. Doc: CHANGES.md gets the W2 block describing config, visible effects, and rollback path. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
danielfbm
force-pushed
the
feat/metrics-w2-bot-consolidation
branch
from
ee78c5b to
df4d8bf
Compare
| @@ -125,6 +125,7 @@ github: | |||
| private_key_path: "" # path to the PEM file (mount via Secret) | |||
There was a problem hiding this comment.
CRITICAL (bug/merge-conflict): Unresolved merge conflict markers (<<<<<<< HEAD, =======, >>>>>>> ee78c5b) present in this file. This will cause YAML parsing failures when the config is loaded. The conflict must be resolved before this PR can be deployed.
| placeholders := strings.Repeat("?, ", len(logins)) | ||
| placeholders = strings.TrimSuffix(placeholders, ", ") | ||
|
|
||
| rebind := func(q string) string { |
There was a problem hiding this comment.
Suggestion (style/code-structure): The rebind function is complex and handles dialect-specific placeholder translation. Consider extracting this into a dedicated helper or adding more documentation about which dialects require this transformation.
| if bots.Size() > 0 { | ||
| res, err := contributions.ConsolidateBots(ctx, store, bots) | ||
| if err != nil { | ||
| logger.Warn("bot consolidation backfill failed", zap.Error(err)) |
There was a problem hiding this comment.
Warning (style/error-handling): ConsolidateBots errors are logged as warnings rather than returned, meaning startup continues even if the backfill fails. This may hide data inconsistency issues in production. Consider adding a startup health check or returning the error.
2 participants
Second workstream of the 2026-05-19 metrics audit follow-up — resolves B4 (56% of
pr_reviewsrows are bots, dragging team-wide first-review p50 to ~0.1h).Summary
0005_first_human_review.sql):pr_reviews.is_bot— tagged whenreviewer_loginmatchesteam_analytics.bot_logins.pull_requests.first_human_review_at—MIN(submitted_at)over non-bot reviews.team_analytics.bot_logins []stringconfig +contributions.BotSetpredicate. Exact + case-folded match, no suffix magic (confirmed by maintainer).author_id/reviewer_id→"bot"and setis_bot/first_human_review_atlive during sync.contributions.ConsolidateBotsruns once on every startup as a retroactive backfill (idempotent) so pre-W2 rows get the same treatment without a re-sync.NetworkDensityreads fromfirst_human_review_atand joins withrv.is_bot = 0.Activation rules
bot_logins: []→ no behaviour change. Predicate is a no-op; backfill returns immediately.Visible effects when enabled
botrow appears in the team table with the volume of automated work (renovate alone is hundreds of PRs in any 6-month window). Already in the W1 allowlist.Rollback
bot_logins: []and restart → predicate disables; new rows ingest under raw login. Schema additions stay (no destructive migration).Test plan
go test ./...+go vet ./...— greenTestBotSet— empty / exact / case-fold / whitespaceTestConsolidateBots— human PR with mixed bot+human reviews, bot-authored PR, idempotency on rerunbot_logins: [alaudabot, alaudaa-renovate, edge-katanomi-app2[bot], copilot-pull-request-reviewer[bot], kilo-code-bot[bot], copilot]— verify backfill counts + first-review p50 climbsbotmember shows up in/teamwith non-trivialprs_mergedandprs_reviewedfirst_review_latency_p50_hoursreflects human review timeDepends on
Independent of W1 (
#189). W1's allowlist already includes the syntheticbotmember, so this PR's volume row stays visible when both ship.Unlocks
review_latency_p50_hours(per-reviewer week p50 offirst_human_review_at - created_at).🤖 Generated with Claude Code