From 5fa79e4156a467217caeaadd97e39b10c44811f8 Mon Sep 17 00:00:00 2001 From: Matt OD Date: Thu, 16 Jul 2026 01:22:36 -0700 Subject: [PATCH] =?UTF-8?q?chore:=20resolve=20issue=20#154=20=E2=80=94=20s?= =?UTF-8?q?urface=20narrative=20reviews=20in=20chat=20context?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Chat claimed "no performance data" for employees who had written reviews but no numeric ratings, while Prep Brief cited those same reviews from the same DB. The context module read `performance_ratings` exclusively and had zero references to `performance_reviews`, and prompt.rs gated the entire Performance section on `all_ratings` being non-empty — so a reviews-only employee rendered nothing and the model correctly reported it saw nothing. Retrieval now carries `review_count` + `latest_review_date` on EmployeeContext, populated on both the single-employee and batch paths (the batch path groups per employee, mirroring the existing ratings/eNPS pattern). prompt.rs renders the Performance section when EITHER ratings or reviews exist. Existence is stated, not content: the narrative text is deliberately not loaded (the employee-context section is token-budgeted), and the line says so explicitly so the model does not claim to have read the reviews. Per the issue's minimum bar — the model must never claim "no review history" when one exists. The richer inclusion-shape question (full narratives vs FTS excerpts) is left for the human decision the issue asks for. Verification: 858 Rust tests pass (854 baseline + 4 new), tsc --noEmit clean, no new compiler warnings (45 before, 45 after). The new DB-backed test exercises both SQL paths against the real migrated schema and covers the previously untested single-employee path. Reverting the prompt.rs gate alone makes the regression tests fail, confirming the lock is real. do-not-touch honored: no changes under src-tauri/src/people_map/ or src-tauri/src/recruiting/. Token-budget contract and provider payload shape unchanged (bail-if not triggered). Resolves #154 Co-Authored-By: Claude Opus 4.8 (1M context) --- src-tauri/src/context/prompt.rs | 96 ++++++++++++++++- src-tauri/src/context/retrieval.rs | 159 +++++++++++++++++++++++++++++ 2 files changed, 254 insertions(+), 1 deletion(-) diff --git a/src-tauri/src/context/prompt.rs b/src-tauri/src/context/prompt.rs index ca3507b..0554fb0 100644 --- a/src-tauri/src/context/prompt.rs +++ b/src-tauri/src/context/prompt.rs @@ -416,7 +416,10 @@ fn format_single_employee_with_budget(emp: &EmployeeContext, token_budget: Optio } // Performance info - if !emp.all_ratings.is_empty() { + // #154: reviews and ratings are independent — an employee may have narrative + // reviews and no numeric ratings. Gating this section on ratings alone made + // such employees look like they had no performance history at all. + if !emp.all_ratings.is_empty() || emp.review_count > 0 { lines.push(" Performance:".to_string()); for rating in emp.all_ratings.iter().take(3) { let label = rating_label(rating.overall_rating); @@ -430,6 +433,19 @@ fn format_single_employee_with_budget(emp: &EmployeeContext, token_budget: Optio if let Some(ref trend) = emp.rating_trend { lines.push(format!(" Trend: {}", trend)); } + if emp.review_count > 0 { + let plural = if emp.review_count == 1 { "" } else { "s" }; + let latest = match emp.latest_review_date.as_deref() { + Some(d) => format!(" (latest {})", d), + None => String::new(), + }; + // State existence, not content: the narrative text is not loaded into + // this context, so the model must not claim to have read it. + lines.push(format!( + " - {} written performance review{} on file{}; narrative text not loaded here", + emp.review_count, plural, latest + )); + } } // eNPS info @@ -1122,6 +1138,8 @@ mod tests { latest_rating: Some(4.2), latest_rating_cycle: Some("2024 H2".to_string()), rating_trend: Some("improving".to_string()), + review_count: 0, + latest_review_date: None, all_ratings: vec![ RatingInfo { cycle_name: "2024 H2".to_string(), @@ -1220,6 +1238,8 @@ mod tests { latest_rating_cycle: None, rating_trend: None, all_ratings: vec![], + review_count: 0, + latest_review_date: None, latest_enps: None, latest_enps_date: None, enps_trend: None, @@ -1237,6 +1257,80 @@ mod tests { assert!(!formatted.contains("Career Summary:")); assert!(!formatted.contains("Key Strengths:")); assert!(!formatted.contains("Recent Review Highlights:")); + // #154: no reviews and no ratings => no Performance section at all. + assert!(!formatted.contains("Performance:")); + } + + /// #154 regression lock: an employee with narrative reviews but NO numeric + /// ratings must still surface a performance section. Previously the section + /// was gated on `all_ratings` alone, so chat asserted "no review history" + /// for exactly these employees while Prep Brief cited the same reviews. + fn make_employee_reviews_no_ratings(review_count: usize, latest: Option<&str>) -> EmployeeContext { + EmployeeContext { + id: "emp-3".to_string(), + full_name: "Maya Patel".to_string(), + email: "maya@company.com".to_string(), + department: Some("Design".to_string()), + job_title: Some("Product Designer".to_string()), + hire_date: None, + work_state: None, + status: "Active".to_string(), + manager_name: None, + latest_rating: None, + latest_rating_cycle: None, + rating_trend: None, + all_ratings: vec![], + review_count, + latest_review_date: latest.map(|s| s.to_string()), + latest_enps: None, + latest_enps_date: None, + enps_trend: None, + all_enps: vec![], + career_summary: None, + key_strengths: vec![], + development_areas: vec![], + recent_highlights: vec![], + } + } + + #[test] + fn test_reviews_without_ratings_are_surfaced() { + let emp = make_employee_reviews_no_ratings(2, Some("2026-03-01")); + let formatted = format_single_employee(&emp); + + // The section must exist even though there is not a single rating. + assert!(formatted.contains("Performance:")); + assert!(formatted.contains("2 written performance reviews on file")); + assert!(formatted.contains("2026-03-01")); + // Existence only — the model must not think it has the narrative text. + assert!(formatted.contains("narrative text not loaded here")); + } + + #[test] + fn test_single_review_is_singular_and_survives_null_date() { + // review_date is nullable; an all-NULL set must not print "(latest )". + let emp = make_employee_reviews_no_ratings(1, None); + let formatted = format_single_employee(&emp); + + assert!(formatted.contains("1 written performance review on file")); + assert!(!formatted.contains("reviews on file")); + assert!(!formatted.contains("latest")); + } + + #[test] + fn test_ratings_and_reviews_both_render() { + let mut emp = make_employee_reviews_no_ratings(3, Some("2026-05-10")); + emp.all_ratings = vec![RatingInfo { + cycle_name: "2026 H1".to_string(), + overall_rating: 4.0, + rating_date: Some("2026-06-01".to_string()), + }]; + emp.rating_trend = Some("stable".to_string()); + let formatted = format_single_employee(&emp); + + assert!(formatted.contains("2026 H1")); + assert!(formatted.contains("Trend: stable")); + assert!(formatted.contains("3 written performance reviews on file")); } #[test] diff --git a/src-tauri/src/context/retrieval.rs b/src-tauri/src/context/retrieval.rs index ea536bb..390221f 100644 --- a/src-tauri/src/context/retrieval.rs +++ b/src-tauri/src/context/retrieval.rs @@ -38,6 +38,12 @@ pub struct EmployeeContext { pub rating_trend: Option, // "improving", "stable", "declining" pub all_ratings: Vec, + // #154: Narrative reviews are stored separately from numeric ratings. An + // employee can have reviews with no ratings; without these the context + // renders nothing and the model asserts there is no review history. + pub review_count: usize, + pub latest_review_date: Option, + // eNPS data pub latest_enps: Option, pub latest_enps_date: Option, @@ -138,6 +144,14 @@ struct RatingRow { rating_date: Option, } +/// Internal struct for the #154 narrative-review count/date probe. +/// `review_date` is nullable, so an all-NULL review set yields `None` here. +#[derive(Debug, Clone, FromRow)] +struct ReviewMetaRow { + review_count: i64, + latest_review_date: Option, +} + /// Internal struct for eNPS query result #[derive(Debug, Clone, FromRow)] struct EnpsRow { @@ -318,6 +332,23 @@ pub async fn get_employee_context( .fetch_all(pool) .await?; + // #154: Narrative reviews — count + latest date only. The full text is + // deliberately not loaded here; the employee-context section is token-budgeted + // and this only needs to establish that a review history exists. + let review_meta: Option = sqlx::query_as( + r#" + SELECT COUNT(*) as review_count, MAX(review_date) as latest_review_date + FROM performance_reviews + WHERE employee_id = ? + "# + ) + .bind(employee_id) + .fetch_optional(pool) + .await?; + let (review_count, latest_review_date) = review_meta + .map(|r| (r.review_count.max(0) as usize, r.latest_review_date)) + .unwrap_or((0, None)); + // Get eNPS responses let enps_responses: Vec = sqlx::query_as( "SELECT score, survey_name, survey_date, feedback_text FROM enps_responses WHERE employee_id = ? ORDER BY survey_date DESC" @@ -416,6 +447,8 @@ pub async fn get_employee_context( latest_rating_cycle: ratings.first().map(|r| r.cycle_name.clone()), rating_trend, all_ratings, + review_count, + latest_review_date, latest_enps: enps_responses.first().map(|e| e.score), latest_enps_date: enps_responses.first().map(|e| e.survey_date.clone()), enps_trend, @@ -557,6 +590,34 @@ pub async fn get_employee_contexts( }); } + // 5) Batch: #154 narrative-review count + latest date, grouped per employee. + #[derive(FromRow)] + struct ReviewMetaRowWithEmpId { + employee_id: String, + review_count: i64, + latest_review_date: Option, + } + let reviews_query = format!( + r#"SELECT employee_id, COUNT(*) as review_count, MAX(review_date) as latest_review_date + FROM performance_reviews + WHERE employee_id IN ({}) + GROUP BY employee_id"#, + placeholders + ); + let mut q = sqlx::query_as::<_, ReviewMetaRowWithEmpId>(&reviews_query); + for id in employee_ids { + q = q.bind(id); + } + let review_rows: Vec = q.fetch_all(pool).await?; + let mut reviews_by_emp: std::collections::HashMap)> = + std::collections::HashMap::new(); + for r in review_rows { + reviews_by_emp.insert( + r.employee_id, + (r.review_count.max(0) as usize, r.latest_review_date), + ); + } + // Assemble in input-ID order, dropping IDs not found in basic_by_id. let mut out: Vec = Vec::with_capacity(employee_ids.len()); for id in employee_ids { @@ -572,6 +633,8 @@ pub async fn get_employee_contexts( let ratings = ratings_by_emp.get(id).cloned().unwrap_or_default(); let enps_responses = enps_by_emp.get(id).cloned().unwrap_or_default(); + let (review_count, latest_review_date) = + reviews_by_emp.get(id).cloned().unwrap_or((0, None)); let rating_trend = calculate_trend( &ratings.iter().map(|r| r.overall_rating).collect::>(), @@ -667,6 +730,8 @@ pub async fn get_employee_contexts( latest_rating: ratings.first().map(|r| r.overall_rating), latest_rating_cycle: ratings.first().map(|r| r.cycle_name.clone()), rating_trend, + review_count, + latest_review_date, all_ratings, latest_enps: enps_responses.first().map(|e| e.score), latest_enps_date: enps_responses.first().map(|e| e.survey_date.clone()), @@ -1189,4 +1254,98 @@ mod tests { .expect("empty batch"); assert!(empty_result.is_empty(), "empty input must return empty Vec"); } + + /// Regression test for #154: narrative reviews live in `performance_reviews`, + /// numeric scores in `performance_ratings`. The context module read only the + /// latter, so an employee with reviews-but-no-ratings surfaced zero + /// performance context and chat asserted "no review history" while Prep Brief + /// cited the same reviews. Exercises BOTH the single and batch query paths + /// against the real migrated schema. + #[tokio::test] + async fn reviews_without_ratings_are_counted_in_context() { + use sqlx::sqlite::{SqliteConnectOptions, SqlitePoolOptions}; + use std::time::Duration; + + let options = SqliteConnectOptions::new() + .filename(":memory:") + .create_if_missing(true) + .foreign_keys(true) + .busy_timeout(Duration::from_secs(5)); + let pool = SqlitePoolOptions::new() + .max_connections(1) + .connect_with(options) + .await + .expect("connect :memory: pool"); + crate::db::run_migrations_for_tests(&pool) + .await + .expect("run migrations"); + + sqlx::query( + r#" + INSERT INTO employees (id, email, full_name) VALUES + ('maya', 'maya@x.com', 'Maya Patel'), + ('noone', 'noone@x.com', 'No Reviews') + "#, + ) + .execute(&pool) + .await + .expect("insert employees"); + + // UNIQUE(employee_id, review_cycle_id) — two reviews need two cycles. + sqlx::query( + r#" + INSERT INTO review_cycles (id, name, cycle_type, start_date, end_date) VALUES + ('c1', '2025 Annual', 'annual', '2025-01-01', '2025-12-31'), + ('c2', '2026 H1', 'annual', '2026-01-01', '2026-06-30') + "#, + ) + .execute(&pool) + .await + .expect("insert cycles"); + + // Reviews only — deliberately NO performance_ratings rows for Maya. + sqlx::query( + r#" + INSERT INTO performance_reviews + (id, employee_id, review_cycle_id, manager_comments, review_date) VALUES + ('r1', 'maya', 'c1', 'Strong systems thinker.', '2025-12-01'), + ('r2', 'maya', 'c2', 'Led the redesign.', '2026-03-01') + "#, + ) + .execute(&pool) + .await + .expect("insert reviews"); + + // Single-employee path. + let ctx = get_employee_context(&pool, "maya") + .await + .expect("single-employee context"); + assert!( + ctx.all_ratings.is_empty(), + "fixture must have no ratings — that is the whole point of #154" + ); + assert_eq!(ctx.review_count, 2, "both narrative reviews must be counted"); + assert_eq!( + ctx.latest_review_date.as_deref(), + Some("2026-03-01"), + "latest_review_date must be MAX(review_date)" + ); + + // Batch path must agree with the single path. + let batch = get_employee_contexts(&pool, &["maya".to_string(), "noone".to_string()]) + .await + .expect("batch context"); + assert_eq!(batch[0].review_count, 2); + assert_eq!(batch[0].latest_review_date.as_deref(), Some("2026-03-01")); + + // An employee with no reviews must report zero, not a phantom count. + assert_eq!(batch[1].review_count, 0); + assert_eq!(batch[1].latest_review_date, None); + + let solo = get_employee_context(&pool, "noone") + .await + .expect("single-employee context for reviewless employee"); + assert_eq!(solo.review_count, 0); + assert_eq!(solo.latest_review_date, None); + } }