The plan is strong, but I’d revise it in 10 places to improve correctness, scalability, and operator trust. 1. **Add rename/old-path awareness (correctness gap)** Analysis: right now both existing code and your plan still center on `position_new_path` / `new_path` matches (`src/cli/commands/who.rs:643`, `src/cli/commands/who.rs:681`). That misses expertise on renamed/deleted paths and under-ranks long-time owners after refactors. ```diff @@ ## Context -This produces two compounding problems: +This produces three compounding problems: @@ 2. **Reviewer inflation**: ... +3. **Path-history blindness**: Renamed/moved files lose historical expertise because matching relies on current-path fields only. @@ ### 3. SQL Restructure (who.rs) -AND n.position_new_path {path_op} +AND (n.position_new_path {path_op} OR n.position_old_path {path_op}) -AND fc.new_path {path_op} +AND (fc.new_path {path_op} OR fc.old_path {path_op}) ``` 2. **Follow rename chains for queried paths** Analysis: matching `old_path` helps, but true continuity needs alias expansion (A→B→C). Without this, expertise before multi-hop renames is fragmented. ```diff @@ ### 3. SQL Restructure (who.rs) +**Path alias expansion**: Before scoring, resolve a bounded rename alias set (default max depth: 20) +from `mr_file_changes(change_type='renamed')`. Query signals against all aliases. +Output includes `path_aliases_used` for transparency. ``` 3. **Use hybrid SQL pre-aggregation instead of fully raw rows** Analysis: the “raw row” design is simpler but will degrade on large repos with heavy DiffNote volume. Pre-aggregating to `(user, mr)` for MR signals and `(user, mr, note_count)` for note signals keeps memory/latency predictable. ```diff @@ ### 3. SQL Restructure (who.rs) -The SQL CTE ... removes the outer GROUP BY aggregation. Instead, it returns raw signal rows: -SELECT username, signal, mr_id, note_id, ts FROM signals +Use hybrid aggregation: +- SQL returns MR-level rows for author/reviewer signals (1 row per user+MR+signal_class) +- SQL returns note groups (1 row per user+MR with note_count, max_ts) +- Rust applies decay + ln(1+count) + final ranking. ``` 4. **Make timestamp policy state-aware (merged vs opened)** Analysis: replacing `updated_at` with only `COALESCE(merged_at, created_at)` over-decays long-running open MRs. Open MRs need recency from active lifecycle; merged MRs should anchor to merge time. ```diff @@ ### 3. SQL Restructure (who.rs) -Replace m.updated_at with COALESCE(m.merged_at, m.created_at) +Use state-aware timestamp: +activity_ts = + CASE + WHEN m.state = 'merged' THEN COALESCE(m.merged_at, m.updated_at, m.created_at, m.last_seen_at) + WHEN m.state = 'opened' THEN COALESCE(m.updated_at, m.created_at, m.last_seen_at) + END ``` 5. **Replace fixed `24m` with config-driven max age** Analysis: `24m` is reasonable now, but brittle after tuning weights/half-lives. Tie cutoff to config so model behavior remains coherent as parameters evolve. ```diff @@ ### 1. ScoringConfig (config.rs) +pub max_age_days: u32, // default: 730 (or 1095) @@ ### 5. Default --since Change -Expert mode: "6m" -> "24m" +Expert mode default window derives from `scoring.max_age_days` +unless user passes `--since` or `--all-history`. ``` 6. **Add reproducible scoring time via `--as-of`** Analysis: decayed ranking is time-sensitive; debugging and tests become flaky without a fixed reference clock. This improves reliability and incident triage. ```diff @@ ## Files to Modify -2. src/cli/commands/who.rs +2. src/cli/commands/who.rs +3. src/cli/mod.rs +4. src/main.rs @@ ### 5. Default --since Change +Add `--as-of ` to score at a fixed timestamp. +`resolved_input` includes `as_of_ms` and `as_of_iso`. ``` 7. **Add explainability output (`--explain-score`)** Analysis: decayed multi-signal ranking will be disputed without decomposition. Show components and top evidence MRs to make results actionable and debuggable. ```diff @@ ## Rejected Ideas (with rationale) -- **`--explain-score` flag with component breakdown**: ... deferred +**Included in this iteration**: `--explain-score` adds per-user score components +(`author`, `review_participated`, `review_assigned`, `notes`) plus top evidence MRs. ``` 8. **Add confidence/coverage metadata** Analysis: rankings can look precise while data is incomplete (`mr_file_changes` gaps, sparse DiffNotes). Confidence fields prevent false certainty. ```diff @@ ### 4. Rust-Side Aggregation (who.rs) +Compute and emit: +- `coverage`: {mrs_considered, mrs_with_file_changes, mrs_with_diffnotes, percent} +- `confidence`: high|medium|low (threshold-based) ``` 9. **Add index migration for new query shapes** Analysis: your new `EXISTS/NOT EXISTS` reviewer split and path dual-matching will need better indexes; current `who` indexes are not enough for author+path+time combinations. ```diff @@ ## Files to Modify +3. **`migrations/021_who_decay_indexes.sql`** — indexes for decay query patterns: + - notes(diffnote path + author + created_at + discussion_id) partial + - notes(old_path variant) partial + - mr_file_changes(project_id, new_path, merge_request_id) + - mr_file_changes(project_id, old_path, merge_request_id) partial + - merge_requests(state, merged_at, updated_at, created_at) ``` 10. **Expand tests to invariants and determinism** Analysis: example-based tests are good, but ranking systems need invariant tests to avoid subtle regressions. ```diff @@ ### 7. New Tests (TDD) +**`test_score_monotonicity_by_age`**: same signal, older timestamp never scores higher +**`test_row_order_independence`**: shuffled SQL row order yields identical ranking +**`test_as_of_reproducibility`**: same data + same `--as-of` => identical output +**`test_rename_alias_chain_scoring`**: expertise carries across A->B->C rename chain +**`test_overlap_participated_vs_assigned_counts`**: overlap reflects split reviewer semantics ``` If you want, I can produce a full consolidated `v2` plan doc patch (single unified diff against `plans/time-decay-expert-scoring.md`) rather than per-change snippets.