docs: update TUI PRD, time-decay scoring, and plan-to-beads plans
TUI PRD v2 (frankentui): Rounds 10-11 feedback refining the hybrid Ratatui terminal UI approach — component architecture, keybinding model, and incremental search integration. Time-decay expert scoring: Round 6 feedback on the weighted scoring model for the `who` command's expert mode, covering decay curves, activity normalization, and bot filtering thresholds. Plan-to-beads v2: Draft specification for the next iteration of the plan-to-beads skill that converts markdown plans into dependency- aware beads with full agent-executable context.
This commit is contained in:
teernisse
@@ -2,12 +2,12 @@
|
||||
plan: true
|
||||
title: ""
|
||||
status: iterating
|
||||
iteration: 5
|
||||
iteration: 6
|
||||
target_iterations: 8
|
||||
beads_revision: 1
|
||||
related_plans: []
|
||||
created: 2026-02-08
|
||||
updated: 2026-02-09
|
||||
updated: 2026-02-12
|
||||
---
|
||||
|
||||
# Time-Decay Expert Scoring Model
|
||||
@@ -70,7 +70,8 @@ Author/reviewer signals are deduplicated per MR (one signal per distinct MR). No
|
||||
1. **`src/core/config.rs`** — Add half-life fields + assigned-only reviewer config to `ScoringConfig`; add config validation
|
||||
2. **`src/cli/commands/who.rs`** — Core changes:
|
||||
- Add `half_life_decay()` pure function
|
||||
- Restructure `query_expert()`: SQL returns hybrid-aggregated signal rows with timestamps (MR-level for author/reviewer, note-count-per-MR for notes), Rust applies decay + `log2(1+count)` + final ranking
|
||||
- Add `normalize_query_path()` for input canonicalization before path resolution
|
||||
- Restructure `query_expert()`: SQL returns hybrid-aggregated signal rows with timestamps and state multiplier (MR-level for author/reviewer, note-count-per-MR for notes), Rust applies decay + `log2(1+count)` + final ranking
|
||||
- Match both `new_path` and `old_path` in all signal queries (rename awareness)
|
||||
- Extend rename awareness to `build_path_query()` probes and `suffix_probe()` (not just scoring)
|
||||
- Split reviewer signal into participated vs assigned-only
|
||||
@@ -106,10 +107,10 @@ pub struct ScoringConfig {
|
||||
```
|
||||
|
||||
**Config validation**: Add a `validate_scoring()` call in `Config::load_from_path()` after deserialization:
|
||||
- All `*_half_life_days` must be > 0 (prevents division by zero in decay function)
|
||||
- All `*_half_life_days` must be > 0 and <= 3650 (prevents division by zero in decay function; rejects absurd 10+ year half-lives that would effectively disable decay)
|
||||
- All `*_weight` / `*_bonus` must be >= 0 (negative weights produce nonsensical scores)
|
||||
- `closed_mr_multiplier` must be in `(0.0, 1.0]` (0 would discard closed MRs entirely; >1 would over-weight them)
|
||||
- `reviewer_min_note_chars` must be >= 0 (0 disables the filter; typical useful values: 10-50)
|
||||
- `closed_mr_multiplier` must be finite (not NaN/Inf) and in `(0.0, 1.0]` (0 would discard closed MRs entirely; >1 would over-weight them; NaN/Inf would propagate through all scores)
|
||||
- `reviewer_min_note_chars` must be >= 0 and <= 4096 (0 disables the filter; 4096 is a sane upper bound — no real review comment needs to be longer to qualify; typical useful values: 10-50)
|
||||
- `excluded_usernames` entries must be non-empty strings (no blank entries)
|
||||
- Return `LoreError::ConfigInvalid` with a clear message on failure
|
||||
|
||||
@@ -126,9 +127,9 @@ fn half_life_decay(elapsed_ms: i64, half_life_days: u32) -> f64 {
|
||||
|
||||
### 3. SQL Restructure (who.rs)
|
||||
|
||||
The SQL uses **CTE-based dual-path matching** and **hybrid aggregation**. Rather than repeating `OR old_path` in every signal subquery, two foundational CTEs (`matched_notes`, `matched_file_changes`) centralize path matching. A third CTE (`reviewer_participation`) precomputes which reviewers actually left DiffNotes, avoiding correlated `EXISTS`/`NOT EXISTS` subqueries.
|
||||
The SQL uses **CTE-based dual-path matching**, a **centralized `mr_activity` CTE**, and **hybrid aggregation**. Rather than repeating `OR old_path` in every signal subquery, two foundational CTEs (`matched_notes`, `matched_file_changes`) centralize path matching. A `mr_activity` CTE centralizes the state-aware timestamp and state multiplier in one place, eliminating repetition of the CASE expression across signals 3, 4a, 4b. A fourth CTE (`reviewer_participation`) precomputes which reviewers actually left DiffNotes, avoiding correlated `EXISTS`/`NOT EXISTS` subqueries.
|
||||
|
||||
MR-level signals return one row per (username, signal, mr_id) with a timestamp; note signals return one row per (username, mr_id) with `note_count` and `max_ts`. This keeps row counts bounded (dozens to low hundreds per path) while giving Rust the data it needs for decay and `log2(1+count)`.
|
||||
MR-level signals return one row per (username, signal, mr_id) with a timestamp and state multiplier; note signals return one row per (username, mr_id) with `note_count` and `max_ts`. This keeps row counts bounded (dozens to low hundreds per path) while giving Rust the data it needs for decay and `log2(1+count)`.
|
||||
|
||||
```sql
|
||||
WITH matched_notes_raw AS (
|
||||
@@ -177,6 +178,24 @@ matched_file_changes AS (
|
||||
SELECT DISTINCT merge_request_id, project_id
|
||||
FROM matched_file_changes_raw
|
||||
),
|
||||
mr_activity AS (
|
||||
-- Centralized state-aware timestamps and state multiplier.
|
||||
-- Defined once, referenced by all file-change-based signals (3, 4a, 4b).
|
||||
-- Scoped to MRs matched by file changes to avoid materializing the full MR table.
|
||||
SELECT DISTINCT
|
||||
m.id AS mr_id,
|
||||
m.author_username,
|
||||
m.state,
|
||||
CASE
|
||||
WHEN m.state = 'merged' THEN COALESCE(m.merged_at, m.created_at)
|
||||
WHEN m.state = 'closed' THEN COALESCE(m.closed_at, m.created_at)
|
||||
ELSE COALESCE(m.updated_at, m.created_at)
|
||||
END AS activity_ts,
|
||||
CASE WHEN m.state = 'closed' THEN ?5 ELSE 1.0 END AS state_mult
|
||||
FROM merge_requests m
|
||||
JOIN matched_file_changes mfc ON mfc.merge_request_id = m.id
|
||||
WHERE m.state IN ('opened','merged','closed')
|
||||
),
|
||||
reviewer_participation AS (
|
||||
-- Precompute which (mr_id, username) pairs have substantive DiffNote participation.
|
||||
-- Materialized once, then joined against mr_reviewers to classify.
|
||||
@@ -185,17 +204,20 @@ reviewer_participation AS (
|
||||
-- reviewer from 3-point to 10-point weight, defeating the purpose of the split.
|
||||
-- Note: mn.id refers back to notes.id, so we join notes to access the body column
|
||||
-- (not carried in matched_notes to avoid bloating that CTE with body text).
|
||||
-- ?6 is the configured reviewer_min_note_chars value (default 20).
|
||||
SELECT DISTINCT d.merge_request_id AS mr_id, mn.author_username AS username
|
||||
FROM matched_notes mn
|
||||
JOIN discussions d ON mn.discussion_id = d.id
|
||||
JOIN notes n_body ON mn.id = n_body.id
|
||||
WHERE d.merge_request_id IS NOT NULL
|
||||
AND LENGTH(TRIM(COALESCE(n_body.body, ''))) >= {reviewer_min_note_chars}
|
||||
AND LENGTH(TRIM(COALESCE(n_body.body, ''))) >= ?6
|
||||
),
|
||||
raw AS (
|
||||
-- Signal 1: DiffNote reviewer (individual notes for note_cnt)
|
||||
-- Computes state_mult inline (not via mr_activity) because this joins through discussions, not file changes.
|
||||
SELECT mn.author_username AS username, 'diffnote_reviewer' AS signal,
|
||||
m.id AS mr_id, mn.id AS note_id, mn.created_at AS seen_at, m.state AS mr_state
|
||||
m.id AS mr_id, mn.id AS note_id, mn.created_at AS seen_at,
|
||||
CASE WHEN m.state = 'closed' THEN ?5 ELSE 1.0 END AS state_mult
|
||||
FROM matched_notes mn
|
||||
JOIN discussions d ON mn.discussion_id = d.id
|
||||
JOIN merge_requests m ON d.merge_request_id = m.id
|
||||
@@ -205,8 +227,10 @@ raw AS (
|
||||
UNION ALL
|
||||
|
||||
-- Signal 2: DiffNote MR author
|
||||
-- Computes state_mult inline (same reason as signal 1).
|
||||
SELECT m.author_username AS username, 'diffnote_author' AS signal,
|
||||
m.id AS mr_id, NULL AS note_id, MAX(mn.created_at) AS seen_at, m.state AS mr_state
|
||||
m.id AS mr_id, NULL AS note_id, MAX(mn.created_at) AS seen_at,
|
||||
CASE WHEN m.state = 'closed' THEN ?5 ELSE 1.0 END AS state_mult
|
||||
FROM merge_requests m
|
||||
JOIN discussions d ON d.merge_request_id = m.id
|
||||
JOIN matched_notes mn ON mn.discussion_id = d.id
|
||||
@@ -216,65 +240,59 @@ raw AS (
|
||||
|
||||
UNION ALL
|
||||
|
||||
-- Signal 3: MR author via file changes (state-aware timestamp)
|
||||
SELECT m.author_username AS username, 'file_author' AS signal,
|
||||
m.id AS mr_id, NULL AS note_id,
|
||||
{state_aware_ts} AS seen_at, m.state AS mr_state
|
||||
FROM matched_file_changes mfc
|
||||
JOIN merge_requests m ON mfc.merge_request_id = m.id
|
||||
WHERE m.author_username IS NOT NULL
|
||||
AND m.state IN ('opened','merged','closed')
|
||||
AND {state_aware_ts} >= ?2
|
||||
AND {state_aware_ts} < ?4
|
||||
-- Signal 3: MR author via file changes (uses mr_activity CTE for timestamp + state_mult)
|
||||
SELECT a.author_username AS username, 'file_author' AS signal,
|
||||
a.mr_id, NULL AS note_id,
|
||||
a.activity_ts AS seen_at, a.state_mult
|
||||
FROM mr_activity a
|
||||
WHERE a.author_username IS NOT NULL
|
||||
AND a.activity_ts >= ?2
|
||||
AND a.activity_ts < ?4
|
||||
|
||||
UNION ALL
|
||||
|
||||
-- Signal 4a: Reviewer participated (in mr_reviewers AND left DiffNotes on path)
|
||||
SELECT r.username AS username, 'file_reviewer_participated' AS signal,
|
||||
m.id AS mr_id, NULL AS note_id,
|
||||
{state_aware_ts} AS seen_at, m.state AS mr_state
|
||||
FROM matched_file_changes mfc
|
||||
JOIN merge_requests m ON mfc.merge_request_id = m.id
|
||||
JOIN mr_reviewers r ON r.merge_request_id = m.id
|
||||
JOIN reviewer_participation rp ON rp.mr_id = m.id AND rp.username = r.username
|
||||
a.mr_id, NULL AS note_id,
|
||||
a.activity_ts AS seen_at, a.state_mult
|
||||
FROM mr_activity a
|
||||
JOIN mr_reviewers r ON r.merge_request_id = a.mr_id
|
||||
JOIN reviewer_participation rp ON rp.mr_id = a.mr_id AND rp.username = r.username
|
||||
WHERE r.username IS NOT NULL
|
||||
AND (m.author_username IS NULL OR r.username != m.author_username)
|
||||
AND m.state IN ('opened','merged','closed')
|
||||
AND {state_aware_ts} >= ?2
|
||||
AND {state_aware_ts} < ?4
|
||||
AND (a.author_username IS NULL OR r.username != a.author_username)
|
||||
AND a.activity_ts >= ?2
|
||||
AND a.activity_ts < ?4
|
||||
|
||||
UNION ALL
|
||||
|
||||
-- Signal 4b: Reviewer assigned-only (in mr_reviewers, NO DiffNotes on path)
|
||||
SELECT r.username AS username, 'file_reviewer_assigned' AS signal,
|
||||
m.id AS mr_id, NULL AS note_id,
|
||||
{state_aware_ts} AS seen_at, m.state AS mr_state
|
||||
FROM matched_file_changes mfc
|
||||
JOIN merge_requests m ON mfc.merge_request_id = m.id
|
||||
JOIN mr_reviewers r ON r.merge_request_id = m.id
|
||||
LEFT JOIN reviewer_participation rp ON rp.mr_id = m.id AND rp.username = r.username
|
||||
a.mr_id, NULL AS note_id,
|
||||
a.activity_ts AS seen_at, a.state_mult
|
||||
FROM mr_activity a
|
||||
JOIN mr_reviewers r ON r.merge_request_id = a.mr_id
|
||||
LEFT JOIN reviewer_participation rp ON rp.mr_id = a.mr_id AND rp.username = r.username
|
||||
WHERE rp.username IS NULL -- NOT in participation set
|
||||
AND r.username IS NOT NULL
|
||||
AND (m.author_username IS NULL OR r.username != m.author_username)
|
||||
AND m.state IN ('opened','merged','closed')
|
||||
AND {state_aware_ts} >= ?2
|
||||
AND {state_aware_ts} < ?4
|
||||
AND (a.author_username IS NULL OR r.username != a.author_username)
|
||||
AND a.activity_ts >= ?2
|
||||
AND a.activity_ts < ?4
|
||||
),
|
||||
aggregated AS (
|
||||
-- MR-level signals: 1 row per (username, signal_class, mr_id) with MAX(ts)
|
||||
SELECT username, signal, mr_id, 1 AS qty, MAX(seen_at) AS ts, mr_state
|
||||
SELECT username, signal, mr_id, 1 AS qty, MAX(seen_at) AS ts, MAX(state_mult) AS state_mult
|
||||
FROM raw WHERE signal != 'diffnote_reviewer'
|
||||
GROUP BY username, signal, mr_id
|
||||
UNION ALL
|
||||
-- Note signals: 1 row per (username, mr_id) with note_count and max_ts
|
||||
SELECT username, 'note_group' AS signal, mr_id, COUNT(*) AS qty, MAX(seen_at) AS ts, mr_state
|
||||
SELECT username, 'note_group' AS signal, mr_id, COUNT(*) AS qty, MAX(seen_at) AS ts, MAX(state_mult) AS state_mult
|
||||
FROM raw WHERE signal = 'diffnote_reviewer' AND note_id IS NOT NULL
|
||||
GROUP BY username, mr_id
|
||||
)
|
||||
SELECT username, signal, mr_id, qty, ts, mr_state FROM aggregated WHERE username IS NOT NULL
|
||||
SELECT username, signal, mr_id, qty, ts, state_mult FROM aggregated WHERE username IS NOT NULL
|
||||
```
|
||||
|
||||
Where `{state_aware_ts}` is the state-aware timestamp expression (defined in the next section), `{path_op}` is either `= ?1` or `LIKE ?1 ESCAPE '\\'` depending on the path query type, `?4` is the `as_of_ms` exclusive upper bound (defaults to `now_ms` when `--as-of` is not specified), and `{reviewer_min_note_chars}` is the configured `reviewer_min_note_chars` value (default 20, inlined as a literal in the SQL string). The `>= ?2 AND < ?4` pattern (half-open interval) ensures that when `--as-of` is set to a past date, events at or after that date are excluded — without this, "future" events would leak in with full weight, breaking reproducibility. The exclusive upper bound avoids edge-case ambiguity when events have timestamps exactly equal to the as-of value.
|
||||
Where `{path_op}` is either `= ?1` or `LIKE ?1 ESCAPE '\\'` depending on the path query type, `?2` is `since_ms`, `?3` is the optional project_id, `?4` is the `as_of_ms` exclusive upper bound (defaults to `now_ms` when `--as-of` is not specified), `?5` is the `closed_mr_multiplier` (default 0.5, bound as a parameter), and `?6` is the configured `reviewer_min_note_chars` value (default 20, bound as a parameter). The `>= ?2 AND < ?4` pattern (half-open interval) ensures that when `--as-of` is set to a past date, events at or after that date are excluded — without this, "future" events would leak in with full weight, breaking reproducibility. The exclusive upper bound avoids edge-case ambiguity when events have timestamps exactly equal to the as-of value.
|
||||
|
||||
**Rationale for CTE-based dual-path matching**: The previous approach (repeating `OR old_path` in every signal subquery) duplicated the path matching logic 5 times. Factoring it into foundational CTEs (`matched_notes_raw` → `matched_notes`, `matched_file_changes_raw` → `matched_file_changes`) means path matching is defined once, each index branch is explicit, and adding future path resolution logic (e.g., alias chains) only requires changes in one place. The UNION ALL + dedup pattern ensures SQLite uses the optimal index for each path column independently.
|
||||
|
||||
@@ -308,7 +326,21 @@ Both columns already exist in the schema (`notes.position_old_path` from migrati
|
||||
- **Signal 4a** (`file_reviewer_participated`): User is in `mr_reviewers` AND appears in the `reviewer_participation` CTE (left DiffNotes on the path for that MR). Gets `reviewer_weight` (10) and `reviewer_half_life_days` (90).
|
||||
- **Signal 4b** (`file_reviewer_assigned`): User is in `mr_reviewers` but NOT in the `reviewer_participation` CTE. Gets `reviewer_assignment_weight` (3) and `reviewer_assignment_half_life_days` (45).
|
||||
|
||||
### 3a. Path Resolution Probes (who.rs)
|
||||
**Rationale for `mr_activity` CTE**: The previous approach repeated the state-aware CASE expression and `m.state` column in signals 3, 4a, and 4b, with the `closed_mr_multiplier` applied later in Rust by string-matching on `mr_state`. This split was brittle — the CASE expression could drift between signal branches, and per-row state-string handling in Rust was unnecessary indirection. The `mr_activity` CTE defines the timestamp and multiplier once, scoped to matched MRs only (via JOIN with `matched_file_changes`) to avoid materializing the full MR table. Signals 3, 4a, 4b now reference `a.activity_ts` and `a.state_mult` directly. Signals 1 and 2 (DiffNote-based) still compute `state_mult` inline because they join through `discussions`, not `matched_file_changes`, and adding them to `mr_activity` would require a second join path that doesn't simplify anything.
|
||||
|
||||
**Rationale for parameterized `reviewer_min_note_chars` and `closed_mr_multiplier`**: Previous iterations inlined `reviewer_min_note_chars` as a literal in the SQL string and kept `closed_mr_multiplier` in Rust only. Binding both as SQL parameters (`?5` for `closed_mr_multiplier`, `?6` for `reviewer_min_note_chars`) eliminates statement-cache churn (the SQL text is identical regardless of config values), avoids SQL-text variability that complicates EXPLAIN QUERY PLAN analysis, and centralizes the multiplier application in SQL for file-change signals. The DiffNote signals (1, 2) still compute `state_mult` inline because they don't go through `mr_activity`.
|
||||
|
||||
### 3a. Path Canonicalization and Resolution Probes (who.rs)
|
||||
|
||||
**Path canonicalization**: Before any path resolution or scoring, normalize the user's input path via `normalize_query_path()`:
|
||||
- Strip leading `./` (e.g., `./src/foo.rs` → `src/foo.rs`)
|
||||
- Collapse repeated `/` (e.g., `src//foo.rs` → `src/foo.rs`)
|
||||
- Trim leading/trailing whitespace
|
||||
- Preserve trailing `/` only when present — it signals explicit prefix intent
|
||||
|
||||
This is applied once at the top of `run_who()` before `build_path_query()`. The robot JSON `resolved_input` includes both `path_input_original` (raw user input) and `path_input_normalized` (after canonicalization) for debugging transparency. The normalization is purely syntactic — no filesystem lookups, no canonicalization against the database.
|
||||
|
||||
**Path resolution probes**: Rename awareness must extend beyond scoring queries to the path resolution layer. Currently `build_path_query()` (line 457) and `suffix_probe()` (line 584) only check `position_new_path` and `new_path`. If a user queries an old path name, these probes return "not found" and the scoring query never runs.
|
||||
|
||||
Rename awareness must extend beyond scoring queries to the path resolution layer. Currently `build_path_query()` (line 457) and `suffix_probe()` (line 584) only check `position_new_path` and `new_path`. If a user queries an old path name, these probes return "not found" and the scoring query never runs.
|
||||
|
||||
@@ -337,39 +369,29 @@ WHERE old_path IS NOT NULL
|
||||
|
||||
This ensures that querying by an old filename (e.g., `login.rs` after it was renamed to `auth.rs`) still resolves to a usable path for scoring. The UNION deduplicates so the same path appearing in both old and new columns doesn't cause false ambiguity.
|
||||
|
||||
**State-aware timestamps for file-change signals (signals 3, 4a, 4b)**: Replace `m.updated_at` with a state-aware expression:
|
||||
|
||||
```sql
|
||||
CASE
|
||||
WHEN m.state = 'merged' THEN COALESCE(m.merged_at, m.created_at)
|
||||
WHEN m.state = 'closed' THEN COALESCE(m.closed_at, m.created_at)
|
||||
ELSE COALESCE(m.updated_at, m.created_at) -- opened / other
|
||||
END AS activity_ts
|
||||
```
|
||||
**State-aware timestamps for file-change signals (signals 3, 4a, 4b)**: Centralized in the `mr_activity` CTE (see section 3). The CASE expression uses `merged_at` for merged MRs, `closed_at` for closed MRs, and `updated_at` for open MRs, with `created_at` as fallback when the preferred timestamp is NULL.
|
||||
|
||||
**Rationale**: `updated_at` is noisy for merged MRs — it changes on label edits, title changes, rebases, and metadata touches, creating false recency. `merged_at` is the best indicator of when code expertise was formed (the moment the code entered the branch). But for **open MRs**, `updated_at` is actually the right signal because it reflects ongoing active work. `closed_at` anchors closed-without-merge MRs to their closure time (these represent review effort even if the code was abandoned). Each state gets the timestamp that best represents when expertise was last exercised.
|
||||
|
||||
### 4. Rust-Side Aggregation (who.rs)
|
||||
|
||||
For each username, accumulate into a struct with:
|
||||
- **Author MRs**: `HashMap<i64, (i64, String)>` (mr_id -> (max timestamp, mr_state)) from `diffnote_author` + `file_author` signals
|
||||
- **Reviewer Participated MRs**: `HashMap<i64, (i64, String)>` from `diffnote_reviewer` + `file_reviewer_participated` signals
|
||||
- **Reviewer Assigned-Only MRs**: `HashMap<i64, (i64, String)>` from `file_reviewer_assigned` signals (excluding any MR already in participated set)
|
||||
- **Notes per MR**: `HashMap<i64, (u32, i64, String)>` (mr_id -> (count, max_ts, mr_state)) from `note_group` rows in the aggregated query (already grouped per user+MR with note_count in `qty`). Used for `log2(1 + count)` diminishing returns.
|
||||
- **Author MRs**: `HashMap<i64, (i64, f64)>` (mr_id -> (max timestamp, state_mult)) from `diffnote_author` + `file_author` signals
|
||||
- **Reviewer Participated MRs**: `HashMap<i64, (i64, f64)>` from `diffnote_reviewer` + `file_reviewer_participated` signals
|
||||
- **Reviewer Assigned-Only MRs**: `HashMap<i64, (i64, f64)>` from `file_reviewer_assigned` signals (excluding any MR already in participated set)
|
||||
- **Notes per MR**: `HashMap<i64, (u32, i64, f64)>` (mr_id -> (count, max_ts, state_mult)) from `note_group` rows in the aggregated query (already grouped per user+MR with note_count in `qty`). Used for `log2(1 + count)` diminishing returns.
|
||||
- **Last seen**: max of all timestamps
|
||||
- **Components** (when `--explain-score`): Track per-component f64 subtotals for `author`, `reviewer_participated`, `reviewer_assigned`, `notes`
|
||||
|
||||
The `mr_state` field from each SQL row is stored alongside the timestamp so the Rust-side can apply `closed_mr_multiplier` when `mr_state == "closed"`.
|
||||
The `state_mult` field from each SQL row (already computed in SQL as 1.0 for merged/open or `closed_mr_multiplier` for closed) is stored alongside the timestamp — no string-matching on MR state needed in Rust.
|
||||
|
||||
Compute score as `f64` with **deterministic contribution ordering**: within each signal type, sort contributions by `(mr_id ASC)` before summing. This eliminates platform-dependent HashMap iteration order as a source of f64 rounding variance near ties, ensuring CI reproducibility without the complexity of compensated summation (Neumaier/Kahan). Each MR-level contribution is multiplied by `closed_mr_multiplier` (default 0.5) when the MR's state is `"closed"`:
|
||||
Compute score as `f64` with **deterministic contribution ordering**: within each signal type, sort contributions by `(mr_id ASC)` before summing. This eliminates platform-dependent HashMap iteration order as a source of f64 rounding variance near ties, ensuring CI reproducibility without the complexity of compensated summation (Neumaier/Kahan). Each MR-level contribution is multiplied by its `state_mult` (already computed in SQL):
|
||||
```
|
||||
state_mult(mr) = if mr.state == "closed" { closed_mr_multiplier } else { 1.0 }
|
||||
|
||||
raw_score =
|
||||
sum(author_weight * state_mult(mr) * decay(now - ts, author_hl) for (mr, ts) in author_mrs)
|
||||
+ sum(reviewer_weight * state_mult(mr) * decay(now - ts, reviewer_hl) for (mr, ts) in reviewer_participated)
|
||||
+ sum(reviewer_assignment_weight * state_mult(mr) * decay(now - ts, reviewer_assignment_hl) for (mr, ts) in reviewer_assigned)
|
||||
+ sum(note_bonus * state_mult(mr) * log2(1 + count) * decay(now - ts, note_hl) for (mr, count, ts) in notes_per_mr)
|
||||
sum(author_weight * state_mult * decay(now - ts, author_hl) for (mr, ts, state_mult) in author_mrs)
|
||||
+ sum(reviewer_weight * state_mult * decay(now - ts, reviewer_hl) for (mr, ts, state_mult) in reviewer_participated)
|
||||
+ sum(reviewer_assignment_weight * state_mult * decay(now - ts, reviewer_assignment_hl) for (mr, ts, state_mult) in reviewer_assigned)
|
||||
+ sum(note_bonus * state_mult * log2(1 + count) * decay(now - ts, note_hl) for (mr, count, ts, state_mult) in notes_per_mr)
|
||||
```
|
||||
|
||||
**Why include closed MRs?** A closed-without-merge MR still represents review effort and code familiarity — the reviewer read the diff, left comments, and engaged with the code even though it was ultimately abandoned. Excluding closed MRs entirely (the previous plan's approach) discarded this signal. The `closed_mr_multiplier` (default 0.5) halves the contribution, reflecting that the code never landed but the reviewer's cognitive engagement was real. This also eliminates the dead-code inconsistency where the state-aware CASE expression handled `closed` but the WHERE clause excluded it.
|
||||
@@ -458,9 +480,16 @@ CREATE INDEX IF NOT EXISTS idx_mfc_new_path_project_mr
|
||||
CREATE INDEX IF NOT EXISTS idx_notes_diffnote_discussion_author
|
||||
ON notes(discussion_id, author_username, created_at)
|
||||
WHERE note_type = 'DiffNote' AND is_system = 0;
|
||||
|
||||
-- Support path resolution probes on old_path (build_path_query() and suffix_probe())
|
||||
-- The existing idx_notes_diffnote_path_created covers new_path probes, but old_path probes
|
||||
-- need their own index since probes don't constrain author_username.
|
||||
CREATE INDEX IF NOT EXISTS idx_notes_old_path_project_created
|
||||
ON notes(position_old_path, project_id, created_at)
|
||||
WHERE note_type = 'DiffNote' AND is_system = 0 AND position_old_path IS NOT NULL;
|
||||
```
|
||||
|
||||
**Rationale**: The existing indexes cover `position_new_path` and `new_path` but not their `old_path` counterparts. Without these, the `OR old_path` clauses would force table scans on renamed files. The `reviewer_participation` CTE joins `matched_notes` -> `discussions` -> `merge_requests`, so an index on `(discussion_id, author_username)` speeds up the CTE materialization.
|
||||
**Rationale**: The existing indexes cover `position_new_path` and `new_path` but not their `old_path` counterparts. Without these, the `OR old_path` clauses would force table scans on renamed files. The `reviewer_participation` CTE joins `matched_notes` -> `discussions` -> `merge_requests`, so an index on `(discussion_id, author_username)` speeds up the CTE materialization. The `idx_notes_old_path_project_created` index supports path resolution probes (`build_path_query()` and `suffix_probe()`) which run existence/path-only checks without constraining `author_username` — the scoring-oriented `idx_notes_old_path_author` has `author_username` as the second column, which is suboptimal for these probes.
|
||||
|
||||
**Schema note**: The `notes` table uses `discussion_id` as its FK to `discussions`, which in turn has `merge_request_id`. There is no `noteable_id` column on `notes`. The previous plan revision incorrectly referenced `noteable_id` — this is corrected.
|
||||
|
||||
@@ -526,6 +555,14 @@ Add timestamp-aware variants:
|
||||
|
||||
**`test_null_timestamp_fallback_to_created_at`**: Insert a merged MR with `merged_at = NULL` (edge case: old data before the column was populated). The state-aware timestamp should fall back to `created_at`. Verify the score reflects `created_at`, not 0 or a panic.
|
||||
|
||||
**`test_path_normalization_handles_dot_and_double_slash`**: Call `normalize_query_path("./src//foo.rs")` — should return `"src/foo.rs"`. Call `normalize_query_path(" src/bar.rs ")` — should return `"src/bar.rs"`. Call `normalize_query_path("src/foo.rs")` — should return unchanged (already normalized). Call `normalize_query_path("")` — should return `""` (empty input passes through).
|
||||
|
||||
**`test_path_normalization_preserves_prefix_semantics`**: Call `normalize_query_path("./src/dir/")` — should return `"src/dir/"` (trailing slash preserved for prefix intent). Call `normalize_query_path("src/dir")` — should return `"src/dir"` (no trailing slash = file, not prefix).
|
||||
|
||||
**`test_config_validation_rejects_absurd_half_life`**: `ScoringConfig` with `author_half_life_days = 5000` (>3650 cap) should return `ConfigInvalid` error. Similarly, `reviewer_min_note_chars = 5000` (>4096 cap) should fail.
|
||||
|
||||
**`test_config_validation_rejects_nan_multiplier`**: `ScoringConfig` with `closed_mr_multiplier = f64::NAN` should return `ConfigInvalid` error. Same for `f64::INFINITY`.
|
||||
|
||||
#### Invariant tests (regression safety for ranking systems)
|
||||
|
||||
**`test_score_monotonicity_by_age`**: For any single signal type, an older timestamp must never produce a higher score than a newer timestamp with the same weight and half-life. Generate N random (age, half_life) pairs and assert `decay(older) <= decay(newer)` for all.
|
||||
@@ -554,6 +591,8 @@ The `test_expert_scoring_weights_are_configurable` test needs `..Default::defaul
|
||||
- Confirm that `matched_notes_raw` branch 1 uses the existing new_path index and branch 2 uses `idx_notes_old_path_author` (not a full table scan on either branch)
|
||||
- Confirm that `matched_file_changes_raw` branch 1 uses `idx_mfc_new_path_project_mr` and branch 2 uses `idx_mfc_old_path_project_mr`
|
||||
- Confirm that `reviewer_participation` CTE uses `idx_notes_diffnote_discussion_author`
|
||||
- Confirm that `mr_activity` CTE joins `merge_requests` via primary key from `matched_file_changes`
|
||||
- Confirm that path resolution probes (old_path leg) use `idx_notes_old_path_project_created`
|
||||
- Document the observed plan in a comment near the SQL for future regression reference
|
||||
7. Performance baseline (manual, not CI-gated):
|
||||
- Run `time cargo run --release -- who --path <exact-path>` on the real database for exact, prefix, and suffix modes
|
||||
@@ -571,6 +610,7 @@ The `test_expert_scoring_weights_are_configurable` test needs `..Default::defaul
|
||||
- Spot-check that reviewers who only left "LGTM"-style notes are classified as assigned-only (not participated)
|
||||
- Verify closed MRs contribute at ~50% of equivalent merged MR scores via `--explain-score`
|
||||
- If the project has known bot accounts (e.g., renovate-bot), add them to `excluded_usernames` config and verify they no longer appear in results. Run again with `--include-bots` to confirm they reappear.
|
||||
- Test path normalization: `who --path ./src//foo.rs` and `who --path src/foo.rs` should produce identical results
|
||||
|
||||
## Accepted from External Review
|
||||
|
||||
@@ -614,6 +654,14 @@ Ideas incorporated from ChatGPT review (feedback-1 through feedback-4) that genu
|
||||
- **Performance baseline SLOs**: Added manual performance baseline step to verification — record timings for exact/prefix/suffix modes and flag >2x regressions. Kept lightweight (no CI gating, no synthetic benchmarks) to match the project's current maturity.
|
||||
- **New tests**: `test_as_of_exclusive_upper_bound`, `test_excluded_usernames_filters_bots`, `test_include_bots_flag_disables_filtering`, `test_deterministic_accumulation_order` — cover the newly-accepted features.
|
||||
|
||||
**From feedback-6 (ChatGPT review):**
|
||||
- **Centralized `mr_activity` CTE**: The state-aware timestamp CASE expression and `closed_mr_multiplier` were repeated across signals 3, 4a, 4b with the multiplier applied later in Rust via string-matching on `mr_state`. This was brittle — the CASE could drift between branches and the Rust-side string matching was unnecessary indirection. A single `mr_activity` CTE defines both `activity_ts` and `state_mult` once, scoped to matched MRs only (via JOIN with `matched_file_changes`). Signals 1 and 2 still compute `state_mult` inline because they join through `discussions`, not `matched_file_changes`.
|
||||
- **Parameterized `reviewer_min_note_chars` and `closed_mr_multiplier`**: Previously `reviewer_min_note_chars` was inlined as a literal in the SQL string and `closed_mr_multiplier` was applied only in Rust. Binding both as SQL parameters (`?5` for `closed_mr_multiplier`, `?6` for `reviewer_min_note_chars`) eliminates statement-cache churn, ensures identical SQL text regardless of config values, and simplifies EXPLAIN QUERY PLAN analysis.
|
||||
- **Tightened config validation**: Added upper bounds — `*_half_life_days <= 3650` (10-year safety cap), `reviewer_min_note_chars <= 4096`, and `closed_mr_multiplier` must be finite (not NaN/Inf). These prevent absurd configurations from silently producing nonsensical results.
|
||||
- **Path canonicalization via `normalize_query_path()`**: Inputs like `./src//foo.rs` or whitespace-padded paths could fail path resolution even when the file exists in the database. A simple syntactic normalization (strip `./`, collapse `//`, trim whitespace, preserve trailing `/`) runs before `build_path_query()` to reduce false negatives. No filesystem or database lookups — purely string manipulation.
|
||||
- **Probe-optimized `idx_notes_old_path_project_created` index**: The scoring-oriented `idx_notes_old_path_author` index has `author_username` as its second column, which is suboptimal for path resolution probes that don't constrain author. A dedicated probe index on `(position_old_path, project_id, created_at)` ensures `build_path_query()` and `suffix_probe()` old_path lookups are efficient.
|
||||
- **New tests**: `test_path_normalization_handles_dot_and_double_slash`, `test_path_normalization_preserves_prefix_semantics`, `test_config_validation_rejects_absurd_half_life`, `test_config_validation_rejects_nan_multiplier` — cover the path canonicalization and tightened validation logic.
|
||||
|
||||
## Rejected Ideas (with rationale)
|
||||
|
||||
These suggestions were considered during review but explicitly excluded from this iteration:
|
||||
@@ -635,3 +683,6 @@ These suggestions were considered during review but explicitly excluded from thi
|
||||
- **Full evidence drill-down in `--explain-score`** (feedback-5 #8): Proposes `--explain-score=summary|full` with per-MR evidence rows. Already rejected in feedback-2 #7. Component totals are sufficient for v1 debugging — they answer "which signal type drives this user's score." Per-MR drill-down requires additional SQL queries and significant output format complexity. Deferred unless component breakdowns prove insufficient.
|
||||
- **Neumaier compensated summation** (feedback-5 #7 partial): Accepted the sorting aspect for deterministic ordering, but rejected Neumaier/Kahan compensated summation. At the scale of dozens to low hundreds of contributions per user, the rounding error from naive f64 summation is on the order of 1e-14 — several orders of magnitude below any meaningful score difference. Compensated summation adds code complexity and a maintenance burden for no practical benefit at this scale.
|
||||
- **Automated CI benchmark gate** (feedback-5 #10 partial): Accepted manual performance baselines, but rejected automated CI regression gating with synthetic fixtures (100k/1M/5M notes). Building and maintaining benchmark infrastructure is a significant investment that's premature for a CLI tool with ~3 users. Manual timing checks during development are sufficient until performance becomes a real concern.
|
||||
- **Epsilon-based tie buckets for ranking** (feedback-6 #4) — rejected because the plan already has deterministic contribution ordering by `mr_id` within each signal type, which eliminates HashMap-iteration nondeterminism. Platform-dependent `powf` differences at the scale of dozens to hundreds of contributions per user are sub-epsilon (order of 1e-15). If two users genuinely score within 1e-9 of each other, the existing tiebreak by `(last_seen DESC, username ASC)` is already meaningful and deterministic. Adding a bucketing layer introduces a magic epsilon constant and floor operation for a problem that doesn't manifest in practice.
|
||||
- **`--diagnose-score` aggregated diagnostics flag** (feedback-6 #5) — rejected because this is diagnostic/debugging tooling that adds a new flag, new output format, and new counting logic (matched_notes_raw_count, dedup_count, window exclusions, etc.) across the SQL pipeline. The existing `--explain-score` component breakdown + manual EXPLAIN QUERY PLAN verification already covers the debugging need. The additional SQL instrumentation required (counting rows at each CTE stage) would complicate the query for a feature with unclear demand. A v2 addition if operational debugging becomes a recurring need.
|
||||
- **Multi-path expert scoring (`--path` repeatable)** (feedback-6 #7) — rejected because this is a feature expansion, not a plan improvement for the time-decay model. Multi-path requires a `requested_paths` CTE, modified dedup logic keyed on `(username, signal, mr_id)` across paths, CLI parsing changes for repeatable `--path` and `--path-file`, and new test cases for overlap/prefix/dedup semantics. This is a separate bead/feature that should be designed independently — it's orthogonal to time-decay scoring and can be added later without requiring any changes to the decay model.
|
||||
|
||||
Reference in New Issue
Block a user