From 9ddbc93f4e4355040975e45b4ae29a8f11ce7f79 Mon Sep 17 00:00:00 2001 From: isletspace Date: Tue, 8 Sep 2026 14:16:42 +0800 Subject: [PATCH 1/3] feat(history): snapshot the actual LLM (provider/model) per review record (RENG-38) (0.10.2) Every history record now shows which LLM actually produced each expert report, at expert granularity: - CompletionResult carries the provider name; complete_with_fallback attributes a success to the exact fallback-chain config that answered. - ExpertReport/AggregatedReport gain #[serde(default)] Option llm_provider/llm_model, backfilled at all expert/aggregator production points (pipeline, orchestrator, expert runner); pre-0.10.2 JSON still deserializes. - Migration 0002_llm_snapshot.sql: expert_reports.llm_provider/llm_model + reviews.llm_summary (deduped [{provider, model}] JSON, written on terminal completion so the list page never parses reviews.result). Name snapshots, not FKs: llm_providers is rewritten DELETE+INSERT on every config save, so foreign keys would dangle. - API: GET /reviews/{id} experts gain llmProvider/llmModel; GET /reviews items gain llmSummary. History page: per-expert provider/model tag in the detail drawer + a compact LLM list column; records predating the snapshot render as unknown/- instead of erroring. Deferred: lead-overview/verifier/adjudicator passes call the LLM but produce no expert_reports row, so their provider/model is not yet snapshotted. --- CHANGELOG.md | 5 + frontend/src/i18n/locales/en.ts | 5 + frontend/src/i18n/locales/fr.ts | 5 + frontend/src/i18n/locales/ja.ts | 5 + frontend/src/i18n/locales/ko.ts | 5 + frontend/src/i18n/locales/zh-CN.ts | 5 + frontend/src/i18n/locales/zh-TW.ts | 5 + frontend/src/services/reviews.ts | 7 +- frontend/src/types/history.ts | 16 +++ frontend/src/views/ReviewHistory.vue | 51 ++++++++- migrations/0002_llm_snapshot.sql | 15 +++ src/actions/repo_review/mod.rs | 2 + src/cli/handlers/ask.rs | 6 + src/cli/handlers/changelog.rs | 2 + src/cli/handlers/describe.rs | 6 + src/cli/handlers/improve.rs | 6 + src/cli/handlers/output.rs | 2 + src/cli/handlers/review.rs | 2 + src/expert/mod.rs | 13 ++- src/llm/client/mod.rs | 22 +++- src/llm/client/tests.rs | 40 ++++++- src/llm/provider.rs | 7 ++ src/models/finding.rs | 126 +++++++++++++++++++++ src/output/parser.rs | 14 +++ src/output/team_renderer/tests.rs | 12 ++ src/repo/experts/aggregator.rs | 2 + src/repo/experts/llm_experts/tests.rs | 1 + src/server/api/dashboard.rs | 1 + src/server/api/repo.rs | 1 + src/server/api/review/discussion.rs | 2 + src/server/api/review/handlers.rs | 1 + src/server/api/review/task.rs | 11 ++ src/server/api/review/tests.rs | 2 + src/server/api/types.rs | 9 ++ src/server/github.rs | 2 + src/server/gitlab/hooks.rs | 2 + src/server/mod.rs | 14 +++ src/server/task_queue.rs | 13 +++ src/store/mod.rs | 24 +++- src/store/rows.rs | 40 ++++++- src/store/sqlx.rs | 157 +++++++++++++++++++++++++- src/team/lead_consolidator/tests.rs | 2 + src/team/orchestrator/mod.rs | 12 +- src/team/orchestrator/pipeline.rs | 7 +- src/team/orchestrator/tests.rs | 2 + src/team/verifier.rs | 2 + tests/server/llm_configs.rs | 87 ++++++++++++++ 47 files changed, 759 insertions(+), 19 deletions(-) create mode 100644 migrations/0002_llm_snapshot.sql diff --git a/CHANGELOG.md b/CHANGELOG.md index a167f01..f77bb83 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,10 @@ # Changelog +## [0.10.3] - 2026-09-10 + +### Added +- **Review history shows the LLM that actually produced each report (RENG-38)**: `CompletionResult` now carries the hitting provider's name — `complete_with_fallback` attributes a success to the exact fallback-chain config that answered, not the caller's primary — and every expert/aggregator production point snapshots `(provider, model)` onto the report (`ExpertReport.llm_provider/llm_model`, `AggregatedReport.llm_provider/llm_model`, both `Option`, `#[serde(default)]` so pre-0.10.2 JSON still deserializes). Migration `0002_llm_snapshot.sql` adds `expert_reports.llm_provider/llm_model` (denormalized per-expert snapshot — deliberately name snapshots, not FK references: `llm_providers` is rewritten DELETE+INSERT on every config save, so foreign keys would dangle) and `reviews.llm_summary` (deduplicated `[{provider, model}]` JSON, written on terminal completion so the history list never parses `reviews.result`). API: `GET /reviews/{id}` experts gain `llmProvider`/`llmModel`, `GET /reviews` items gain `llmSummary`; the History page shows a per-expert `provider/model` tag in the detail drawer and a compact LLM column in the list. Records predating 0.10.2 carry NULLs and render as "未知"/not shown — never an error. Note: the lead-overview, verifier, and adjudicator passes also call the LLM but produce no expert report row, so their provider/model is not yet snapshotted (deferred). (`src/llm/provider.rs`, `src/llm/client/mod.rs`, `src/models/finding.rs`, `src/team/orchestrator/pipeline.rs`, `src/team/orchestrator/mod.rs`, `src/expert/mod.rs`, `migrations/0002_llm_snapshot.sql`, `src/store/rows.rs`, `src/store/sqlx.rs`, `src/server/task_queue.rs`, `src/server/api/types.rs`, `src/server/api/review/task.rs`, `frontend/src/types/history.ts`, `frontend/src/services/reviews.ts`, `frontend/src/views/ReviewHistory.vue`, `frontend/src/i18n/locales/*` ×6) + ## [0.10.2] - 2026-09-08 ### Fixed diff --git a/frontend/src/i18n/locales/en.ts b/frontend/src/i18n/locales/en.ts index a7b496a..0f79375 100644 --- a/frontend/src/i18n/locales/en.ts +++ b/frontend/src/i18n/locales/en.ts @@ -183,11 +183,16 @@ export default { author: 'Author', status: 'Status', score: 'Score', + llm: 'LLM', duration: 'Duration', created: 'Created', time: 'Time', actions: 'Actions', }, + llm: { + unknown: 'unknown', + expertTooltip: 'LLM that actually produced this report (provider/model)', + }, actions: { rerun: 'Re-run review', viewDetails: 'View details', diff --git a/frontend/src/i18n/locales/fr.ts b/frontend/src/i18n/locales/fr.ts index 18d2875..d88bdcc 100644 --- a/frontend/src/i18n/locales/fr.ts +++ b/frontend/src/i18n/locales/fr.ts @@ -175,11 +175,16 @@ export default { author: 'Auteur', status: 'Statut', score: 'Score', + llm: 'LLM', duration: 'Durée', created: 'Créée', time: 'Heure', actions: 'Actions', }, + llm: { + unknown: 'inconnu', + expertTooltip: 'LLM ayant réellement produit ce rapport (provider/model)', + }, actions: { rerun: 'Relancer la revue', viewDetails: 'Voir les détails', diff --git a/frontend/src/i18n/locales/ja.ts b/frontend/src/i18n/locales/ja.ts index 344c1a3..5892460 100644 --- a/frontend/src/i18n/locales/ja.ts +++ b/frontend/src/i18n/locales/ja.ts @@ -174,11 +174,16 @@ export default { author: '作成者', status: 'ステータス', score: 'スコア', + llm: 'LLM', duration: '所要時間', created: '作成日時', time: '時間', actions: '操作', }, + llm: { + unknown: '不明', + expertTooltip: 'このレポートを実際に生成した LLM（provider/model）', + }, actions: { rerun: 'レビューを再実行', viewDetails: '詳細を表示', diff --git a/frontend/src/i18n/locales/ko.ts b/frontend/src/i18n/locales/ko.ts index d731193..d3c1689 100644 --- a/frontend/src/i18n/locales/ko.ts +++ b/frontend/src/i18n/locales/ko.ts @@ -173,11 +173,16 @@ export default { author: '작성자', status: '상태', score: '점수', + llm: 'LLM', duration: '소요 시간', created: '생성 시간', time: '시간', actions: '작업', }, + llm: { + unknown: '알 수 없음', + expertTooltip: '이 리포트를 실제로 생성한 LLM (provider/model)', + }, actions: { rerun: '리뷰 다시 실행', viewDetails: '상세 보기', diff --git a/frontend/src/i18n/locales/zh-CN.ts b/frontend/src/i18n/locales/zh-CN.ts index 646305b..4682ea4 100644 --- a/frontend/src/i18n/locales/zh-CN.ts +++ b/frontend/src/i18n/locales/zh-CN.ts @@ -171,11 +171,16 @@ export default { author: '作者', status: '状态', score: '得分', + llm: 'LLM', duration: '耗时', created: '创建时间', time: '时间', actions: '操作', }, + llm: { + unknown: '未知', + expertTooltip: '实际生成该报告的 LLM（provider/model）', + }, actions: { rerun: '重新评审', viewDetails: '查看详情', diff --git a/frontend/src/i18n/locales/zh-TW.ts b/frontend/src/i18n/locales/zh-TW.ts index daeff4d..a8da41a 100644 --- a/frontend/src/i18n/locales/zh-TW.ts +++ b/frontend/src/i18n/locales/zh-TW.ts @@ -171,11 +171,16 @@ export default { author: '作者', status: '狀態', score: '得分', + llm: 'LLM', duration: '耗時', created: '建立時間', time: '時間', actions: '操作', }, + llm: { + unknown: '未知', + expertTooltip: '實際產生該報告的 LLM（provider/model）', + }, actions: { rerun: '重新審查', viewDetails: '查看詳情', diff --git a/frontend/src/services/reviews.ts b/frontend/src/services/reviews.ts index aa6a9a1..677c972 100644 --- a/frontend/src/services/reviews.ts +++ b/frontend/src/services/reviews.ts @@ -1,6 +1,6 @@ import { request } from './api'; import { normalizeStatus } from './status'; -import type { ReviewListItem, ReviewDetail, HistoryFilters, RiskLevel, ReviewAssessment } from '../types/history'; +import type { ReviewListItem, ReviewDetail, HistoryFilters, RiskLevel, ReviewAssessment, LlmUsage } from '../types/history'; export interface ReviewsListResponse { items: ReviewListItem[]; @@ -40,6 +40,8 @@ interface RawReviewItem { createdAt?: string; gitlab_mr_url?: string | null; gitlabMrUrl?: string; + /** RENG-38: deduplicated LLM pairs (`reviews.llm_summary` snapshot). */ + llmSummary?: LlmUsage[] | null; /** Embedded full `ReviewOutput` JSON (carries `consolidated.assessment`). */ result?: unknown; } @@ -115,6 +117,9 @@ function normalizeReviewListItem(raw: RawReviewItem): ReviewListItem { createdAt: raw.createdAt ?? raw.created_at ?? '', gitlabMrUrl: raw.gitlabMrUrl ?? raw.gitlab_mr_url ?? undefined, assessment: extractAssessment(raw.result), + // Pass the snapshot through only when it is a non-empty array — anything + // else (null, missing, malformed) degrades to "unknown" at render time. + llmSummary: Array.isArray(raw.llmSummary) && raw.llmSummary.length > 0 ? raw.llmSummary : undefined, }; } diff --git a/frontend/src/types/history.ts b/frontend/src/types/history.ts index 8b6e2ca..a164cfd 100644 --- a/frontend/src/types/history.ts +++ b/frontend/src/types/history.ts @@ -26,6 +26,15 @@ export interface ReviewAuthor { email?: string } +/** + * One LLM `provider/model` pair observed during a review (RENG-38), from the + * backend's `reviews.llm_summary` snapshot (`ReviewOutput::llm_usages`). + */ +export interface LlmUsage { + provider: string + model: string +} + export interface ExpertResult { expertId: string expertName: string @@ -36,6 +45,10 @@ export interface ExpertResult { summary: string // Raw LLM response (`report.raw_llm_response`); debugging aid only. details?: string + // RENG-38: name snapshot of the LLM that actually produced this report + // (the fallback-chain hit). Absent for records predating 0.10.2. + llmProvider?: string | null + llmModel?: string | null } export interface ReviewListItem { @@ -51,6 +64,9 @@ export interface ReviewListItem { createdAt: string gitlabMrUrl?: string assessment?: ReviewAssessment + // RENG-38: deduplicated LLM pairs used by this review (`reviews.llm_summary`). + // Absent for records predating 0.10.2 and non-completed tasks. + llmSummary?: LlmUsage[] | null } export interface ReviewDetail { diff --git a/frontend/src/views/ReviewHistory.vue b/frontend/src/views/ReviewHistory.vue index 0dd812c..91ab6b0 100644 --- a/frontend/src/views/ReviewHistory.vue +++ b/frontend/src/views/ReviewHistory.vue @@ -20,7 +20,7 @@ import { import { ElMessage, ElMessageBox, ElNotification } from 'element-plus' import { useI18n } from 'vue-i18n' import type { ApiError } from '../services/api' -import type { ReviewListItem, HistoryFilters, RiskLevel } from '../types/history' +import type { ReviewListItem, ExpertResult, HistoryFilters, RiskLevel } from '../types/history' import { getReviews } from '../services/reviews' import { useReviews } from '../composables/useReviews' import StatusBadge from '../components/ReviewHistory/StatusBadge.vue' @@ -424,6 +424,21 @@ function riskLabelKey(level: RiskLevel): string { return 'history.riskLevel.' + riskLevelKeys[level] } +/* ─────────────── LLM usage snapshot (RENG-38) ─────────────── */ + +/** Compact `provider/model` form for one expert's LLM tag; null when the + record predates the 0.10.2 snapshot (rendered as "未知"/"Unknown"). */ +function expertLlmLabel(exp: ExpertResult): string | null { + if (!exp.llmProvider && !exp.llmModel) return null + return `${exp.llmProvider ?? t('history.llm.unknown')}/${exp.llmModel ?? t('history.llm.unknown')}` +} + +/** Compact list-cell form of the deduplicated `llmSummary` snapshot. */ +function formatLlmSummary(usages: ReviewListItem['llmSummary']): string { + if (!usages || usages.length === 0) return '-' + return usages.map((u) => `${u.provider}/${u.model}`).join(', ') +} + const hasRawComment = computed( () => !!selectedReview.value?.rawComment?.trim() ) @@ -622,6 +637,14 @@ watch(() => route.query, () => { + + + + + @@ -1167,6 +1195,27 @@ watch(() => route.query, () => { margin-right: 50px; } +/* RENG-38: per-expert LLM snapshot tag (provider/model). Mono so model IDs + stay scannable; ellipsized when a provider ships a very long model name. */ +.llm-tag { + font-family: var(--font-mono, monospace); + max-width: 220px; + overflow: hidden; + text-overflow: ellipsis; +} + +/* RENG-38: history list LLM column cell. */ +.llm-cell { + font-family: var(--font-mono, monospace); + font-size: 12px; + color: var(--text-secondary); + white-space: nowrap; + overflow: hidden; + text-overflow: ellipsis; + display: inline-block; + max-width: 100%; +} + .expert-content { padding: 8px 0; display: flex; diff --git a/migrations/0002_llm_snapshot.sql b/migrations/0002_llm_snapshot.sql new file mode 100644 index 0000000..869f5f4 --- /dev/null +++ b/migrations/0002_llm_snapshot.sql @@ -0,0 +1,15 @@ +-- 0.10.2 (RENG-38)：评审历史的 LLM 使用快照。 +-- 报告侧冗余名称快照（不做外键、不做软删）：llm_providers 是整表 +-- DELETE+INSERT 语义，行 id 每次保存都会重生成，外键引用必然悬空； +-- 历史展示要的是"当时实际用了哪个 provider/model"，存名称快照即可。 +-- 方言约束与 0001 一致：占位符 `?`（由 store 层重写）、JSON 一律 TEXT。 +-- 旧行三列均为 NULL，前端显示「未知」/不显示，不做回填。 + +-- 每条专家报告实际使用的 LLM（命中 fallback 链中第几个 config 就记哪个）。 +ALTER TABLE expert_reports ADD COLUMN llm_provider TEXT; +ALTER TABLE expert_reports ADD COLUMN llm_model TEXT; + +-- 评审级去重后的 provider/model 对列表（JSON 数组 TEXT， +-- 形如 [{"provider":"xiaomi","model":"mimo-v2.5-pro"}]）， +-- 供历史列表页免解析 reviews.result 直接展示。 +ALTER TABLE reviews ADD COLUMN llm_summary TEXT; diff --git a/src/actions/repo_review/mod.rs b/src/actions/repo_review/mod.rs index 0c39955..a12c47f 100644 --- a/src/actions/repo_review/mod.rs +++ b/src/actions/repo_review/mod.rs @@ -462,6 +462,8 @@ pub async fn run_repo_review( raw_llm_response: String::new(), parse_error: None, raw_dump_path: None, + llm_provider: None, + llm_model: None, }]; let dropped = crate::team::verifier::verify_findings(&mut reports, &[], local_path, llm_configs, max_file_bytes) diff --git a/src/cli/handlers/ask.rs b/src/cli/handlers/ask.rs index 9e24649..d2ae50f 100644 --- a/src/cli/handlers/ask.rs +++ b/src/cli/handlers/ask.rs @@ -51,6 +51,8 @@ pub async fn run_ask( raw_llm_response: String::new(), parse_error: None, raw_dump_path: None, + llm_model: None, + llm_provider: None, }], aggregated: None, dropped_findings: vec![], @@ -103,6 +105,8 @@ async fn run_ask_with_diff( raw_llm_response: String::new(), parse_error: None, raw_dump_path: None, + llm_model: None, + llm_provider: None, }], aggregated: None, dropped_findings: vec![], @@ -176,6 +180,8 @@ pub async fn run_ask_local_repo( raw_llm_response: String::new(), parse_error: None, raw_dump_path: None, + llm_model: None, + llm_provider: None, }], aggregated: None, dropped_findings: vec![], diff --git a/src/cli/handlers/changelog.rs b/src/cli/handlers/changelog.rs index 7a7ee93..8d01e27 100644 --- a/src/cli/handlers/changelog.rs +++ b/src/cli/handlers/changelog.rs @@ -59,6 +59,8 @@ pub async fn run_update_changelog( raw_llm_response: String::new(), parse_error: None, raw_dump_path: None, + llm_model: None, + llm_provider: None, }], aggregated: None, dropped_findings: vec![], diff --git a/src/cli/handlers/describe.rs b/src/cli/handlers/describe.rs index f1ef5e1..0b52c84 100644 --- a/src/cli/handlers/describe.rs +++ b/src/cli/handlers/describe.rs @@ -54,6 +54,8 @@ pub async fn run_describe( raw_llm_response: String::new(), parse_error: None, raw_dump_path: None, + llm_model: None, + llm_provider: None, }], aggregated: None, dropped_findings: vec![], @@ -114,6 +116,8 @@ pub async fn run_describe_local_diff( raw_llm_response: String::new(), parse_error: None, raw_dump_path: None, + llm_model: None, + llm_provider: None, }], aggregated: None, dropped_findings: vec![], @@ -177,6 +181,8 @@ pub async fn run_describe_local_repo( raw_llm_response: String::new(), parse_error: None, raw_dump_path: None, + llm_model: None, + llm_provider: None, }], aggregated: None, dropped_findings: vec![], diff --git a/src/cli/handlers/improve.rs b/src/cli/handlers/improve.rs index 79ab6f3..d833f76 100644 --- a/src/cli/handlers/improve.rs +++ b/src/cli/handlers/improve.rs @@ -52,6 +52,8 @@ pub async fn run_improve( raw_llm_response: String::new(), parse_error: None, raw_dump_path: None, + llm_model: None, + llm_provider: None, }], aggregated: None, dropped_findings: vec![], @@ -110,6 +112,8 @@ pub async fn run_improve_local_diff( raw_llm_response: String::new(), parse_error: None, raw_dump_path: None, + llm_model: None, + llm_provider: None, }], aggregated: None, dropped_findings: vec![], @@ -171,6 +175,8 @@ pub async fn run_improve_local_repo( raw_llm_response: String::new(), parse_error: None, raw_dump_path: None, + llm_model: None, + llm_provider: None, }], aggregated: None, dropped_findings: vec![], diff --git a/src/cli/handlers/output.rs b/src/cli/handlers/output.rs index 053e749..93ca02a 100644 --- a/src/cli/handlers/output.rs +++ b/src/cli/handlers/output.rs @@ -237,6 +237,8 @@ mod tests { raw_llm_response: String::new(), parse_error: None, raw_dump_path: None, + llm_model: None, + llm_provider: None, }], aggregated: None, dropped_findings: vec![], diff --git a/src/cli/handlers/review.rs b/src/cli/handlers/review.rs index e6b69f3..20eaed5 100644 --- a/src/cli/handlers/review.rs +++ b/src/cli/handlers/review.rs @@ -394,6 +394,8 @@ pub async fn run_local_path( raw_llm_response: String::new(), parse_error: None, raw_dump_path: None, + llm_model: None, + llm_provider: None, }); } diff --git a/src/expert/mod.rs b/src/expert/mod.rs index 8782684..5b41b72 100644 --- a/src/expert/mod.rs +++ b/src/expert/mod.rs @@ -41,7 +41,11 @@ pub async fn run_single_expert( let config = crate::llm::select_llm_config(expert, llm_configs); let result = llm_client.complete_with_fallback(&config, &system, &user).await?; - Ok(parser::parse_llm_response(&expert.name, &result.content)) + let mut report = parser::parse_llm_response(&expert.name, &result.content); + // RENG-38: snapshot the LLM that actually produced this report. + report.llm_provider = Some(result.provider.clone()); + report.llm_model = Some(result.model.clone()); + Ok(report) } /// Execute the aggregator expert to merge multiple expert reports. @@ -65,5 +69,10 @@ pub async fn run_aggregator_expert( let config = crate::llm::select_llm_config(aggregator, llm_configs); let result = llm_client.complete_with_fallback(&config, &system, &user).await?; - parser::parse_aggregator_response(&result.content) + parser::parse_aggregator_response(&result.content).map(|mut agg| { + // RENG-38: snapshot the aggregator's actual LLM. + agg.llm_provider = Some(result.provider.clone()); + agg.llm_model = Some(result.model.clone()); + agg + }) } diff --git a/src/llm/client/mod.rs b/src/llm/client/mod.rs index 5df34ee..eb6c438 100644 --- a/src/llm/client/mod.rs +++ b/src/llm/client/mod.rs @@ -85,6 +85,18 @@ impl LLMClient { std::time::Duration::from_millis(base_ms.min(30_000) + jitter_ms.min(1000)) } + /// Attribute a successful completion to the hitting config's `provider` + /// (RENG-38): the provider instance name usually equals it (registry is + /// keyed by config.provider), but the config entry is the source of truth + /// for history snapshots — e.g. an empty `config.provider` falls back to + /// the registry name instead of recording an empty string. + fn attribute_provider(mut result: CompletionResult, config: &LLMConfig) -> CompletionResult { + if !config.provider.is_empty() { + result.provider = config.provider.clone(); + } + result + } + /// Complete using a specific LLM config (backward-compatible API). pub async fn complete( &self, @@ -105,14 +117,14 @@ impl LLMClient { }; let result = provider.complete(¶ms).await; Self::record_llm_metrics(&config.provider, &config.model, result.is_ok()); - return result; + return result.map(|r| Self::attribute_provider(r, config)); } } // Fallback: use the direct OpenAI-compatible HTTP approach (original behavior) let result = self.complete_direct(config, system_prompt, user_prompt).await; Self::record_llm_metrics(&config.provider, &config.model, result.is_ok()); - result + result.map(|r| Self::attribute_provider(r, config)) } /// Direct HTTP-based completion (backward compat, OpenAI-compatible only). @@ -209,6 +221,7 @@ impl LLMClient { content, total_tokens, model, + provider: config.provider.clone(), }) } @@ -249,7 +262,10 @@ impl LLMClient { attempt_dur, _cf_start.elapsed() ); - return Ok(r); + // RENG-38: attribute the hit to THIS config's provider + // — the fallback chain may have succeeded on a later + // entry than the caller's primary. + return Ok(Self::attribute_provider(r, config)); } Err(e) => { let err_str = e.to_string(); diff --git a/src/llm/client/tests.rs b/src/llm/client/tests.rs index ca62e92..ee4107b 100644 --- a/src/llm/client/tests.rs +++ b/src/llm/client/tests.rs @@ -292,6 +292,7 @@ impl super::super::provider::LLMProvider for MockProvider { content: "success".to_string(), total_tokens: 10, model: "mock".to_string(), + provider: self.name.clone(), }) } } @@ -420,8 +421,45 @@ async fn test_complete_with_fallback_fallback_to_next_provider() { disable_thinking: None, }, ]; - let result = client.complete_with_fallback(&configs, "system", "user").await; assert!(result.is_ok()); assert_eq!(result.unwrap().content, "success"); } + +/// RENG-38: the completion carries the hitting config's provider — when the +/// fallback chain succeeds on the SECOND entry, the result is attributed to +/// that entry, not the primary. This is the attribution the history snapshot +/// (`expert_reports.llm_provider`) is built from. +#[tokio::test] +async fn test_fallback_result_is_attributed_to_the_hitting_provider() { + let client = LLMClient::new(); + let mut registry = ProviderRegistry::new(); + registry.register(Box::new(MockProvider::new("first", 999, "500"))); + registry.register(Box::new(MockProvider::new("second", 0, "unused"))); + let client = client.with_registry(Arc::new(registry)); + + let config = |provider: &str| LLMConfig { + provider: provider.to_string(), + model: format!("{provider}-model"), + api_key: "test".to_string(), + api_base: format!("https://api.{provider}.com/v1"), + max_tokens: 4096, + temperature: 0.3, + disable_thinking: None, + }; + + // Fallback hit: second config wins. + let result = client + .complete_with_fallback(&[config("first"), config("second")], "system", "user") + .await + .unwrap(); + assert_eq!( + result.provider, "second", + "the hitting config's provider must be recorded" + ); + + // Direct hit: attributed to the (only) config. + let result = client.complete(&config("second"), "system", "user").await.unwrap(); + assert_eq!(result.provider, "second"); + assert_eq!(result.model, "mock", "mock provider's reported model is preserved"); +} diff --git a/src/llm/provider.rs b/src/llm/provider.rs index 3c825a2..2d2fa40 100644 --- a/src/llm/provider.rs +++ b/src/llm/provider.rs @@ -22,6 +22,11 @@ pub struct CompletionResult { pub total_tokens: u64, /// Actual model identifier used (may differ from request if provider remapped). pub model: String, + /// Provider name that produced this completion. Filled by the provider + /// itself (`LLMProvider::name`); [`LLMClient`](super::client::LLMClient) + /// overwrites it with the hitting config's `provider` so fallback-chain + /// hits are attributed to the configured entry (RENG-38). + pub provider: String, } /// Parameters for LLM completion requests. @@ -154,6 +159,7 @@ impl LLMProvider for OpenAIProvider { content, total_tokens, model, + provider: self.name().to_string(), }) } } @@ -300,6 +306,7 @@ impl LLMProvider for AnthropicProvider { content, total_tokens, model, + provider: self.name().to_string(), }) } } diff --git a/src/models/finding.rs b/src/models/finding.rs index face7e7..e9ceadf 100644 --- a/src/models/finding.rs +++ b/src/models/finding.rs @@ -26,6 +26,26 @@ pub struct ExpertReport { /// raw exchange was not persisted to disk. #[serde(default)] pub raw_dump_path: Option, + /// Name snapshot of the LLM provider that actually produced this report + /// (the fallback-chain entry that succeeded — RENG-38). `None` for + /// reports produced before 0.10.2 or by non-LLM paths. + #[serde(default)] + pub llm_provider: Option, + /// Model identifier snapshot paired with [`Self::llm_provider`]. + #[serde(default)] + pub llm_model: Option, +} + +/// One LLM `(provider, model)` pair observed during a review (RENG-38). +/// +/// Serialized into `reviews.llm_summary` (TEXT JSON) so the history list can +/// render the compact `provider/model` form without parsing `reviews.result`. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct LlmUsage { + /// Provider name snapshot (e.g. `"xiaomi"`). + pub provider: String, + /// Model identifier snapshot (e.g. `"mimo-v2.5-pro"`). + pub model: String, } /// A single finding / issue identified during a code review. @@ -174,6 +194,13 @@ pub struct AggregatedReport { /// Path of the dumped raw LLM prompt + response with `--verbose`. #[serde(default)] pub raw_dump_path: Option, + /// Name snapshot of the LLM provider that produced this aggregated report + /// (RENG-38). `None` for pre-0.10.2 records. + #[serde(default)] + pub llm_provider: Option, + /// Model identifier snapshot paired with [`Self::llm_provider`]. + #[serde(default)] + pub llm_model: Option, } impl ReviewOutput { @@ -208,6 +235,36 @@ impl ReviewOutput { self.consolidated = Some(consolidated); self } + + /// De-duplicated `(provider, model)` pairs that actually produced this + /// review's reports (RENG-38), in first-seen order: per-expert reports + /// first, then the aggregator. Entries missing either side of the pair + /// (pre-0.10.2 records, non-LLM paths) are skipped. Persisted as + /// `reviews.llm_summary` by the store layer. + pub fn llm_usages(&self) -> Vec { + let mut usages: Vec = Vec::new(); + let mut push = |provider: &Option, model: &Option| { + if let (Some(p), Some(m)) = (provider, model) { + if p.is_empty() || m.is_empty() { + return; + } + let usage = LlmUsage { + provider: p.clone(), + model: m.clone(), + }; + if !usages.contains(&usage) { + usages.push(usage); + } + } + }; + for report in &self.reports { + push(&report.llm_provider, &report.llm_model); + } + if let Some(agg) = &self.aggregated { + push(&agg.llm_provider, &agg.llm_model); + } + usages + } } #[cfg(test)] @@ -345,4 +402,73 @@ mod tests { assert_eq!(back.category, f.category); assert_eq!(back.fingerprint(), f.fingerprint()); } + + fn bare_report(expert: &str, provider: Option<&str>, model: Option<&str>) -> ExpertReport { + ExpertReport { + expert_name: expert.to_string(), + findings: Vec::new(), + markdown: String::new(), + raw_llm_response: String::new(), + parse_error: None, + raw_dump_path: None, + llm_provider: provider.map(str::to_string), + llm_model: model.map(str::to_string), + } + } + + /// RENG-38: `llm_usages` dedups (provider, model) pairs in first-seen + /// order across expert reports + the aggregator, skipping entries that + /// lack either side (pre-0.10.2 records, empty strings). + #[test] + fn llm_usages_dedups_in_first_seen_order() { + let mut output = ReviewOutput::new(vec![ + bare_report("a", Some("xiaomi"), Some("mimo-v2.5-pro")), + bare_report("b", Some("xiaomi"), Some("mimo-v2.5-pro")), + bare_report("c", Some("deepseek"), Some("deepseek-v4")), + bare_report("d", None, None), // no snapshot: skipped + bare_report("e", Some(""), Some("model")), // blank provider: skipped + bare_report("f", Some("xiaomi"), Some("mimo-v2-pro")), + ]); + output.aggregated = Some(AggregatedReport { + findings: Vec::new(), + markdown: String::new(), + raw_llm_response: String::new(), + parse_error: None, + raw_dump_path: None, + llm_provider: Some("deepseek".to_string()), + llm_model: Some("deepseek-v4".to_string()), + }); + + let usages = output.llm_usages(); + assert_eq!( + usages, + vec![ + LlmUsage { + provider: "xiaomi".into(), + model: "mimo-v2.5-pro".into() + }, + LlmUsage { + provider: "deepseek".into(), + model: "deepseek-v4".into() + }, + LlmUsage { + provider: "xiaomi".into(), + model: "mimo-v2-pro".into() + }, + ] + ); + + // No snapshots anywhere → empty list (→ NULL column upstream). + let bare = ReviewOutput::new(vec![bare_report("a", None, None)]); + assert!(bare.llm_usages().is_empty()); + } + + /// Pre-0.10.2 report JSON (no llm_* keys) must still deserialize. + #[test] + fn expert_report_without_llm_fields_deserializes() { + let json = r#"{"expert_name":"a","findings":[],"markdown":"","raw_llm_response":""}"#; + let report: ExpertReport = serde_json::from_str(json).unwrap(); + assert!(report.llm_provider.is_none()); + assert!(report.llm_model.is_none()); + } } diff --git a/src/output/parser.rs b/src/output/parser.rs index 700175c..2c883f8 100644 --- a/src/output/parser.rs +++ b/src/output/parser.rs @@ -68,6 +68,8 @@ fn fallback_report(expert_name: &str, yaml_text: &str) -> ExpertReport { // failure so the report can surface a ⚠️ instead of a false clean bill. parse_error: Some("LLM response could not be parsed into a valid review; treated as no findings".to_string()), raw_dump_path: None, + llm_provider: None, + llm_model: None, } } @@ -90,6 +92,8 @@ pub fn parse_aggregator_response(yaml_text: &str) -> Result { raw_llm_response: yaml_text.to_string(), parse_error: Some("aggregator LLM response could not be parsed; treated as empty".to_string()), raw_dump_path: None, + llm_provider: None, + llm_model: None, }); } @@ -108,6 +112,8 @@ pub fn parse_aggregator_response(yaml_text: &str) -> Result { raw_llm_response: yaml_text.to_string(), parse_error: Some("aggregator LLM response could not be parsed; treated as empty".to_string()), raw_dump_path: None, + llm_provider: None, + llm_model: None, }); } v @@ -132,6 +138,8 @@ pub fn parse_aggregator_response(yaml_text: &str) -> Result { "aggregator LLM response could not be parsed; treated as empty".to_string(), ), raw_dump_path: None, + llm_provider: None, + llm_model: None, }); } } @@ -143,6 +151,8 @@ pub fn parse_aggregator_response(yaml_text: &str) -> Result { raw_llm_response: yaml_text.to_string(), parse_error: Some("aggregator LLM response could not be parsed; treated as empty".to_string()), raw_dump_path: None, + llm_provider: None, + llm_model: None, }); } } @@ -157,6 +167,8 @@ pub fn parse_aggregator_response(yaml_text: &str) -> Result { raw_llm_response: yaml_text.to_string(), parse_error: None, raw_dump_path: None, + llm_provider: None, + llm_model: None, }) } @@ -171,6 +183,8 @@ fn build_expert_report(expert_name: &str, raw_response: &str, value: &serde_yaml raw_llm_response: raw_response.to_string(), parse_error: None, raw_dump_path: None, + llm_provider: None, + llm_model: None, }) } diff --git a/src/output/team_renderer/tests.rs b/src/output/team_renderer/tests.rs index 06149e2..22ac4de 100644 --- a/src/output/team_renderer/tests.rs +++ b/src/output/team_renderer/tests.rs @@ -39,6 +39,8 @@ fn test_render_team_report_with_findings() { raw_llm_response: String::new(), parse_error: None, raw_dump_path: None, + llm_provider: None, + llm_model: None, }]; let metrics = vec![ExpertMetrics { name: "security".to_string(), @@ -79,6 +81,8 @@ fn test_render_team_report_with_custom_scoring() { raw_llm_response: String::new(), parse_error: None, raw_dump_path: None, + llm_provider: None, + llm_model: None, }]; let metrics = vec![ExpertMetrics { name: "security".to_string(), @@ -124,6 +128,8 @@ fn test_render_team_report_backward_compatible() { raw_llm_response: String::new(), parse_error: None, raw_dump_path: None, + llm_provider: None, + llm_model: None, }]; let metrics = vec![ExpertMetrics { name: "security".to_string(), @@ -354,6 +360,8 @@ fn test_render_expert_section_plain_report_unchanged() { raw_llm_response: "raw".to_string(), parse_error: None, raw_dump_path: None, + llm_provider: None, + llm_model: None, }; assert_eq!(render_expert_section(&report), report.markdown); } @@ -367,6 +375,8 @@ fn test_render_expert_section_parse_error_surfaces_instead_of_no_issues() { raw_llm_response: "review:\n findings: [unclosed".to_string(), parse_error: Some("YAML parse failed".to_string()), raw_dump_path: None, + llm_provider: None, + llm_model: None, }; let section = render_expert_section(&report); assert!( @@ -389,6 +399,8 @@ fn test_render_expert_section_raw_dump_path_referenced() { raw_llm_response: "x".repeat(1200), parse_error: None, raw_dump_path: Some("/tmp/report.raw/security.1.response.txt".to_string()), + llm_provider: None, + llm_model: None, }; let section = render_expert_section(&report); assert!(section.contains("Raw LLM response"), "raw section must be present"); diff --git a/src/repo/experts/aggregator.rs b/src/repo/experts/aggregator.rs index 2c07c1b..26a9d6e 100644 --- a/src/repo/experts/aggregator.rs +++ b/src/repo/experts/aggregator.rs @@ -262,6 +262,8 @@ fn consolidate_chunk_findings( raw_llm_response: String::new(), parse_error: None, raw_dump_path: None, + llm_provider: None, + llm_model: None, }; let consolidator = match app_config { Some(c) => ConsolidatorConfig { diff --git a/src/repo/experts/llm_experts/tests.rs b/src/repo/experts/llm_experts/tests.rs index 56a0e16..94c5bbc 100644 --- a/src/repo/experts/llm_experts/tests.rs +++ b/src/repo/experts/llm_experts/tests.rs @@ -409,6 +409,7 @@ impl LLMProvider for ScriptedProvider { content: self.bodies[i].clone(), total_tokens: 1, model: "mock".to_string(), + provider: "mock".to_string(), }) } } diff --git a/src/server/api/dashboard.rs b/src/server/api/dashboard.rs index 060f262..76d1a4a 100644 --- a/src/server/api/dashboard.rs +++ b/src/server/api/dashboard.rs @@ -229,6 +229,7 @@ mod tests { }, progress: None, expert_name: None, + llm_summary: None, } } diff --git a/src/server/api/repo.rs b/src/server/api/repo.rs index 02a3671..be7f70a 100644 --- a/src/server/api/repo.rs +++ b/src/server/api/repo.rs @@ -153,6 +153,7 @@ async fn submit_repo_scan(State(state): State>, Json(body): Json ReviewDetail { } else { Some(report.raw_llm_response.clone()) }, + llm_provider: report.llm_provider.clone(), + llm_model: report.llm_model.clone(), }) .collect(); let raw_comment = output @@ -128,6 +130,12 @@ pub(crate) fn build_review_list_item(entry: &TaskEntry) -> ReviewListItem { duration_ms: entry.duration_ms(), created_at: entry.created_at.to_rfc3339(), gitlab_mr_url: meta.gitlab_mr_url.clone(), + // RENG-38: the snapshot column is TEXT JSON; a corrupt value degrades + // to None (the list shows "unknown") instead of failing the page. + llm_summary: entry + .llm_summary + .as_deref() + .and_then(|s| serde_json::from_str::>(s).ok()), } } @@ -384,6 +392,7 @@ mod tests { source_meta: SourceMeta::default(), progress: None, expert_name: None, + llm_summary: None, } } @@ -417,6 +426,8 @@ mod tests { raw_llm_response: String::new(), parse_error: None, raw_dump_path: None, + llm_provider: None, + llm_model: None, } } diff --git a/src/server/api/review/tests.rs b/src/server/api/review/tests.rs index 1d70c33..2f3ac53 100644 --- a/src/server/api/review/tests.rs +++ b/src/server/api/review/tests.rs @@ -150,6 +150,8 @@ fn make_report(name: &str, findings: Vec) -> crate::mode raw_llm_response: format!("raw {}", name), parse_error: None, raw_dump_path: None, + llm_provider: None, + llm_model: None, } } diff --git a/src/server/api/types.rs b/src/server/api/types.rs index c538c68..1356d4a 100644 --- a/src/server/api/types.rs +++ b/src/server/api/types.rs @@ -150,6 +150,11 @@ pub struct ExpertResultDetail { pub score: Option, pub summary: String, pub details: Option, + /// RENG-38: name snapshot of the LLM provider that actually produced this + /// expert's report (the fallback-chain hit). `null` for pre-0.10.2 records. + pub llm_provider: Option, + /// Model identifier snapshot paired with `llm_provider`. + pub llm_model: Option, } /// Author of the reviewed MR/PR, matching `ReviewDetail.author`. @@ -212,4 +217,8 @@ pub struct ReviewListItem { pub duration_ms: Option, pub created_at: String, pub gitlab_mr_url: Option, + /// RENG-38: deduplicated LLM `(provider, model)` pairs used by this + /// review, decoded from the `reviews.llm_summary` snapshot column. + /// `null` for pre-0.10.2 records and non-completed tasks. + pub llm_summary: Option>, } diff --git a/src/server/github.rs b/src/server/github.rs index d38b69a..34ac46e 100644 --- a/src/server/github.rs +++ b/src/server/github.rs @@ -598,6 +598,8 @@ mod tests { raw_llm_response: "raw".to_string(), parse_error: None, raw_dump_path: None, + llm_provider: None, + llm_model: None, }; let outcome: anyhow::Result = Ok(crate::models::ReviewOutput::new(vec![report])); diff --git a/src/server/gitlab/hooks.rs b/src/server/gitlab/hooks.rs index e2b303b..955a737 100644 --- a/src/server/gitlab/hooks.rs +++ b/src/server/gitlab/hooks.rs @@ -863,6 +863,8 @@ mod tests { raw_llm_response: "raw".to_string(), parse_error: None, raw_dump_path: None, + llm_provider: None, + llm_model: None, }; let output = crate::models::ReviewOutput::new(vec![report]); let outcome: anyhow::Result = Ok(output); diff --git a/src/server/mod.rs b/src/server/mod.rs index a31be2f..375824a 100644 --- a/src/server/mod.rs +++ b/src/server/mod.rs @@ -382,6 +382,8 @@ mod tests { raw_llm_response: String::new(), parse_error: None, raw_dump_path: None, + llm_provider: None, + llm_model: None, }]; let agg = Some(AggregatedReport { findings: vec![], @@ -389,6 +391,8 @@ mod tests { raw_llm_response: String::new(), parse_error: None, raw_dump_path: None, + llm_provider: None, + llm_model: None, }); let output = build_review_output_from_reports(reports, agg); assert!( @@ -406,6 +410,8 @@ mod tests { raw_llm_response: String::new(), parse_error: None, raw_dump_path: None, + llm_provider: None, + llm_model: None, }]; let output = build_review_output_from_reports(reports, None); assert!( @@ -445,6 +451,8 @@ mod tests { raw_llm_response: "---\n".to_string(), parse_error: None, raw_dump_path: None, + llm_provider: None, + llm_model: None, }; let reports = vec![ExpertReport { expert_name: "security".to_string(), @@ -453,6 +461,8 @@ mod tests { raw_llm_response: String::new(), parse_error: None, raw_dump_path: None, + llm_provider: None, + llm_model: None, }]; let output = build_review_output_from_reports(reports, Some(agg_report)); @@ -472,6 +482,8 @@ mod tests { raw_llm_response: String::new(), parse_error: None, raw_dump_path: None, + llm_provider: None, + llm_model: None, }]; let output = build_review_output_from_reports(reports, None); assert!(output.aggregated.is_none()); @@ -492,6 +504,8 @@ mod tests { raw_llm_response: String::new(), parse_error: None, raw_dump_path: None, + llm_provider: None, + llm_model: None, }]; let output = build_review_output_from_reports(reports, None); assert!(output.aggregated.is_none()); diff --git a/src/server/task_queue.rs b/src/server/task_queue.rs index f7649cf..409d926 100644 --- a/src/server/task_queue.rs +++ b/src/server/task_queue.rs @@ -90,6 +90,12 @@ pub struct TaskEntry { pub source_meta: SourceMeta, pub progress: Option, // 0-100 pub expert_name: Option, // current active expert + /// RENG-38: JSON snapshot of the deduplicated `[{provider, model}]` LLM + /// pairs that produced this review's reports (see + /// [`crate::models::ReviewOutput::llm_usages`]). Filled on terminal + /// `update` from `result` so the in-memory path (db=None) and the + /// write-through row carry the same value. + pub llm_summary: Option, } /// A real-time event broadcast to SSE subscribers when a task's state changes. @@ -220,6 +226,7 @@ impl TaskStore { source_meta: source_meta.unwrap_or_default(), progress: None, expert_name: None, + llm_summary: None, }; self.inner.write().await.insert(id, entry.clone()); let _ = self.tx.send(TaskEvent { @@ -303,6 +310,9 @@ impl TaskStore { entry.state = new_state.clone(); entry.result = result; entry.error = error.clone(); + // RENG-38: refresh the LLM-usage snapshot from the new result + // (terminal transitions carry it; mid-flight updates clear it). + entry.llm_summary = entry.result.as_ref().and_then(crate::store::rows::llm_summary_json); if new_state == TaskState::Completed || new_state == TaskState::Failed || new_state == TaskState::Cancelled { entry.completed_at = Some(chrono::Utc::now()); @@ -820,6 +830,7 @@ mod tests { source_meta: SourceMeta::default(), progress: None, expert_name: None, + llm_summary: None, }; assert_eq!(entry.duration_ms(), Some(0), "inverted span must clamp, not wrap"); @@ -952,6 +963,8 @@ mod tests { raw_llm_response: "raw".to_string(), parse_error: None, raw_dump_path: None, + llm_provider: None, + llm_model: None, }; crate::models::ReviewOutput { reports: vec![report("security"), report("performance")], diff --git a/src/store/mod.rs b/src/store/mod.rs index 29500a1..ca8c919 100644 --- a/src/store/mod.rs +++ b/src/store/mod.rs @@ -334,7 +334,29 @@ mod tests { .fetch_one(store.pool()) .await .unwrap(); - assert_eq!(applied, 1, "only 0001_init should be recorded"); + assert_eq!(applied, 2, "0001_init + 0002_llm_snapshot should be recorded"); + + // 0002 (RENG-38): the snapshot columns exist on both history tables. + let er_cols: Vec = ::sqlx::query_scalar("SELECT name FROM pragma_table_info('expert_reports')") + .fetch_all(store.pool()) + .await + .unwrap(); + assert!( + er_cols.iter().any(|c| c == "llm_provider"), + "expert_reports missing llm_provider" + ); + assert!( + er_cols.iter().any(|c| c == "llm_model"), + "expert_reports missing llm_model" + ); + let rv_cols: Vec = ::sqlx::query_scalar("SELECT name FROM pragma_table_info('reviews')") + .fetch_all(store.pool()) + .await + .unwrap(); + assert!( + rv_cols.iter().any(|c| c == "llm_summary"), + "reviews missing llm_summary" + ); } /// 验证点 A(b): `?` placeholder INSERT + SELECT round trip on SQLite diff --git a/src/store/rows.rs b/src/store/rows.rs index 1edab91..51da592 100644 --- a/src/store/rows.rs +++ b/src/store/rows.rs @@ -197,6 +197,10 @@ pub(crate) struct ReviewRow { pub result: Option, pub error: Option, pub progress: Option, + /// RENG-38: deduplicated `[{provider, model}]` JSON snapshot of the LLMs + /// that produced this review (`ReviewOutput::llm_usages`), materialized + /// at write time so the history list never parses `result` (§8.1). + pub llm_summary: Option, pub created_at: String, pub started_at: Option, pub completed_at: Option, @@ -209,6 +213,22 @@ fn opt_json(value: &Option, what: &str) -> Result> { .transpose() } +/// RENG-38: compute the `reviews.llm_summary` TEXT (JSON array of +/// [`crate::models::LlmUsage`]) from a serialized `ReviewOutput` result. +/// `None` when the result is absent, not a `ReviewOutput`, or carries no +/// llm snapshots (pre-0.10.2 records, all-experts-failed runs) — a +/// non-ReviewOutput result is a legitimate shape (`complete` only warns and +/// skips the expert_reports split), never a store error. +pub(crate) fn llm_summary_json(result: &Value) -> Option { + let output: crate::models::ReviewOutput = serde_json::from_value(result.clone()).ok()?; + let usages = output.llm_usages(); + if usages.is_empty() { + None + } else { + serde_json::to_string(&usages).ok() + } +} + /// `TaskState` → the `reviews.state` string. Single source of truth is the /// API projection mapping (`task_status_str`); the store reuses it so the /// DB vocabulary can never drift from the SSE / API vocabulary (§5.3). @@ -246,6 +266,12 @@ pub(crate) fn task_entry_to_row(entry: &TaskEntry) -> Result { result: opt_json(&entry.result, "reviews.result")?, error: entry.error.clone(), progress: entry.progress.map(i64::from), + // The entry's live field wins (filled on terminal update); when it is + // absent (e.g. the create-time write-through) compute from `result`. + llm_summary: entry + .llm_summary + .clone() + .or_else(|| entry.result.as_ref().and_then(llm_summary_json)), created_at: encode_ts(&entry.created_at), started_at: entry.started_at.as_ref().map(encode_ts), completed_at: entry.completed_at.as_ref().map(encode_ts), @@ -255,7 +281,7 @@ pub(crate) fn task_entry_to_row(entry: &TaskEntry) -> Result { /// Column list of the shared `reviews` SELECT used by the read path /// (`sqlx.rs`); the order matches [`ReviewRowTuple`]. pub(crate) const REVIEW_COLUMNS: &str = "task_id, state, source_meta, project, repository, request, \ - result, error, progress, created_at, started_at, completed_at"; + result, error, progress, llm_summary, created_at, started_at, completed_at"; /// Raw decode target for a `SELECT {REVIEW_COLUMNS}` query, in column order. #[allow(clippy::type_complexity)] @@ -269,6 +295,7 @@ pub(crate) type ReviewRowTuple = ( Option, Option, Option, + Option, String, Option, Option, @@ -286,6 +313,7 @@ impl From for ReviewRow { result, error, progress, + llm_summary, created_at, started_at, completed_at, @@ -301,6 +329,7 @@ impl From for ReviewRow { result, error, progress, + llm_summary, created_at, started_at, completed_at, @@ -359,6 +388,7 @@ pub(crate) fn review_from_row(row: ReviewRow) -> Result { .progress .map(|p| u8::try_from(p).with_context(|| format!("reviews.progress out of range: {p}"))) .transpose()?, + llm_summary: row.llm_summary, // Live-only fields: the DB is the history source, the in-memory // `expert_name` (current active expert) is not persisted. expert_name: None, @@ -374,6 +404,11 @@ pub(crate) struct ExpertReportRow { /// Per-expert duration: always NULL for now — `TaskEntry` does not track /// it yet (design/persistence.md §5.4 note). pub duration_ms: Option, + /// RENG-38: LLM name snapshots denormalized from the report JSON so the + /// per-expert provider/model is queryable without parsing `report`. + /// NULL for pre-0.10.2 rows and non-LLM reports. + pub llm_provider: Option, + pub llm_model: Option, pub created_at: String, } @@ -392,6 +427,8 @@ pub(crate) fn expert_report_rows(task_id: &Uuid, result: &Value, created_at: Str report: serde_json::to_string(report) .with_context(|| format!("serialize expert report {:?}", report.expert_name))?, duration_ms: None, + llm_provider: report.llm_provider.clone(), + llm_model: report.llm_model.clone(), created_at: created_at.clone(), }) }) @@ -451,6 +488,7 @@ mod tests { result: None, error: None, progress: Some(100), + llm_summary: None, created_at: "2026-09-03T01:00:00.000000Z".to_string(), started_at: Some("2026-09-03T01:00:01.000000Z".to_string()), completed_at: Some("2026-09-03T01:00:42.000000Z".to_string()), diff --git a/src/store/sqlx.rs b/src/store/sqlx.rs index 66d4da8..adeccd1 100644 --- a/src/store/sqlx.rs +++ b/src/store/sqlx.rs @@ -325,8 +325,8 @@ impl ReviewStore for SqlxStore { let row = rows::task_entry_to_row(entry)?; let sql = self.sql( "INSERT INTO reviews (task_id, state, source_meta, project, repository, request, \ - result, error, progress, created_at, started_at, completed_at) \ - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)", + result, error, progress, llm_summary, created_at, started_at, completed_at) \ + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)", ); ::sqlx::query(&sql) .bind(&row.task_id) @@ -338,6 +338,7 @@ impl ReviewStore for SqlxStore { .bind(&row.result) .bind(&row.error) .bind(row.progress) + .bind(&row.llm_summary) .bind(&row.created_at) .bind(&row.started_at) .bind(&row.completed_at) @@ -378,7 +379,7 @@ impl ReviewStore for SqlxStore { let report_created_at = row.completed_at.clone().unwrap_or_else(|| encode_ts(&Utc::now())); let mut tx = self.pool().begin().await.context("begin complete review")?; let update = self.sql( - "UPDATE reviews SET state = ?, result = ?, error = ?, completed_at = ?, progress = ? \ + "UPDATE reviews SET state = ?, result = ?, error = ?, completed_at = ?, progress = ?, llm_summary = ? \ WHERE task_id = ?", ); let res = ::sqlx::query(&update) @@ -387,6 +388,7 @@ impl ReviewStore for SqlxStore { .bind(&row.error) .bind(&row.completed_at) .bind(row.progress) + .bind(&row.llm_summary) .bind(&row.task_id) .execute(&mut *tx) .await @@ -404,8 +406,8 @@ impl ReviewStore for SqlxStore { match rows::expert_report_rows(&entry.task_id, result, report_created_at) { Ok(report_rows) => { let insert = self.sql( - "INSERT INTO expert_reports (task_id, expert_name, report, duration_ms, created_at) \ - VALUES (?, ?, ?, ?, ?)", + "INSERT INTO expert_reports (task_id, expert_name, report, duration_ms, llm_provider, llm_model, created_at) \ + VALUES (?, ?, ?, ?, ?, ?, ?)", ); for report in &report_rows { ::sqlx::query(&insert) @@ -413,6 +415,8 @@ impl ReviewStore for SqlxStore { .bind(&report.expert_name) .bind(&report.report) .bind(report.duration_ms) + .bind(&report.llm_provider) + .bind(&report.llm_model) .bind(&report.created_at) .execute(&mut *tx) .await @@ -1006,6 +1010,7 @@ mod tests { }, progress: None, expert_name: None, + llm_summary: None, }; ReviewStore::create(&store, &entry).await.unwrap(); @@ -1046,6 +1051,7 @@ mod tests { created_at, started_at, completed_at, + llm_summary: None, }) .unwrap(); assert_eq!(decoded.task_id, entry.task_id); @@ -1064,6 +1070,147 @@ mod tests { assert!(decoded.expert_name.is_none(), "live-only field is not persisted"); } + /// RENG-38: completing a review persists the LLM usage snapshot — + /// per-expert `expert_reports.llm_provider/llm_model` columns and the + /// deduplicated `reviews.llm_summary` list — and the read path hands the + /// summary back on the decoded `TaskEntry`. + #[tokio::test] + async fn complete_persists_llm_snapshot_columns() { + let store = fresh_store().await; + + fn report(expert: &str, provider: &str, model: &str) -> crate::models::ExpertReport { + crate::models::ExpertReport { + expert_name: expert.to_string(), + findings: Vec::new(), + markdown: String::new(), + raw_llm_response: String::new(), + parse_error: None, + raw_dump_path: None, + llm_provider: Some(provider.to_string()), + llm_model: Some(model.to_string()), + } + } + + let entry = TaskEntry { + task_id: uuid::Uuid::new_v4(), + state: TaskState::Pending, + created_at: Utc::now(), + started_at: None, + completed_at: None, + result: None, + error: None, + request: None, + source_meta: Default::default(), + progress: None, + expert_name: None, + llm_summary: None, + }; + ReviewStore::create(&store, &entry).await.unwrap(); + + // Two experts share one provider/model pair (must dedup), a third + // uses a different model on the same provider. + let mut output = crate::models::ReviewOutput::new(vec![ + report("security", "xiaomi", "mimo-v2.5-pro"), + report("performance", "xiaomi", "mimo-v2.5-pro"), + report("quality", "xiaomi", "mimo-v2-pro"), + ]); + output.aggregated = Some(crate::models::AggregatedReport { + findings: Vec::new(), + markdown: String::new(), + raw_llm_response: String::new(), + parse_error: None, + raw_dump_path: None, + llm_provider: Some("anthropic".to_string()), + llm_model: Some("claude-4".to_string()), + }); + let mut completed = entry.clone(); + completed.state = TaskState::Completed; + completed.completed_at = Some(Utc::now()); + completed.result = Some(serde_json::to_value(&output).unwrap()); + ReviewStore::complete(&store, &completed).await.unwrap(); + + // Per-expert snapshot columns are populated from the report JSON. + let rows: Vec<(String, Option, Option)> = ::sqlx::query_as( + "SELECT expert_name, llm_provider, llm_model FROM expert_reports WHERE task_id = ? ORDER BY expert_name", + ) + .bind(completed.task_id.to_string()) + .fetch_all(store.pool()) + .await + .unwrap(); + assert_eq!(rows.len(), 3); + assert_eq!( + rows[0], + ( + "performance".to_string(), + Some("xiaomi".to_string()), + Some("mimo-v2.5-pro".to_string()) + ) + ); + // ORDER BY expert_name: performance, quality, security. + assert_eq!(rows[1].1.as_deref(), Some("xiaomi")); + assert_eq!(rows[1].2.as_deref(), Some("mimo-v2-pro")); + + // The review-level summary dedups pairs in first-seen order and + // includes the aggregator's pair. + let summary: Option = ::sqlx::query_scalar("SELECT llm_summary FROM reviews WHERE task_id = ?") + .bind(completed.task_id.to_string()) + .fetch_one(store.pool()) + .await + .unwrap(); + let summary = summary.expect("llm_summary must be set"); + let usages: Vec = serde_json::from_str(&summary).unwrap(); + assert_eq!( + usages, + vec![ + crate::models::LlmUsage { + provider: "xiaomi".into(), + model: "mimo-v2.5-pro".into() + }, + crate::models::LlmUsage { + provider: "xiaomi".into(), + model: "mimo-v2-pro".into() + }, + crate::models::LlmUsage { + provider: "anthropic".into(), + model: "claude-4".into() + }, + ] + ); + + // Read path: the decoded entry carries the summary back. + let decoded = ReviewStore::get_review(&store, completed.task_id) + .await + .unwrap() + .unwrap(); + assert_eq!(decoded.llm_summary.as_deref(), Some(summary.as_str())); + + // A result without llm snapshots (pre-0.10.2 shape) leaves the + // columns NULL rather than erroring. + let legacy = crate::models::ReviewOutput::new(vec![{ + let mut r = report("security", "xiaomi", "mimo-v2.5-pro"); + r.llm_provider = None; + r.llm_model = None; + r + }]); + let mut legacy_entry = entry.clone(); + legacy_entry.task_id = uuid::Uuid::new_v4(); + ReviewStore::create(&store, &legacy_entry).await.unwrap(); + legacy_entry.state = TaskState::Completed; + legacy_entry.completed_at = Some(Utc::now()); + legacy_entry.result = Some(serde_json::to_value(&legacy).unwrap()); + ReviewStore::complete(&store, &legacy_entry).await.unwrap(); + let (provider, summary): (Option, Option) = ::sqlx::query_as( + "SELECT (SELECT llm_provider FROM expert_reports WHERE task_id = ?), llm_summary FROM reviews WHERE task_id = ?", + ) + .bind(legacy_entry.task_id.to_string()) + .bind(legacy_entry.task_id.to_string()) + .fetch_one(store.pool()) + .await + .unwrap(); + assert!(provider.is_none(), "legacy report must keep llm_provider NULL"); + assert!(summary.is_none(), "legacy review must keep llm_summary NULL"); + } + // ─── DiscussionStore (step 6a) ─── fn note(note_id: u64, created_at: &str, body: &str) -> DiscussionNote { diff --git a/src/team/lead_consolidator/tests.rs b/src/team/lead_consolidator/tests.rs index 4021cf8..d1f2139 100644 --- a/src/team/lead_consolidator/tests.rs +++ b/src/team/lead_consolidator/tests.rs @@ -30,6 +30,8 @@ fn make_report(expert_name: &str, findings: Vec) -> ExpertReport { raw_llm_response: String::new(), parse_error: None, raw_dump_path: None, + llm_provider: None, + llm_model: None, } } diff --git a/src/team/orchestrator/mod.rs b/src/team/orchestrator/mod.rs index 22e03db..0a46c91 100644 --- a/src/team/orchestrator/mod.rs +++ b/src/team/orchestrator/mod.rs @@ -169,7 +169,10 @@ impl TeamOrchestrator for DefaultOrchestrator { prompt_engine.build_aggregator_prompt(&reports, &mr_info, _global_context.as_ref(), "en")?; let llm_config = select_llm_config(aggregator, llm_configs); let result = llm_client.complete_with_fallback(&llm_config, &system, &user).await?; - let agg_report = crate::output::parser::parse_aggregator_response(&result.content)?; + let mut agg_report = crate::output::parser::parse_aggregator_response(&result.content)?; + // RENG-38: snapshot the aggregator's actual LLM. + agg_report.llm_provider = Some(result.provider.clone()); + agg_report.llm_model = Some(result.model.clone()); Some(agg_report) } else { None @@ -321,7 +324,12 @@ pub async fn run_aggregator( } } - crate::output::parser::parse_aggregator_response(&result.content) + crate::output::parser::parse_aggregator_response(&result.content).map(|mut agg| { + // RENG-38: snapshot the aggregator's actual LLM. + agg.llm_provider = Some(result.provider.clone()); + agg.llm_model = Some(result.model.clone()); + agg + }) } /// Resolve the review input into raw diff text and MR info. diff --git a/src/team/orchestrator/pipeline.rs b/src/team/orchestrator/pipeline.rs index 8b1acad..13da438 100644 --- a/src/team/orchestrator/pipeline.rs +++ b/src/team/orchestrator/pipeline.rs @@ -297,8 +297,11 @@ fn create_expert_task( )?; let llm_config = select_llm_config(&expert, &llm_configs); let result = llm_client.complete_with_fallback(&llm_config, &system, &user).await?; - let report = crate::output::parser::parse_llm_response(&expert.name, &result.content); - let mut report = report; + let mut report = crate::output::parser::parse_llm_response(&expert.name, &result.content); + // RENG-38: snapshot the LLM that actually produced this report (the + // fallback-chain hit), so history can show provider/model per expert. + report.llm_provider = Some(result.provider.clone()); + report.llm_model = Some(result.model.clone()); // `--verbose`: persist the raw LLM prompt + response to the dump dir so // a zero-finding or mis-parsed run can be debugged from the actual LLM // exchange, and reference the file path on the report (the renderer diff --git a/src/team/orchestrator/tests.rs b/src/team/orchestrator/tests.rs index 7f4a1f3..ba2ca34 100644 --- a/src/team/orchestrator/tests.rs +++ b/src/team/orchestrator/tests.rs @@ -32,6 +32,8 @@ fn make_report(expert_name: &str, findings: Vec) -> ExpertReport { raw_llm_response: String::new(), parse_error: None, raw_dump_path: None, + llm_provider: None, + llm_model: None, } } diff --git a/src/team/verifier.rs b/src/team/verifier.rs index 637d015..507b7fb 100644 --- a/src/team/verifier.rs +++ b/src/team/verifier.rs @@ -374,6 +374,8 @@ mod tests { raw_llm_response: String::new(), parse_error: None, raw_dump_path: None, + llm_provider: None, + llm_model: None, } } diff --git a/tests/server/llm_configs.rs b/tests/server/llm_configs.rs index 657c104..f81cbbb 100644 --- a/tests/server/llm_configs.rs +++ b/tests/server/llm_configs.rs @@ -558,3 +558,90 @@ async fn webhook_review_uses_hot_applied_server_llm_configs() { "the WebUI-configured LLM provider must actually have been called by the webhook review" ); } + +// ─── RENG-38: LLM usage snapshot in review history ─────────────── + +/// RENG-38: a completed review records which LLM actually produced each +/// expert report. The mock poses as provider `xiaomi` answering with model +/// `mimo-v2.5-pro`; afterwards the detail endpoint must carry +/// `experts[].llmProvider/llmModel` and the history list must carry the +/// deduplicated `llmSummary` (read from the `reviews.llm_summary` column, +/// not re-parsed from `result`). +#[tokio::test] +async fn completed_review_history_carries_llm_snapshot() { + let mock = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/chat/completions")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "choices": [{"message": {"content": "findings: []"}}], + "usage": {"total_tokens": 8}, + "model": "mimo-v2.5-pro" + }))) + .mount(&mock) + .await; + let llm_config_env = serde_json::json!([{ + "provider": "xiaomi", + "model": "mimo-v2.5-pro", + "api_key": "sk-test", + "api_base": mock.uri(), + "max_tokens": 2048, + "temperature": 0.3 + }]) + .to_string(); + + let port = find_free_port(); + let _guard = spawn_server_inner_with_env(port, None, &[("LLM_CONFIG", &llm_config_env)]); + wait_for_server(port).await; + + let client = bootstrap_authed_client(port, API_TOKEN).await; + let base = format!("http://127.0.0.1:{}", port); + let final_body = post_review_and_poll(&base, &client, None).await; + assert_eq!( + final_body["status"].as_str(), + Some("completed"), + "review must complete against the mock LLM, got {:?}", + final_body + ); + let task_id = final_body["task_id"].as_str().expect("task_id"); + + // Detail: every expert report names the LLM that produced it. + let experts = final_body["experts"].as_array().expect("experts is an array"); + assert!(!experts.is_empty(), "a completed review must have expert results"); + for expert in experts { + assert_eq!( + expert["llmProvider"].as_str(), + Some("xiaomi"), + "expert {} missing llmProvider: {:?}", + expert["expertName"], + expert + ); + assert_eq!( + expert["llmModel"].as_str(), + Some("mimo-v2.5-pro"), + "expert {} missing llmModel: {:?}", + expert["expertName"], + expert + ); + } + + // List: the review-level summary is the deduplicated pair list. + let list: serde_json::Value = client + .get(format!("{}/api/v1/reviews?per_page=100", base)) + .send() + .await + .expect("failed to GET /api/v1/reviews") + .json() + .await + .expect("reviews list body is not JSON"); + let items = list["items"].as_array().expect("reviews.items is an array"); + let item = items + .iter() + .find(|i| i["id"].as_str() == Some(task_id) || i["task_id"].as_str() == Some(task_id)) + .expect("completed review must appear in the history list"); + assert_eq!( + item["llmSummary"], + serde_json::json!([{ "provider": "xiaomi", "model": "mimo-v2.5-pro" }]), + "list item must carry the deduplicated llmSummary: {:?}", + item + ); +} From 76981866e1fb87a4866cd8d1fc62caad4057afb2 Mon Sep 17 00:00:00 2001 From: isletspace Date: Wed, 9 Sep 2026 10:01:06 +0800 Subject: [PATCH 2/3] feat(history): show expert status tag only on failure and move LLM tag into expanded panel (RENG-38) (0.10.2) --- frontend/src/views/ReviewHistory.vue | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/frontend/src/views/ReviewHistory.vue b/frontend/src/views/ReviewHistory.vue index 91ab6b0..e5acffa 100644 --- a/frontend/src/views/ReviewHistory.vue +++ b/frontend/src/views/ReviewHistory.vue @@ -799,19 +799,24 @@ watch(() => route.query, () => {
{{ exp.expertName }}
- + + {{ exp.score }} - - - {{ expertLlmLabel(exp) }} -
+ +
+ + {{ expertLlmLabel(exp) }} + +
From 1a6ecd006a59dede3afcae5f50e080c9ce5a5254 Mon Sep 17 00:00:00 2001 From: isletspace Date: Thu, 10 Sep 2026 14:38:33 +0800 Subject: [PATCH 3/3] chore(release): bump version to 0.10.3 (RENG-38) --- Cargo.lock | 2 +- Cargo.toml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index cf73fc5..6e63722 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2134,7 +2134,7 @@ dependencies = [ [[package]] name = "review-engine" -version = "0.10.2" +version = "0.10.3" dependencies = [ "anyhow", "async-trait", diff --git a/Cargo.toml b/Cargo.toml index 262bc6e..817192c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "review-engine" -version = "0.10.2" +version = "0.10.3" license = "Apache-2.0" edition = "2021"