From b3d7b050005033f2700edd6b7bff923422798f53 Mon Sep 17 00:00:00 2001 From: SwartzMss Date: Thu, 23 Jul 2026 19:50:55 +0800 Subject: [PATCH 01/19] docs: design malformed AI findings retry --- ...26-07-23-ai-malformed-json-retry-design.md | 77 +++++++++++++++++++ 1 file changed, 77 insertions(+) create mode 100644 docs/superpowers/specs/2026-07-23-ai-malformed-json-retry-design.md diff --git a/docs/superpowers/specs/2026-07-23-ai-malformed-json-retry-design.md b/docs/superpowers/specs/2026-07-23-ai-malformed-json-retry-design.md new file mode 100644 index 0000000..209d893 --- /dev/null +++ b/docs/superpowers/specs/2026-07-23-ai-malformed-json-retry-design.md @@ -0,0 +1,77 @@ +# AI 审查残缺 JSON 可观测性与恢复设计 + +## 背景 + +AI 审查供应商可能返回 HTTP 200，但最终 `submit_review_findings` 参数或 +assistant content 是被截断的 JSON。当前实现只重试传输、超时和部分 HTTP +错误；最终 JSON 解析失败会直接使该 AI 审查任务失败。同时，响应模型没有保留 +`finish_reason` 和 token usage，日志无法区分输出长度限制与供应商生成异常。 + +## 目标 + +- 记录首个 choice 的 `finish_reason` 以及响应 token usage。 +- 保持对不返回这些可选字段的 OpenAI 兼容供应商的兼容性。 +- 仅在最终 findings 发生 `AiResponseParseFailed` 时执行一次恢复请求。 +- 恢复请求不得重放残缺 tool call，也不得形成无限重试。 +- 第二次仍失败时保留现有结构化错误行为。 + +## 非目标 + +- 不新增或调整 `max_tokens`、`max_completion_tokens` 配置。 +- 不尝试补齐、修剪或猜测残缺 JSON。 +- 不改变 HTTP、超时、工具预算以及 diff-only fallback 的既有重试语义。 +- 不重跑已经完成的上下文工具调用。 + +## 设计 + +### 响应元数据 + +`OpenAiChoice` 新增可选 `finish_reason`。`OpenAiChatResponse` 新增可选 usage， +usage 内的 prompt、completion 和 total token 均为可选值。缺失或供应商返回 +部分 usage 时仍可成功反序列化。 + +每次准备处理响应 choice 时输出结构化日志，至少包含： + +- AI review、model、attempt 和 batch 标识； +- `finish_reason`； +- prompt、completion、total token； +- assistant content 或 tool arguments 的字节数。 + +未知或缺失字段记录为空，不作为错误。 + +### 定向 finalization 重试 + +最终消息进入 `parse_openai_message` 后，如果错误码是 +`AiResponseParseFailed`，且本批尚未执行过 malformed-finalization retry： + +1. 丢弃该残缺 assistant 消息，不把它加入对话历史； +2. 保留原始 system/user 消息和已成功完成的工具上下文； +3. 追加一条可信 system 指令，说明上次最终参数不是完整 JSON，要求立即重新调用 + `submit_review_findings`，压缩描述并放弃证据不足的问题； +4. 关闭 `read_file`、`search_code` 和 `list_files`，只提供 + `submit_review_findings`； +5. 发起一次新的 finalization HTTP 请求。 + +重试响应正常时返回 findings。重试响应仍无法解析时直接返回第二次的 +`AiResponseParseFailed`，不再重试。 + +若当前是无 tool-call 的 JSON content fallback，则同样只进行一次定向请求， +继续使用 `response_format=json_object`，并要求重新输出完整、精简的 JSON。 + +### 错误边界 + +只对最终 findings 的结构化解析失败启用该恢复路径。外层 OpenAI 响应无法解析、 +无 choices、无 content、非 2xx、超时和其他错误继续沿用现有处理逻辑，避免扩大 +重试范围或掩盖协议错误。 + +## 测试 + +遵循 TDD 增加以下覆盖： + +1. OpenAI 响应可以解析完整和缺失的 finish reason/usage。 +2. 首次最终 findings 残缺、第二次完整时，仅发出一次恢复请求并返回 findings。 +3. 两次最终 findings 均残缺时返回解析错误，总请求次数有明确上限。 +4. 恢复请求不包含残缺 assistant tool call，且不再暴露上下文工具。 +5. 正常完整响应不产生额外请求。 + +完成后运行格式检查、相关单元测试、完整 `cargo test` 和 Clippy。 From ae8c90595ede0336a72cdb3a2036fe2e3a718c79 Mon Sep 17 00:00:00 2001 From: SwartzMss Date: Thu, 23 Jul 2026 19:55:31 +0800 Subject: [PATCH 02/19] docs: plan malformed AI findings retry --- .../2026-07-23-ai-malformed-json-retry.md | 343 ++++++++++++++++++ 1 file changed, 343 insertions(+) create mode 100644 docs/superpowers/plans/2026-07-23-ai-malformed-json-retry.md diff --git a/docs/superpowers/plans/2026-07-23-ai-malformed-json-retry.md b/docs/superpowers/plans/2026-07-23-ai-malformed-json-retry.md new file mode 100644 index 0000000..9d2572e --- /dev/null +++ b/docs/superpowers/plans/2026-07-23-ai-malformed-json-retry.md @@ -0,0 +1,343 @@ +# AI Review Malformed JSON Retry Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Make final AI review JSON failures observable and recover once without repeating context work or looping indefinitely. + +**Architecture:** Extend the OpenAI-compatible response DTO with optional completion metadata, then log that metadata at the response-processing boundary. Refactor final findings payload selection away from JSON decoding so the completion loop can distinguish a present-but-malformed payload from a missing payload and issue exactly one finalization-only retry. + +**Tech Stack:** Rust, Serde, Tokio, tracing, the existing local TCP HTTP test harness. + +--- + +### Task 1: Preserve and expose completion metadata + +**Files:** +- Modify: `src/review/ai_schema.rs` +- Modify: `src/review/ai.rs` + +- [ ] **Step 1: Write failing response compatibility tests** + +Add tests beside the existing response parsing tests in `src/review/ai.rs`: + +```rust +#[test] +fn parses_openai_completion_metadata_when_present() { + let response: OpenAiChatResponse = serde_json::from_str( + r#"{ + "choices":[{ + "finish_reason":"length", + "message":{"content":"{\"findings\":[]}"} + }], + "usage":{ + "prompt_tokens":120, + "completion_tokens":80, + "total_tokens":200 + } + }"#, + ) + .unwrap(); + + assert_eq!(response.choices[0].finish_reason.as_deref(), Some("length")); + let usage = response.usage.unwrap(); + assert_eq!(usage.prompt_tokens, Some(120)); + assert_eq!(usage.completion_tokens, Some(80)); + assert_eq!(usage.total_tokens, Some(200)); +} + +#[test] +fn parses_openai_response_without_completion_metadata() { + let response: OpenAiChatResponse = serde_json::from_str( + r#"{"choices":[{"message":{"content":"{\"findings\":[]}"}}]}"#, + ) + .unwrap(); + + assert_eq!(response.choices[0].finish_reason, None); + assert!(response.usage.is_none()); +} +``` + +- [ ] **Step 2: Run the metadata tests and verify RED** + +Run: + +```bash +cargo test review::ai::tests::parses_openai_completion_metadata_when_present +cargo test review::ai::tests::parses_openai_response_without_completion_metadata +``` + +Expected: compilation fails because `finish_reason` and `usage` do not exist. + +- [ ] **Step 3: Implement optional response fields** + +Update `src/review/ai_schema.rs`: + +```rust +#[derive(Deserialize)] +pub(crate) struct OpenAiChatResponse { + pub(crate) choices: Vec, + #[serde(default)] + pub(crate) usage: Option, +} + +#[derive(Deserialize)] +pub(crate) struct OpenAiChoice { + pub(crate) message: OpenAiMessage, + #[serde(default)] + pub(crate) finish_reason: Option, +} + +#[derive(Deserialize)] +pub(crate) struct OpenAiUsage { + #[serde(default)] + pub(crate) prompt_tokens: Option, + #[serde(default)] + pub(crate) completion_tokens: Option, + #[serde(default)] + pub(crate) total_tokens: Option, +} +``` + +At the start of each iteration of `complete_ai_review_response`, select the full choice instead of only its message and emit an `info!` event containing `finish_reason`, all three token counts, content bytes, tool-call count, attempt, model and batch fields. Use optional fields directly so missing metadata logs as `None`. + +- [ ] **Step 4: Run metadata tests and verify GREEN** + +Run: + +```bash +cargo test review::ai::tests::parses_openai_completion_metadata_when_present +cargo test review::ai::tests::parses_openai_response_without_completion_metadata +``` + +Expected: both tests pass. + +- [ ] **Step 5: Commit metadata support** + +```bash +git add src/review/ai_schema.rs src/review/ai.rs +git commit -m "feat: log AI completion metadata" +``` + +### Task 2: Retry one malformed tool-call finalization + +**Files:** +- Modify: `src/review/ai.rs` + +- [ ] **Step 1: Write a failing recovery test** + +Add an async local-server test following `retries_retryable_tool_loop_http_response_once_before_succeeding`. The server must: + +1. Return a `submit_review_findings` call whose arguments end at `{"findings":[`. +2. Inspect the second request and assert its tools are exactly `["submit_review_findings"]`. +3. Assert no assistant message contains the malformed call. +4. Assert a retry instruction mentions incomplete JSON. +5. Return a valid finding on request two. + +The final assertions are: + +```rust +assert_eq!(findings.len(), 1); +assert_eq!(findings[0].path, "src/lib.rs"); +assert_eq!(request_count.load(Ordering::SeqCst), 2); +assert_eq!(malformed_call_leaked.load(Ordering::SeqCst), 0); +assert_eq!(finalization_only_count.load(Ordering::SeqCst), 1); +``` + +- [ ] **Step 2: Run the recovery test and verify RED** + +Run: + +```bash +cargo test review::ai::tests::retries_malformed_submit_findings_once_without_replaying_it -- --nocapture +``` + +Expected: FAIL because the first parse error is returned and only one request reaches the server. + +- [ ] **Step 3: Separate payload lookup from payload decoding** + +Introduce a payload selector that reports missing output before JSON parsing: + +```rust +fn final_findings_payload(message: &OpenAiMessage) -> AppResult<&str> { + tool_call_arguments(message).or_else(|_| { + message + .content + .as_deref() + .map(str::trim) + .filter(|content| !content.is_empty()) + .ok_or_else(|| { + AppError::ai_review( + ReviewErrorCode::AiResponseParseFailed, + "AI review API returned no content", + ) + }) + }) +} +``` + +Make `parse_openai_message` call `final_findings_payload` and then `parse_ai_findings_response`. This keeps absent payload errors outside the malformed-payload retry condition. + +- [ ] **Step 4: Implement a single finalization retry** + +Add a loop flag: + +```rust +let mut malformed_finalization_retry_requested = false; +``` + +When a final response is reached: + +```rust +let payload = final_findings_payload(message)?; +match parse_ai_findings_payload(&config.id, &config.title, payload) { + Ok(findings) => return Ok(findings), + Err(err) if !malformed_finalization_retry_requested => { + malformed_finalization_retry_requested = true; + finalization_requested = true; + warn!(error = %err, "AI review final findings payload was malformed; retrying finalization once"); + messages.push(ChatMessage { + role: "user".into(), + content: Some(MALFORMED_FINALIZATION_INSTRUCTION.into()), + tool_call_id: None, + tool_calls: None, + }); + } + Err(err) => return Err(err), +} +``` + +Define a concise Chinese trusted instruction requiring a complete, compact result and prohibiting context tools. Serialize the follow-up using the current `use_tool_calls` mode and `context_tools_enabled=false`; do not append the malformed assistant response. + +- [ ] **Step 5: Run the recovery test and verify GREEN** + +Run: + +```bash +cargo test review::ai::tests::retries_malformed_submit_findings_once_without_replaying_it -- --nocapture +``` + +Expected: PASS with two HTTP requests. + +- [ ] **Step 6: Commit tool-call recovery** + +```bash +git add src/review/ai.rs +git commit -m "fix: retry malformed AI findings once" +``` + +### Task 3: Bound failure and cover JSON-content fallback + +**Files:** +- Modify: `src/review/ai.rs` + +- [ ] **Step 1: Write a failing bounded-retry test** + +Add a server test that always returns a present but truncated findings payload. Assert: + +```rust +assert_eq!( + execution + .result + .unwrap_err() + .review_failure() + .map(|failure| failure.code), + Some(ReviewErrorCode::AiResponseParseFailed) +); +assert_eq!(request_count.load(Ordering::SeqCst), 2); +``` + +- [ ] **Step 2: Run bounded-retry test** + +Run: + +```bash +cargo test review::ai::tests::stops_after_one_malformed_findings_retry -- --nocapture +``` + +Expected after Task 2: PASS. If it fails, adjust only the retry guard so no third request can occur. + +- [ ] **Step 3: Write a JSON-content fallback recovery test** + +Return HTTP 400 tool rejection for request one, malformed JSON content for request two, and valid JSON content for request three. On request three assert: + +```rust +assert_eq!(request["response_format"]["type"], "json_object"); +assert!(request.get("tools").is_none()); +assert!(request["messages"].as_array().unwrap().iter().any(|message| { + message["content"] + .as_str() + .is_some_and(|content| content.contains("JSON") && content.contains("完整")) +})); +``` + +Final assertions: + +```rust +assert!(findings.is_empty()); +assert_eq!(request_count.load(Ordering::SeqCst), 3); +``` + +- [ ] **Step 4: Run fallback recovery test and verify behavior** + +Run: + +```bash +cargo test review::ai::tests::retries_malformed_json_content_in_json_mode -- --nocapture +``` + +Expected: PASS; if it fails because the retry request enables tools, change follow-up serialization to preserve `use_tool_calls=false`. + +- [ ] **Step 5: Commit bounded and fallback coverage** + +```bash +git add src/review/ai.rs +git commit -m "test: cover malformed findings retry bounds" +``` + +### Task 4: Full verification + +**Files:** +- Modify only if formatting requires it: `src/review/ai.rs`, `src/review/ai_schema.rs` + +- [ ] **Step 1: Format and confirm no diff errors** + +Run: + +```bash +cargo fmt --check +git diff --check +``` + +Expected: both exit 0. If formatting fails, run `cargo fmt`, then repeat both checks. + +- [ ] **Step 2: Run all tests** + +Run: + +```bash +cargo test +``` + +Expected: all unit, integration and documentation tests pass with zero failures. + +- [ ] **Step 3: Run Clippy** + +Run: + +```bash +cargo clippy --all-targets --all-features -- -D warnings +``` + +Expected: exit 0 with no warnings. + +- [ ] **Step 4: Review scope and status** + +Run: + +```bash +git status --short +git diff main...HEAD --stat +git log --oneline main..HEAD +``` + +Expected: only the design, plan, response schema, AI completion loop and associated tests differ from `main`; the worktree is clean after the final commit. From 9261093c959e171cfa26fffd79d488f59ef2d979 Mon Sep 17 00:00:00 2001 From: SwartzMss Date: Thu, 23 Jul 2026 19:57:13 +0800 Subject: [PATCH 03/19] feat: log AI completion metadata --- src/review/ai.rs | 79 +++++++++++++++++++++++++++++++++++------ src/review/ai_schema.rs | 14 ++++++++ 2 files changed, 83 insertions(+), 10 deletions(-) diff --git a/src/review/ai.rs b/src/review/ai.rs index 4e61b7e..f31bc6c 100644 --- a/src/review/ai.rs +++ b/src/review/ai.rs @@ -886,16 +886,27 @@ async fn complete_ai_review_response(context: AiReviewCompletion<'_>) -> AppResu let response: OpenAiChatResponse = serde_json::from_str(&body).map_err(|err| { AppError::ai_review(ReviewErrorCode::AiResponseParseFailed, err.to_string()) })?; - let message = response - .choices - .first() - .map(|choice| &choice.message) - .ok_or_else(|| { - AppError::ai_review( - ReviewErrorCode::AiResponseParseFailed, - "AI review API returned no choices", - ) - })?; + let choice = response.choices.first().ok_or_else(|| { + AppError::ai_review( + ReviewErrorCode::AiResponseParseFailed, + "AI review API returned no choices", + ) + })?; + let message = &choice.message; + info!( + ai_review_id = %config.id, + model = %config.model, + attempt, + batch_index = batch.map(|(index, _)| index), + batch_count = batch.map(|(_, count)| count), + finish_reason = ?choice.finish_reason, + prompt_tokens = ?response.usage.as_ref().and_then(|usage| usage.prompt_tokens), + completion_tokens = ?response.usage.as_ref().and_then(|usage| usage.completion_tokens), + total_tokens = ?response.usage.as_ref().and_then(|usage| usage.total_tokens), + assistant_output_bytes = assistant_output_bytes(message), + assistant_tool_calls = message.tool_calls.len(), + "AI review completion metadata received" + ); if has_submit_review_findings(message) || !use_tool_calls { if *tool_calls_used > 0 { info!( @@ -2070,6 +2081,20 @@ fn tool_call_arguments(message: &OpenAiMessage) -> AppResult<&str> { }) } +fn assistant_output_bytes(message: &OpenAiMessage) -> usize { + message + .content + .as_deref() + .map_or(0, str::len) + .saturating_add( + message + .tool_calls + .iter() + .map(|tool_call| tool_call.function.arguments.len()) + .sum::(), + ) +} + fn parse_severity(value: &str) -> Severity { match value.trim().to_ascii_lowercase().as_str() { "error" => Severity::Error, @@ -2353,6 +2378,40 @@ mod tests { assert_eq!(findings[0].message, "Avoid unwrap here."); } + #[test] + fn parses_openai_completion_metadata_when_present() { + let response: OpenAiChatResponse = serde_json::from_str( + r#"{ + "choices":[{ + "finish_reason":"length", + "message":{"content":"{\"findings\":[]}"} + }], + "usage":{ + "prompt_tokens":120, + "completion_tokens":80, + "total_tokens":200 + } + }"#, + ) + .unwrap(); + + assert_eq!(response.choices[0].finish_reason.as_deref(), Some("length")); + let usage = response.usage.unwrap(); + assert_eq!(usage.prompt_tokens, Some(120)); + assert_eq!(usage.completion_tokens, Some(80)); + assert_eq!(usage.total_tokens, Some(200)); + } + + #[test] + fn parses_openai_response_without_completion_metadata() { + let response: OpenAiChatResponse = + serde_json::from_str(r#"{"choices":[{"message":{"content":"{\"findings\":[]}"}}]}"#) + .unwrap(); + + assert_eq!(response.choices[0].finish_reason, None); + assert!(response.usage.is_none()); + } + #[test] fn parses_findings_from_assistant_content_with_explanatory_prefix() { let response = r#" diff --git a/src/review/ai_schema.rs b/src/review/ai_schema.rs index 67c0bd3..cbb5577 100644 --- a/src/review/ai_schema.rs +++ b/src/review/ai_schema.rs @@ -59,11 +59,25 @@ pub(crate) struct ChatMessage { #[derive(Deserialize)] pub(crate) struct OpenAiChatResponse { pub(crate) choices: Vec, + #[serde(default)] + pub(crate) usage: Option, } #[derive(Deserialize)] pub(crate) struct OpenAiChoice { pub(crate) message: OpenAiMessage, + #[serde(default)] + pub(crate) finish_reason: Option, +} + +#[derive(Deserialize)] +pub(crate) struct OpenAiUsage { + #[serde(default)] + pub(crate) prompt_tokens: Option, + #[serde(default)] + pub(crate) completion_tokens: Option, + #[serde(default)] + pub(crate) total_tokens: Option, } #[derive(Deserialize)] From eea2e6d27aea5978fdb7ad8e8409e725fba7d772 Mon Sep 17 00:00:00 2001 From: SwartzMss Date: Thu, 23 Jul 2026 19:59:43 +0800 Subject: [PATCH 04/19] fix: retry malformed AI findings once --- src/review/ai.rs | 607 +++++++++++++++++++++++++++++------------------ 1 file changed, 371 insertions(+), 236 deletions(-) diff --git a/src/review/ai.rs b/src/review/ai.rs index f31bc6c..d29e3fc 100644 --- a/src/review/ai.rs +++ b/src/review/ai.rs @@ -39,6 +39,7 @@ const TIMEOUT_EVIDENCE_ITEM_CHARS: usize = 1500; const MIN_CONTEXT_TOOL_RESULT_BYTES: usize = 28; const FINALIZATION_INSTRUCTION: &str = "上下文工具预算已用尽。放弃任何仍缺少证据的候选问题，不得继续请求 read_file、search_code 或 list_files。请立即提交最终审查结果并调用 submit_review_findings；没有已确认问题时提交空 findings。"; const DIFF_ONLY_FINALIZATION_INSTRUCTION: &str = "上下文工具已关闭。请只基于原始 diff 和已明确提供的信息完成审查，不得继续请求 read_file、search_code 或 list_files。放弃任何仍缺少证据的候选问题，并立即调用 submit_review_findings；没有已确认问题时提交空 findings。"; +const MALFORMED_FINALIZATION_INSTRUCTION: &str = "上一次最终审查结果不是完整 JSON。不得继续请求 read_file、search_code 或 list_files。请压缩每条问题描述，放弃证据不足的问题，并立即重新提交一次完整 JSON；可用 submit_review_findings 时必须调用它，没有已确认问题时提交空 findings。"; #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub enum AiReviewExecutionMode { @@ -881,6 +882,7 @@ async fn complete_ai_review_response(context: AiReviewCompletion<'_>) -> AppResu let mut tool_result_bytes_used = 0_usize; let mut finalization_requested = false; let mut diff_only_fallback_requested = false; + let mut malformed_finalization_retry_requested = false; let base_message_count = messages.len(); loop { let response: OpenAiChatResponse = serde_json::from_str(&body).map_err(|err| { @@ -922,282 +924,310 @@ async fn complete_ai_review_response(context: AiReviewCompletion<'_>) -> AppResu "AI review context tool calls completed" ); } - return parse_openai_message(&config.id, &config.title, message); - } - - let context_tool_calls: Vec = message - .tool_calls - .iter() - .filter(|tool_call| is_context_tool_call(tool_call)) - .cloned() - .collect(); - if context_tool_calls.is_empty() { - return parse_openai_message(&config.id, &config.title, message); - } - if finalization_requested { - if diff_only_fallback_requested { - return Err(AppError::ai_review( - ReviewErrorCode::AiRequestFailed, - format!( - "AI review {} failed to submit findings after diff-only finalization fallback and requested another context tool", - config.id - ), - )); - } - diff_only_fallback_requested = true; - warn!( - ai_review_id = %config.id, - model = %config.model, - requested_tool_calls = context_tool_calls.len(), - batch_index = batch.map(|(index, _)| index), - batch_count = batch.map(|(_, count)| count), - "AI review requested context tools after finalization; retrying with diff-only finalization" - ); - request_diff_only_finalization(messages, base_message_count); - } else if !tool_context.source_available() { - if unavailable_context_tool_notice_sent { - return Err(AppError::ai_review( - ReviewErrorCode::AiResponseParseFailed, - format!( - "AI review {} repeatedly requested unavailable context tools", - config.id - ), - )); + let payload = final_findings_payload(message)?; + match parse_openai_findings_payload(&config.id, &config.title, payload) { + Ok(findings) => return Ok(findings), + Err(err) if !malformed_finalization_retry_requested => { + malformed_finalization_retry_requested = true; + finalization_requested = true; + warn!( + ai_review_id = %config.id, + model = %config.model, + attempt, + batch_index = batch.map(|(index, _)| index), + batch_count = batch.map(|(_, count)| count), + finish_reason = ?choice.finish_reason, + assistant_output_bytes = assistant_output_bytes(message), + error = %err, + "AI review final findings payload was malformed; retrying finalization once" + ); + messages.push(ChatMessage { + role: "user".into(), + content: Some(MALFORMED_FINALIZATION_INSTRUCTION.into()), + tool_call_id: None, + tool_calls: None, + }); + } + Err(err) => return Err(err), } - unavailable_context_tool_notice_sent = true; - warn!( - ai_review_id = %config.id, - model = %config.model, - requested_tool_calls = context_tool_calls.len(), - batch_index = batch.map(|(index, _)| index), - batch_count = batch.map(|(_, count)| count), - "AI review requested unavailable context tools; requesting final findings" - ); - messages.push(ChatMessage { - role: "user".into(), - content: Some( - "Context tools are unavailable for this diff-only review. Do not call read_file, search_code, or list_files. Submit final findings now using submit_review_findings." - .into(), - ), - tool_call_id: None, - tool_calls: None, - }); } else { - *tool_rounds_used += 1; - let tool_call_names = context_tool_calls + let context_tool_calls: Vec = message + .tool_calls .iter() - .map(|tool_call| tool_call.function.name.as_str()) - .collect::>() - .join(","); - let unlimited_tool_rounds = config.max_tool_rounds == 0; - let tool_round_budget_exhausted = - !unlimited_tool_rounds && *tool_rounds_used >= config.max_tool_rounds; - info!( - ai_review_id = %config.id, - model = %config.model, - attempt, - tool_rounds_used = *tool_rounds_used, - max_tool_rounds = config.max_tool_rounds, - requested_tool_calls = context_tool_calls.len(), - tool_call_names = %tool_call_names, - tool_calls_used = *tool_calls_used, - max_tool_calls = config.max_tool_calls, - batch_index = batch.map(|(index, _)| index), - batch_count = batch.map(|(_, count)| count), - "AI review context tool calls requested" - ); - send_ai_tool_progress( - &progress, - config, - batch, - *tool_rounds_used, - *tool_calls_used, - ); - let unlimited_tool_calls = config.max_tool_calls == 0; - let remaining_tool_calls = if unlimited_tool_calls { - usize::MAX - } else { - config.max_tool_calls.saturating_sub(*tool_calls_used) - }; - if !unlimited_tool_calls && remaining_tool_calls == 0 && tool_call_limit_notice_sent { + .filter(|tool_call| is_context_tool_call(tool_call)) + .cloned() + .collect(); + if context_tool_calls.is_empty() { + return parse_openai_message(&config.id, &config.title, message); + } + if finalization_requested { + if diff_only_fallback_requested { + return Err(AppError::ai_review( + ReviewErrorCode::AiRequestFailed, + format!( + "AI review {} failed to submit findings after diff-only finalization fallback and requested another context tool", + config.id + ), + )); + } + diff_only_fallback_requested = true; warn!( ai_review_id = %config.id, model = %config.model, requested_tool_calls = context_tool_calls.len(), - tool_calls_used = *tool_calls_used, - max_tool_calls = config.max_tool_calls, batch_index = batch.map(|(index, _)| index), batch_count = batch.map(|(_, count)| count), - "AI review context tool call limit already reported" + "AI review requested context tools after finalization; retrying with diff-only finalization" ); - return Err(AppError::ai_review( - ReviewErrorCode::AiRequestFailed, - format!( - "AI review {} exhausted context tool calls before submitting findings", - config.id - ), - )); - } - if !unlimited_tool_calls && context_tool_calls.len() > remaining_tool_calls { + request_diff_only_finalization(messages, base_message_count); + } else if !tool_context.source_available() { + if unavailable_context_tool_notice_sent { + return Err(AppError::ai_review( + ReviewErrorCode::AiResponseParseFailed, + format!( + "AI review {} repeatedly requested unavailable context tools", + config.id + ), + )); + } + unavailable_context_tool_notice_sent = true; warn!( ai_review_id = %config.id, model = %config.model, requested_tool_calls = context_tool_calls.len(), - remaining_tool_calls, + batch_index = batch.map(|(index, _)| index), + batch_count = batch.map(|(_, count)| count), + "AI review requested unavailable context tools; requesting final findings" + ); + messages.push(ChatMessage { + role: "user".into(), + content: Some( + "Context tools are unavailable for this diff-only review. Do not call read_file, search_code, or list_files. Submit final findings now using submit_review_findings." + .into(), + ), + tool_call_id: None, + tool_calls: None, + }); + } else { + *tool_rounds_used += 1; + let tool_call_names = context_tool_calls + .iter() + .map(|tool_call| tool_call.function.name.as_str()) + .collect::>() + .join(","); + let unlimited_tool_rounds = config.max_tool_rounds == 0; + let tool_round_budget_exhausted = + !unlimited_tool_rounds && *tool_rounds_used >= config.max_tool_rounds; + info!( + ai_review_id = %config.id, + model = %config.model, + attempt, + tool_rounds_used = *tool_rounds_used, + max_tool_rounds = config.max_tool_rounds, + requested_tool_calls = context_tool_calls.len(), + tool_call_names = %tool_call_names, tool_calls_used = *tool_calls_used, max_tool_calls = config.max_tool_calls, batch_index = batch.map(|(index, _)| index), batch_count = batch.map(|(_, count)| count), - "AI review context tool call limit reached" + "AI review context tool calls requested" ); - } - let context_tool_calls = synthesize_context_tool_call_ids(context_tool_calls); - - messages.push(ChatMessage { - role: "assistant".into(), - content: message.content.clone(), - tool_call_id: None, - tool_calls: Some(context_tool_calls.clone()), - }); - let mut real_calls_in_response = 0_usize; - let mut tool_byte_limit_reached = false; - let mut cache_hit_requires_finalization = false; - for tool_call in context_tool_calls { - let tool_call_id = non_empty_tool_call_id(&tool_call); - let tool_name = tool_call.function.name.as_str(); - let arguments_summary = - tool_call_argument_summary(tool_name, &tool_call.function.arguments); - let cache_key = context_tool_cache_key(tool_name, &tool_call.function.arguments); - let remaining_tool_bytes = if config.max_tool_total_bytes == 0 { + send_ai_tool_progress( + &progress, + config, + batch, + *tool_rounds_used, + *tool_calls_used, + ); + let unlimited_tool_calls = config.max_tool_calls == 0; + let remaining_tool_calls = if unlimited_tool_calls { usize::MAX } else { - config - .max_tool_total_bytes - .saturating_sub(tool_result_bytes_used) + config.max_tool_calls.saturating_sub(*tool_calls_used) }; - let result = if tool_cache.contains(&cache_key) { - cache_hit_requires_finalization = true; - let result = cached_context_tool_result(); - info!( - ai_review_id = %config.id, - model = %config.model, - tool_name, - tool_call_id = %tool_call_id, - arguments_summary = %arguments_summary, - cache_hit = true, - tool_calls_used = *tool_calls_used, - tool_result_bytes_used, - max_tool_total_bytes = config.max_tool_total_bytes, - batch_index = batch.map(|(index, _)| index), - batch_count = batch.map(|(_, count)| count), - "AI review context tool result reused" - ); - result - } else if real_calls_in_response < remaining_tool_calls - && (config.max_tool_total_bytes == 0 - || remaining_tool_bytes >= MIN_CONTEXT_TOOL_RESULT_BYTES) + if !unlimited_tool_calls && remaining_tool_calls == 0 && tool_call_limit_notice_sent { - let result_limit = config.max_tool_result_bytes.min(remaining_tool_bytes); - let result = tool_context.call_with_result_limit(&tool_call, result_limit); - *tool_calls_used += 1; - real_calls_in_response += 1; - tool_result_bytes_used = tool_result_bytes_used.saturating_add(result.len()); - tool_cache.insert(cache_key); - info!( - ai_review_id = %config.id, - model = %config.model, - tool_name, - tool_call_id = %tool_call_id, - arguments_summary = %arguments_summary, - result_bytes = tool_result_bytes(&result), - result_truncated = tool_result_truncated(&result), - result_limit_reached = tool_result_limit_reached(&result, config.max_tool_result_bytes), - tool_call_limit_reached = false, - tool_calls_used = *tool_calls_used, - total_tool_calls_used = *tool_calls_used, - max_tool_calls = config.max_tool_calls, - cache_hit = false, - tool_result_bytes_used, - max_tool_total_bytes = config.max_tool_total_bytes, - batch_index = batch.map(|(index, _)| index), - batch_count = batch.map(|(_, count)| count), - "AI review context tool result returned" - ); - send_ai_tool_progress( - &progress, - config, - batch, - *tool_rounds_used, - *tool_calls_used, - ); - result - } else if real_calls_in_response >= remaining_tool_calls { - tool_call_limit_notice_sent = true; - let result = context_tool_call_limit_result(config); warn!( ai_review_id = %config.id, model = %config.model, - tool_name, - tool_call_id = %tool_call_id, - arguments_summary = %arguments_summary, - result_bytes = tool_result_bytes(&result), - result_truncated = tool_result_truncated(&result), - result_limit_reached = tool_result_limit_reached(&result, config.max_tool_result_bytes), - tool_call_limit_reached = true, + requested_tool_calls = context_tool_calls.len(), tool_calls_used = *tool_calls_used, - total_tool_calls_used = *tool_calls_used, max_tool_calls = config.max_tool_calls, batch_index = batch.map(|(index, _)| index), batch_count = batch.map(|(_, count)| count), - "AI review context tool call skipped because limit was reached" + "AI review context tool call limit already reported" ); - result - } else { - tool_byte_limit_reached = true; - let result = context_tool_result_byte_limit_result(config); + return Err(AppError::ai_review( + ReviewErrorCode::AiRequestFailed, + format!( + "AI review {} exhausted context tool calls before submitting findings", + config.id + ), + )); + } + if !unlimited_tool_calls && context_tool_calls.len() > remaining_tool_calls { warn!( ai_review_id = %config.id, model = %config.model, - tool_name, - tool_call_id = %tool_call_id, - arguments_summary = %arguments_summary, - result_bytes = tool_result_bytes(&result), + requested_tool_calls = context_tool_calls.len(), + remaining_tool_calls, tool_calls_used = *tool_calls_used, max_tool_calls = config.max_tool_calls, - tool_result_bytes_used, - max_tool_total_bytes = config.max_tool_total_bytes, batch_index = batch.map(|(index, _)| index), batch_count = batch.map(|(_, count)| count), - "AI review context tool call skipped because result byte limit was reached" + "AI review context tool call limit reached" ); - result - }; + } + let context_tool_calls = synthesize_context_tool_call_ids(context_tool_calls); + messages.push(ChatMessage { - role: "tool".into(), - content: Some(result), - tool_call_id: Some(tool_call_id), - tool_calls: None, + role: "assistant".into(), + content: message.content.clone(), + tool_call_id: None, + tool_calls: Some(context_tool_calls.clone()), }); - } - let tool_call_budget_exhausted = - config.max_tool_calls != 0 && *tool_calls_used >= config.max_tool_calls; - let tool_byte_budget_exhausted = config.max_tool_total_bytes != 0 - && (tool_result_bytes_used >= config.max_tool_total_bytes - || tool_byte_limit_reached); - if tool_call_budget_exhausted - || tool_round_budget_exhausted - || tool_byte_budget_exhausted - || cache_hit_requires_finalization - { - request_finalization(messages, &mut finalization_requested); + let mut real_calls_in_response = 0_usize; + let mut tool_byte_limit_reached = false; + let mut cache_hit_requires_finalization = false; + for tool_call in context_tool_calls { + let tool_call_id = non_empty_tool_call_id(&tool_call); + let tool_name = tool_call.function.name.as_str(); + let arguments_summary = + tool_call_argument_summary(tool_name, &tool_call.function.arguments); + let cache_key = + context_tool_cache_key(tool_name, &tool_call.function.arguments); + let remaining_tool_bytes = if config.max_tool_total_bytes == 0 { + usize::MAX + } else { + config + .max_tool_total_bytes + .saturating_sub(tool_result_bytes_used) + }; + let result = if tool_cache.contains(&cache_key) { + cache_hit_requires_finalization = true; + let result = cached_context_tool_result(); + info!( + ai_review_id = %config.id, + model = %config.model, + tool_name, + tool_call_id = %tool_call_id, + arguments_summary = %arguments_summary, + cache_hit = true, + tool_calls_used = *tool_calls_used, + tool_result_bytes_used, + max_tool_total_bytes = config.max_tool_total_bytes, + batch_index = batch.map(|(index, _)| index), + batch_count = batch.map(|(_, count)| count), + "AI review context tool result reused" + ); + result + } else if real_calls_in_response < remaining_tool_calls + && (config.max_tool_total_bytes == 0 + || remaining_tool_bytes >= MIN_CONTEXT_TOOL_RESULT_BYTES) + { + let result_limit = config.max_tool_result_bytes.min(remaining_tool_bytes); + let result = tool_context.call_with_result_limit(&tool_call, result_limit); + *tool_calls_used += 1; + real_calls_in_response += 1; + tool_result_bytes_used = + tool_result_bytes_used.saturating_add(result.len()); + tool_cache.insert(cache_key); + info!( + ai_review_id = %config.id, + model = %config.model, + tool_name, + tool_call_id = %tool_call_id, + arguments_summary = %arguments_summary, + result_bytes = tool_result_bytes(&result), + result_truncated = tool_result_truncated(&result), + result_limit_reached = tool_result_limit_reached(&result, config.max_tool_result_bytes), + tool_call_limit_reached = false, + tool_calls_used = *tool_calls_used, + total_tool_calls_used = *tool_calls_used, + max_tool_calls = config.max_tool_calls, + cache_hit = false, + tool_result_bytes_used, + max_tool_total_bytes = config.max_tool_total_bytes, + batch_index = batch.map(|(index, _)| index), + batch_count = batch.map(|(_, count)| count), + "AI review context tool result returned" + ); + send_ai_tool_progress( + &progress, + config, + batch, + *tool_rounds_used, + *tool_calls_used, + ); + result + } else if real_calls_in_response >= remaining_tool_calls { + tool_call_limit_notice_sent = true; + let result = context_tool_call_limit_result(config); + warn!( + ai_review_id = %config.id, + model = %config.model, + tool_name, + tool_call_id = %tool_call_id, + arguments_summary = %arguments_summary, + result_bytes = tool_result_bytes(&result), + result_truncated = tool_result_truncated(&result), + result_limit_reached = tool_result_limit_reached(&result, config.max_tool_result_bytes), + tool_call_limit_reached = true, + tool_calls_used = *tool_calls_used, + total_tool_calls_used = *tool_calls_used, + max_tool_calls = config.max_tool_calls, + batch_index = batch.map(|(index, _)| index), + batch_count = batch.map(|(_, count)| count), + "AI review context tool call skipped because limit was reached" + ); + result + } else { + tool_byte_limit_reached = true; + let result = context_tool_result_byte_limit_result(config); + warn!( + ai_review_id = %config.id, + model = %config.model, + tool_name, + tool_call_id = %tool_call_id, + arguments_summary = %arguments_summary, + result_bytes = tool_result_bytes(&result), + tool_calls_used = *tool_calls_used, + max_tool_calls = config.max_tool_calls, + tool_result_bytes_used, + max_tool_total_bytes = config.max_tool_total_bytes, + batch_index = batch.map(|(index, _)| index), + batch_count = batch.map(|(_, count)| count), + "AI review context tool call skipped because result byte limit was reached" + ); + result + }; + messages.push(ChatMessage { + role: "tool".into(), + content: Some(result), + tool_call_id: Some(tool_call_id), + tool_calls: None, + }); + } + let tool_call_budget_exhausted = + config.max_tool_calls != 0 && *tool_calls_used >= config.max_tool_calls; + let tool_byte_budget_exhausted = config.max_tool_total_bytes != 0 + && (tool_result_bytes_used >= config.max_tool_total_bytes + || tool_byte_limit_reached); + if tool_call_budget_exhausted + || tool_round_budget_exhausted + || tool_byte_budget_exhausted + || cache_hit_requires_finalization + { + request_finalization(messages, &mut finalization_requested); + } } } let mut request_body = serialize_review_request_body( config, messages, - true, - tool_context.source_available() && !finalization_requested, + use_tool_calls, + use_tool_calls && tool_context.source_available() && !finalization_requested, )?; let mut last_error = None; let mut response = None; @@ -1251,8 +1281,12 @@ async fn complete_ai_review_response(context: AiReviewCompletion<'_>) -> AppResu base_message_count, &mut finalization_requested, ); - request_body = - serialize_review_request_body(config, messages, true, false)?; + request_body = serialize_review_request_body( + config, + messages, + use_tool_calls, + false, + )?; } last_error = Some(err); continue; @@ -1274,8 +1308,12 @@ async fn complete_ai_review_response(context: AiReviewCompletion<'_>) -> AppResu base_message_count, &mut finalization_requested, ); - request_body = - serialize_review_request_body(config, messages, true, false)?; + request_body = serialize_review_request_body( + config, + messages, + use_tool_calls, + false, + )?; } warn!( ai_review_id = %config.id, @@ -1306,7 +1344,7 @@ async fn complete_ai_review_response(context: AiReviewCompletion<'_>) -> AppResu &mut finalization_requested, ); request_body = - serialize_review_request_body(config, messages, true, false)?; + serialize_review_request_body(config, messages, use_tool_calls, false)?; warn!( ai_review_id = %config.id, model = %config.model, @@ -1976,7 +2014,12 @@ fn parse_openai_message( title: &str, message: &OpenAiMessage, ) -> AppResult> { - let content = tool_call_arguments(message).or_else(|_| { + let content = final_findings_payload(message)?; + parse_openai_findings_payload(review_id, title, content) +} + +fn final_findings_payload(message: &OpenAiMessage) -> AppResult<&str> { + tool_call_arguments(message).or_else(|_| { message .content .as_deref() @@ -1988,7 +2031,14 @@ fn parse_openai_message( "AI review API returned no content", ) }) - })?; + }) +} + +fn parse_openai_findings_payload( + review_id: &str, + title: &str, + content: &str, +) -> AppResult> { info!( ai_review_id = %review_id, assistant_content_bytes = content.len(), @@ -3129,6 +3179,91 @@ mod tests { assert_eq!(client_one, client_two); } + #[tokio::test] + async fn retries_malformed_submit_findings_once_without_replaying_it() { + let request_count = Arc::new(AtomicUsize::new(0)); + let malformed_call_leaked = Arc::new(AtomicUsize::new(0)); + let finalization_only_count = Arc::new(AtomicUsize::new(0)); + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap(); + let request_count_for_server = Arc::clone(&request_count); + let malformed_call_leaked_for_server = Arc::clone(&malformed_call_leaked); + let finalization_only_count_for_server = Arc::clone(&finalization_only_count); + tokio::spawn(async move { + loop { + let (mut stream, _) = listener.accept().await.unwrap(); + let request_count = Arc::clone(&request_count_for_server); + let malformed_call_leaked = Arc::clone(&malformed_call_leaked_for_server); + let finalization_only_count = Arc::clone(&finalization_only_count_for_server); + tokio::spawn(async move { + let request_index = request_count.fetch_add(1, Ordering::SeqCst) + 1; + let request = read_http_json_request(&mut stream).await; + let body = match request_index { + 1 => { + r#"{"choices":[{"finish_reason":"length","message":{"tool_calls":[{"id":"submit_bad","type":"function","function":{"name":"submit_review_findings","arguments":"{\"findings\":["}}]}}],"usage":{"prompt_tokens":120,"completion_tokens":80,"total_tokens":200}}"# + } + 2 => { + let tool_names = request["tools"] + .as_array() + .unwrap() + .iter() + .filter_map(|tool| tool["function"]["name"].as_str()) + .collect::>(); + if tool_names == ["submit_review_findings"] { + finalization_only_count.fetch_add(1, Ordering::SeqCst); + } + let messages = request["messages"].as_array().unwrap(); + let leaked = messages + .iter() + .filter_map(|message| message["tool_calls"].as_array()) + .flatten() + .filter(|call| call["id"] == "submit_bad") + .count(); + malformed_call_leaked.store(leaked, Ordering::SeqCst); + assert!(messages.iter().any(|message| { + message["content"].as_str().is_some_and(|content| { + content.contains("JSON") && content.contains("完整") + }) + })); + r#"{"choices":[{"finish_reason":"tool_calls","message":{"tool_calls":[{"id":"submit_ok","type":"function","function":{"name":"submit_review_findings","arguments":"{\"findings\":[{\"path\":\"src/lib.rs\",\"line\":1,\"severity\":\"error\",\"title\":\"Possible panic\",\"message\":\"Avoid panic here.\"}]}"}}]}}]}"# + } + other => panic!("unexpected request {other}"), + }; + let response = format!( + "HTTP/1.1 200 OK\r\ncontent-type: application/json\r\ncontent-length: {}\r\n\r\n", + body.len() + ); + stream.write_all(response.as_bytes()).await.unwrap(); + stream.write_all(body.as_bytes()).await.unwrap(); + }); + } + }); + + let config = AiReviewConfig { + base_url: format!("http://{addr}"), + ..test_ai_review_config() + }; + let changes = vec![GitLabChange { + old_path: "src/lib.rs".into(), + new_path: "src/lib.rs".into(), + new_file: false, + renamed_file: false, + deleted_file: false, + diff: "@@ -1,0 +1,1 @@\n+panic!();\n".into(), + }]; + + let findings = run_ai_review_execution_with_context(&config, &changes, None, None) + .await + .result + .unwrap(); + + assert_eq!(findings.len(), 1); + assert_eq!(findings[0].path, "src/lib.rs"); + assert_eq!(request_count.load(Ordering::SeqCst), 2); + assert_eq!(malformed_call_leaked.load(Ordering::SeqCst), 0); + assert_eq!(finalization_only_count.load(Ordering::SeqCst), 1); + } + #[tokio::test] async fn returns_tool_limit_result_before_requiring_final_findings() { let request_count = Arc::new(AtomicUsize::new(0)); From 286d60b010cc715aad97bf71fadbfa76213c40d2 Mon Sep 17 00:00:00 2001 From: SwartzMss Date: Thu, 23 Jul 2026 20:00:49 +0800 Subject: [PATCH 05/19] test: cover malformed findings retry bounds --- src/review/ai.rs | 123 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 123 insertions(+) diff --git a/src/review/ai.rs b/src/review/ai.rs index d29e3fc..004c110 100644 --- a/src/review/ai.rs +++ b/src/review/ai.rs @@ -3264,6 +3264,129 @@ mod tests { assert_eq!(finalization_only_count.load(Ordering::SeqCst), 1); } + #[tokio::test] + async fn stops_after_one_malformed_findings_retry() { + let request_count = Arc::new(AtomicUsize::new(0)); + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap(); + let request_count_for_server = Arc::clone(&request_count); + tokio::spawn(async move { + loop { + let (mut stream, _) = listener.accept().await.unwrap(); + let request_count = Arc::clone(&request_count_for_server); + tokio::spawn(async move { + request_count.fetch_add(1, Ordering::SeqCst); + let _request = read_http_json_request(&mut stream).await; + let body = r#"{"choices":[{"finish_reason":"length","message":{"tool_calls":[{"id":"submit_bad","type":"function","function":{"name":"submit_review_findings","arguments":"{\"findings\":["}}]}}]}"#; + let response = format!( + "HTTP/1.1 200 OK\r\ncontent-type: application/json\r\ncontent-length: {}\r\n\r\n", + body.len() + ); + stream.write_all(response.as_bytes()).await.unwrap(); + stream.write_all(body.as_bytes()).await.unwrap(); + }); + } + }); + + let config = AiReviewConfig { + base_url: format!("http://{addr}"), + ..test_ai_review_config() + }; + let changes = vec![GitLabChange { + old_path: "src/lib.rs".into(), + new_path: "src/lib.rs".into(), + new_file: false, + renamed_file: false, + deleted_file: false, + diff: "@@ -1,0 +1,1 @@\n+panic!();\n".into(), + }]; + + let execution = run_ai_review_execution_with_context(&config, &changes, None, None).await; + + assert_eq!( + execution + .result + .unwrap_err() + .review_failure() + .map(|failure| failure.code), + Some(ReviewErrorCode::AiResponseParseFailed) + ); + assert_eq!(request_count.load(Ordering::SeqCst), 2); + } + + #[tokio::test] + async fn retries_malformed_json_content_in_json_mode() { + let request_count = Arc::new(AtomicUsize::new(0)); + let json_retry_count = Arc::new(AtomicUsize::new(0)); + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap(); + let request_count_for_server = Arc::clone(&request_count); + let json_retry_count_for_server = Arc::clone(&json_retry_count); + tokio::spawn(async move { + loop { + let (mut stream, _) = listener.accept().await.unwrap(); + let request_count = Arc::clone(&request_count_for_server); + let json_retry_count = Arc::clone(&json_retry_count_for_server); + tokio::spawn(async move { + let request_index = request_count.fetch_add(1, Ordering::SeqCst) + 1; + let request = read_http_json_request(&mut stream).await; + let (status, body) = match request_index { + 1 => ("400 Bad Request", r#"{"error":"tools unsupported"}"#), + 2 => ( + "200 OK", + r#"{"choices":[{"finish_reason":"length","message":{"content":"{\"findings\":["}}]}"#, + ), + 3 => { + assert_eq!(request["response_format"]["type"], "json_object"); + assert!(request.get("tools").is_none()); + assert!(request["messages"].as_array().unwrap().iter().any( + |message| { + message["content"].as_str().is_some_and(|content| { + content.contains("JSON") && content.contains("完整") + }) + } + )); + json_retry_count.fetch_add(1, Ordering::SeqCst); + ( + "200 OK", + r#"{"choices":[{"finish_reason":"stop","message":{"content":"{\"findings\":[]}"}}]}"#, + ) + } + other => panic!("unexpected request {other}"), + }; + let response = format!( + "HTTP/1.1 {status}\r\ncontent-type: application/json\r\ncontent-length: {}\r\n\r\n", + body.len() + ); + stream.write_all(response.as_bytes()).await.unwrap(); + stream.write_all(body.as_bytes()).await.unwrap(); + }); + } + }); + + let config = AiReviewConfig { + base_url: format!("http://{addr}"), + ..test_ai_review_config() + }; + let changes = vec![GitLabChange { + old_path: "src/lib.rs".into(), + new_path: "src/lib.rs".into(), + new_file: false, + renamed_file: false, + deleted_file: false, + diff: "@@ -1,0 +1,1 @@\n+panic!();\n".into(), + }]; + + let findings = run_ai_review_execution_with_context(&config, &changes, None, None) + .await + .result + .unwrap(); + + assert!(findings.is_empty()); + assert_eq!(request_count.load(Ordering::SeqCst), 3); + assert_eq!(json_retry_count.load(Ordering::SeqCst), 1); + } + #[tokio::test] async fn returns_tool_limit_result_before_requiring_final_findings() { let request_count = Arc::new(AtomicUsize::new(0)); From 62baa2e9c16f82c1b4d8a40ecde51eaff6bcf5a7 Mon Sep 17 00:00:00 2001 From: SwartzMss Date: Thu, 23 Jul 2026 22:22:21 +0800 Subject: [PATCH 06/19] fix: retry malformed content fallback with tools --- src/review/ai.rs | 94 ++++++++++++++++++++++++++++++++++++++++----- tests/e2e_review.rs | 4 +- 2 files changed, 86 insertions(+), 12 deletions(-) diff --git a/src/review/ai.rs b/src/review/ai.rs index 004c110..55e1f72 100644 --- a/src/review/ai.rs +++ b/src/review/ai.rs @@ -909,7 +909,15 @@ async fn complete_ai_review_response(context: AiReviewCompletion<'_>) -> AppResu assistant_tool_calls = message.tool_calls.len(), "AI review completion metadata received" ); - if has_submit_review_findings(message) || !use_tool_calls { + let context_tool_calls: Vec = message + .tool_calls + .iter() + .filter(|tool_call| is_context_tool_call(tool_call)) + .cloned() + .collect(); + let is_final_response = + has_submit_review_findings(message) || !use_tool_calls || context_tool_calls.is_empty(); + if is_final_response { if *tool_calls_used > 0 { info!( ai_review_id = %config.id, @@ -951,15 +959,6 @@ async fn complete_ai_review_response(context: AiReviewCompletion<'_>) -> AppResu Err(err) => return Err(err), } } else { - let context_tool_calls: Vec = message - .tool_calls - .iter() - .filter(|tool_call| is_context_tool_call(tool_call)) - .cloned() - .collect(); - if context_tool_calls.is_empty() { - return parse_openai_message(&config.id, &config.title, message); - } if finalization_requested { if diff_only_fallback_requested { return Err(AppError::ai_review( @@ -2009,6 +2008,7 @@ fn parse_openai_response(review_id: &str, title: &str, text: &str) -> AppResult< parse_openai_message(review_id, title, message) } +#[cfg(test)] fn parse_openai_message( review_id: &str, title: &str, @@ -3387,6 +3387,80 @@ mod tests { assert_eq!(json_retry_count.load(Ordering::SeqCst), 1); } + #[tokio::test] + async fn retries_malformed_json_content_when_tools_are_accepted() { + let request_count = Arc::new(AtomicUsize::new(0)); + let finalization_only_count = Arc::new(AtomicUsize::new(0)); + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap(); + let request_count_for_server = Arc::clone(&request_count); + let finalization_only_count_for_server = Arc::clone(&finalization_only_count); + tokio::spawn(async move { + loop { + let (mut stream, _) = listener.accept().await.unwrap(); + let request_count = Arc::clone(&request_count_for_server); + let finalization_only_count = Arc::clone(&finalization_only_count_for_server); + tokio::spawn(async move { + let request_index = request_count.fetch_add(1, Ordering::SeqCst) + 1; + let request = read_http_json_request(&mut stream).await; + let body = match request_index { + 1 => { + assert!(request.get("tools").is_some()); + r#"{"choices":[{"finish_reason":"length","message":{"content":"{\"findings\":["}}]}"# + } + 2 => { + let tool_names = request["tools"] + .as_array() + .unwrap() + .iter() + .filter_map(|tool| tool["function"]["name"].as_str()) + .collect::>(); + assert_eq!(tool_names, ["submit_review_findings"]); + assert!(request["messages"].as_array().unwrap().iter().any( + |message| { + message["content"].as_str().is_some_and(|content| { + content.contains("JSON") && content.contains("完整") + }) + } + )); + finalization_only_count.fetch_add(1, Ordering::SeqCst); + r#"{"choices":[{"finish_reason":"tool_calls","message":{"tool_calls":[{"id":"submit_ok","type":"function","function":{"name":"submit_review_findings","arguments":"{\"findings\":[]}"}}]}}]}"# + } + other => panic!("unexpected request {other}"), + }; + let response = format!( + "HTTP/1.1 200 OK\r\ncontent-type: application/json\r\ncontent-length: {}\r\n\r\n", + body.len() + ); + stream.write_all(response.as_bytes()).await.unwrap(); + stream.write_all(body.as_bytes()).await.unwrap(); + }); + } + }); + + let config = AiReviewConfig { + base_url: format!("http://{addr}"), + ..test_ai_review_config() + }; + let changes = vec![GitLabChange { + old_path: "src/lib.rs".into(), + new_path: "src/lib.rs".into(), + new_file: false, + renamed_file: false, + deleted_file: false, + diff: "@@ -1,0 +1,1 @@\n+panic!();\n".into(), + }]; + + let findings = run_ai_review_execution_with_context(&config, &changes, None, None) + .await + .result + .unwrap(); + + assert!(findings.is_empty()); + assert_eq!(request_count.load(Ordering::SeqCst), 2); + assert_eq!(finalization_only_count.load(Ordering::SeqCst), 1); + } + #[tokio::test] async fn returns_tool_limit_result_before_requiring_final_findings() { let request_count = Arc::new(AtomicUsize::new(0)); diff --git a/tests/e2e_review.rs b/tests/e2e_review.rs index 74e7747..a5f66c4 100644 --- a/tests/e2e_review.rs +++ b/tests/e2e_review.rs @@ -740,7 +740,7 @@ timeout_seconds = 10 assert_eq!(summary.findings, 0); assert_eq!(summary.comments, 1); assert!(!summary.skipped); - assert_eq!(ai_request_count.load(Ordering::SeqCst), 2); + assert_eq!(ai_request_count.load(Ordering::SeqCst), 3); assert_eq!(discussion_count.load(Ordering::SeqCst), 1); } @@ -2206,7 +2206,7 @@ timeout_seconds = 10 assert_eq!(summary.findings, 0); assert_eq!(summary.comments, 1); assert!(!summary.skipped); - assert_eq!(ai_request_count.load(Ordering::SeqCst), 2); + assert_eq!(ai_request_count.load(Ordering::SeqCst), 3); assert_eq!(discussion_count.load(Ordering::SeqCst), 1); assert_eq!(emoji_count.load(Ordering::SeqCst), 1); } From 2bee7bada0d5d5a79a763e3f706c9e96482b490c Mon Sep 17 00:00:00 2001 From: SwartzMss Date: Thu, 23 Jul 2026 22:45:42 +0800 Subject: [PATCH 07/19] fix: harden malformed findings recovery --- src/review/ai.rs | 272 ++++++++++++++++++++++++++++++++++++++-- src/review/ai_schema.rs | 14 +-- 2 files changed, 261 insertions(+), 25 deletions(-) diff --git a/src/review/ai.rs b/src/review/ai.rs index 55e1f72..2dff55e 100644 --- a/src/review/ai.rs +++ b/src/review/ai.rs @@ -40,6 +40,7 @@ const MIN_CONTEXT_TOOL_RESULT_BYTES: usize = 28; const FINALIZATION_INSTRUCTION: &str = "上下文工具预算已用尽。放弃任何仍缺少证据的候选问题，不得继续请求 read_file、search_code 或 list_files。请立即提交最终审查结果并调用 submit_review_findings；没有已确认问题时提交空 findings。"; const DIFF_ONLY_FINALIZATION_INSTRUCTION: &str = "上下文工具已关闭。请只基于原始 diff 和已明确提供的信息完成审查，不得继续请求 read_file、search_code 或 list_files。放弃任何仍缺少证据的候选问题，并立即调用 submit_review_findings；没有已确认问题时提交空 findings。"; const MALFORMED_FINALIZATION_INSTRUCTION: &str = "上一次最终审查结果不是完整 JSON。不得继续请求 read_file、search_code 或 list_files。请压缩每条问题描述，放弃证据不足的问题，并立即重新提交一次完整 JSON；可用 submit_review_findings 时必须调用它，没有已确认问题时提交空 findings。"; +const JSON_FINALIZATION_INSTRUCTION: &str = "请立即返回一个完整且精简的 JSON 对象，格式必须为 {\"findings\":[...]}。不得调用任何工具；没有已确认问题时返回 {\"findings\":[]}。"; #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub enum AiReviewExecutionMode { @@ -826,7 +827,11 @@ fn is_tool_call_rejection(status: u16, body: &str) -> bool { } fn is_retryable_ai_http_response(status: u16, body: &str) -> bool { - if matches!(status, 408 | 429 | 502 | 503 | 504) { + is_timeout_ai_http_response(status, body) || matches!(status, 429 | 502 | 503) +} + +fn is_timeout_ai_http_response(status: u16, body: &str) -> bool { + if matches!(status, 408 | 504) { return true; } if status < 500 { @@ -901,10 +906,10 @@ async fn complete_ai_review_response(context: AiReviewCompletion<'_>) -> AppResu attempt, batch_index = batch.map(|(index, _)| index), batch_count = batch.map(|(_, count)| count), - finish_reason = ?choice.finish_reason, - prompt_tokens = ?response.usage.as_ref().and_then(|usage| usage.prompt_tokens), - completion_tokens = ?response.usage.as_ref().and_then(|usage| usage.completion_tokens), - total_tokens = ?response.usage.as_ref().and_then(|usage| usage.total_tokens), + finish_reason = ?choice.finish_reason.as_ref().and_then(serde_json::Value::as_str), + prompt_tokens = ?telemetry_usage_token(response.usage.as_ref(), "prompt_tokens"), + completion_tokens = ?telemetry_usage_token(response.usage.as_ref(), "completion_tokens"), + total_tokens = ?telemetry_usage_token(response.usage.as_ref(), "total_tokens"), assistant_output_bytes = assistant_output_bytes(message), assistant_tool_calls = message.tool_calls.len(), "AI review completion metadata received" @@ -944,7 +949,7 @@ async fn complete_ai_review_response(context: AiReviewCompletion<'_>) -> AppResu attempt, batch_index = batch.map(|(index, _)| index), batch_count = batch.map(|(_, count)| count), - finish_reason = ?choice.finish_reason, + finish_reason = ?choice.finish_reason.as_ref().and_then(serde_json::Value::as_str), assistant_output_bytes = assistant_output_bytes(message), error = %err, "AI review final findings payload was malformed; retrying finalization once" @@ -960,6 +965,15 @@ async fn complete_ai_review_response(context: AiReviewCompletion<'_>) -> AppResu } } else { if finalization_requested { + if malformed_finalization_retry_requested { + return Err(AppError::ai_review( + ReviewErrorCode::AiResponseParseFailed, + format!( + "AI review {} requested context tools during malformed findings recovery", + config.id + ), + )); + } if diff_only_fallback_requested { return Err(AppError::ai_review( ReviewErrorCode::AiRequestFailed, @@ -1274,11 +1288,16 @@ async fn complete_ai_review_response(context: AiReviewCompletion<'_>) -> AppResu error = %err, "AI review context follow-up request failed, retrying" ); - if current_response.status == 504 { + if is_timeout_ai_http_response( + current_response.status, + &response_body_preview, + ) { request_timeout_finalization( messages, base_message_count, &mut finalization_requested, + use_tool_calls, + malformed_finalization_retry_requested, ); request_body = serialize_review_request_body( config, @@ -1306,6 +1325,8 @@ async fn complete_ai_review_response(context: AiReviewCompletion<'_>) -> AppResu messages, base_message_count, &mut finalization_requested, + use_tool_calls, + malformed_finalization_retry_requested, ); request_body = serialize_review_request_body( config, @@ -1341,6 +1362,8 @@ async fn complete_ai_review_response(context: AiReviewCompletion<'_>) -> AppResu messages, base_message_count, &mut finalization_requested, + use_tool_calls, + malformed_finalization_retry_requested, ); request_body = serialize_review_request_body(config, messages, use_tool_calls, false)?; @@ -1432,6 +1455,8 @@ fn request_timeout_finalization( messages: &mut Vec, base_message_count: usize, requested: &mut bool, + use_tool_calls: bool, + malformed_retry: bool, ) { let evidence_summary = compact_tool_evidence(messages, base_message_count); messages.truncate(base_message_count); @@ -1446,7 +1471,14 @@ fn request_timeout_finalization( *requested = true; messages.push(ChatMessage { role: "user".into(), - content: Some(FINALIZATION_INSTRUCTION.into()), + content: Some( + match (use_tool_calls, malformed_retry) { + (false, _) => JSON_FINALIZATION_INSTRUCTION, + (true, true) => MALFORMED_FINALIZATION_INSTRUCTION, + (true, false) => FINALIZATION_INSTRUCTION, + } + .into(), + ), tool_call_id: None, tool_calls: None, }); @@ -2145,6 +2177,12 @@ fn assistant_output_bytes(message: &OpenAiMessage) -> usize { ) } +fn telemetry_usage_token(usage: Option<&serde_json::Value>, field: &str) -> Option { + usage + .and_then(|usage| usage.get(field)) + .and_then(serde_json::Value::as_u64) +} + fn parse_severity(value: &str) -> Severity { match value.trim().to_ascii_lowercase().as_str() { "error" => Severity::Error, @@ -2445,11 +2483,26 @@ mod tests { ) .unwrap(); - assert_eq!(response.choices[0].finish_reason.as_deref(), Some("length")); + assert_eq!( + response.choices[0] + .finish_reason + .as_ref() + .and_then(Value::as_str), + Some("length") + ); let usage = response.usage.unwrap(); - assert_eq!(usage.prompt_tokens, Some(120)); - assert_eq!(usage.completion_tokens, Some(80)); - assert_eq!(usage.total_tokens, Some(200)); + assert_eq!( + telemetry_usage_token(Some(&usage), "prompt_tokens"), + Some(120) + ); + assert_eq!( + telemetry_usage_token(Some(&usage), "completion_tokens"), + Some(80) + ); + assert_eq!( + telemetry_usage_token(Some(&usage), "total_tokens"), + Some(200) + ); } #[test] @@ -2462,6 +2515,55 @@ mod tests { assert!(response.usage.is_none()); } + #[test] + fn ignores_nonstandard_finish_reason_and_usage_shape() { + let response = r#"{ + "choices":[{ + "finish_reason":123, + "message":{"content":"{\"findings\":[]}"} + }], + "usage":[] + }"#; + + let findings = parse_openai_response("ai-review", "AI Review", response).unwrap(); + + assert!(findings.is_empty()); + } + + #[test] + fn ignores_nonstandard_usage_token_types() { + let response = r#"{ + "choices":[{ + "finish_reason":"stop", + "message":{"content":"{\"findings\":[]}"} + }], + "usage":{ + "prompt_tokens":"120", + "completion_tokens":"80", + "total_tokens":-1 + } + }"#; + + let findings = parse_openai_response("ai-review", "AI Review", response).unwrap(); + + assert!(findings.is_empty()); + } + + #[test] + fn classifies_timeout_http_responses_for_finalization() { + assert!(is_timeout_ai_http_response(408, "")); + assert!(is_timeout_ai_http_response(504, "")); + assert!(is_timeout_ai_http_response( + 500, + r#"{"error":"request timed out"}"# + )); + assert!(!is_timeout_ai_http_response( + 500, + r#"{"error":"internal failure"}"# + )); + assert!(!is_timeout_ai_http_response(503, "temporarily unavailable")); + } + #[test] fn parses_findings_from_assistant_content_with_explanatory_prefix() { let response = r#" @@ -3314,6 +3416,67 @@ mod tests { assert_eq!(request_count.load(Ordering::SeqCst), 2); } + #[tokio::test] + async fn malformed_findings_recovery_rejects_context_tools_without_third_request() { + let request_count = Arc::new(AtomicUsize::new(0)); + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap(); + let request_count_for_server = Arc::clone(&request_count); + tokio::spawn(async move { + loop { + let (mut stream, _) = listener.accept().await.unwrap(); + let request_count = Arc::clone(&request_count_for_server); + tokio::spawn(async move { + let request_index = request_count.fetch_add(1, Ordering::SeqCst) + 1; + let _request = read_http_json_request(&mut stream).await; + let body = match request_index { + 1 => { + r#"{"choices":[{"finish_reason":"length","message":{"tool_calls":[{"id":"submit_bad","type":"function","function":{"name":"submit_review_findings","arguments":"{\"findings\":["}}]}}]}"# + } + 2 => { + r#"{"choices":[{"finish_reason":"tool_calls","message":{"tool_calls":[{"id":"read_1","type":"function","function":{"name":"read_file","arguments":"{\"path\":\"src/lib.rs\"}"}}]}}]}"# + } + 3 => { + r#"{"choices":[{"finish_reason":"tool_calls","message":{"tool_calls":[{"id":"submit_unexpected","type":"function","function":{"name":"submit_review_findings","arguments":"{\"findings\":[]}"}}]}}]}"# + } + other => panic!("unexpected request {other}"), + }; + let response = format!( + "HTTP/1.1 200 OK\r\ncontent-type: application/json\r\ncontent-length: {}\r\n\r\n", + body.len() + ); + stream.write_all(response.as_bytes()).await.unwrap(); + stream.write_all(body.as_bytes()).await.unwrap(); + }); + } + }); + + let config = AiReviewConfig { + base_url: format!("http://{addr}"), + ..test_ai_review_config() + }; + let changes = vec![GitLabChange { + old_path: "src/lib.rs".into(), + new_path: "src/lib.rs".into(), + new_file: false, + renamed_file: false, + deleted_file: false, + diff: "@@ -1,0 +1,1 @@\n+panic!();\n".into(), + }]; + + let execution = run_ai_review_execution_with_context(&config, &changes, None, None).await; + + assert_eq!( + execution + .result + .unwrap_err() + .review_failure() + .map(|failure| failure.code), + Some(ReviewErrorCode::AiResponseParseFailed) + ); + assert_eq!(request_count.load(Ordering::SeqCst), 2); + } + #[tokio::test] async fn retries_malformed_json_content_in_json_mode() { let request_count = Arc::new(AtomicUsize::new(0)); @@ -3387,6 +3550,89 @@ mod tests { assert_eq!(json_retry_count.load(Ordering::SeqCst), 1); } + #[tokio::test] + async fn json_malformed_recovery_timeout_keeps_json_only_instruction() { + let request_count = Arc::new(AtomicUsize::new(0)); + let contradictory_instruction_count = Arc::new(AtomicUsize::new(0)); + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap(); + let request_count_for_server = Arc::clone(&request_count); + let contradictory_instruction_count_for_server = + Arc::clone(&contradictory_instruction_count); + tokio::spawn(async move { + loop { + let (mut stream, _) = listener.accept().await.unwrap(); + let request_count = Arc::clone(&request_count_for_server); + let contradictory_instruction_count = + Arc::clone(&contradictory_instruction_count_for_server); + tokio::spawn(async move { + let request_index = request_count.fetch_add(1, Ordering::SeqCst) + 1; + let request = read_http_json_request(&mut stream).await; + let (status, body) = match request_index { + 1 => ("400 Bad Request", r#"{"error":"tools unsupported"}"#), + 2 => ( + "200 OK", + r#"{"choices":[{"finish_reason":"length","message":{"content":"{\"findings\":["}}]}"#, + ), + 3 => ("408 Request Timeout", r#"{"error":"timeout"}"#), + 4 => { + assert_eq!(request["response_format"]["type"], "json_object"); + assert!(request.get("tools").is_none()); + let messages = request["messages"].as_array().unwrap(); + if messages.iter().any(|message| { + message["content"].as_str().is_some_and(|content| { + content.contains("调用 submit_review_findings") + }) + }) { + contradictory_instruction_count.fetch_add(1, Ordering::SeqCst); + } + assert!(messages.iter().any(|message| { + message["content"].as_str().is_some_and(|content| { + content.contains("完整") + && content.contains("JSON") + && content.contains("不得调用任何工具") + }) + })); + ( + "200 OK", + r#"{"choices":[{"finish_reason":"stop","message":{"content":"{\"findings\":[]}"}}]}"#, + ) + } + other => panic!("unexpected request {other}"), + }; + let response = format!( + "HTTP/1.1 {status}\r\ncontent-type: application/json\r\ncontent-length: {}\r\n\r\n", + body.len() + ); + stream.write_all(response.as_bytes()).await.unwrap(); + stream.write_all(body.as_bytes()).await.unwrap(); + }); + } + }); + + let config = AiReviewConfig { + base_url: format!("http://{addr}"), + ..test_ai_review_config() + }; + let changes = vec![GitLabChange { + old_path: "src/lib.rs".into(), + new_path: "src/lib.rs".into(), + new_file: false, + renamed_file: false, + deleted_file: false, + diff: "@@ -1,0 +1,1 @@\n+panic!();\n".into(), + }]; + + let findings = run_ai_review_execution_with_context(&config, &changes, None, None) + .await + .result + .unwrap(); + + assert!(findings.is_empty()); + assert_eq!(request_count.load(Ordering::SeqCst), 4); + assert_eq!(contradictory_instruction_count.load(Ordering::SeqCst), 0); + } + #[tokio::test] async fn retries_malformed_json_content_when_tools_are_accepted() { let request_count = Arc::new(AtomicUsize::new(0)); @@ -4430,7 +4676,7 @@ mod tests { ]; let mut requested = false; - request_timeout_finalization(&mut messages, 2, &mut requested); + request_timeout_finalization(&mut messages, 2, &mut requested, true, false); assert!(requested); assert_eq!(messages.len(), 4); diff --git a/src/review/ai_schema.rs b/src/review/ai_schema.rs index cbb5577..4eb38fa 100644 --- a/src/review/ai_schema.rs +++ b/src/review/ai_schema.rs @@ -60,24 +60,14 @@ pub(crate) struct ChatMessage { pub(crate) struct OpenAiChatResponse { pub(crate) choices: Vec, #[serde(default)] - pub(crate) usage: Option, + pub(crate) usage: Option, } #[derive(Deserialize)] pub(crate) struct OpenAiChoice { pub(crate) message: OpenAiMessage, #[serde(default)] - pub(crate) finish_reason: Option, -} - -#[derive(Deserialize)] -pub(crate) struct OpenAiUsage { - #[serde(default)] - pub(crate) prompt_tokens: Option, - #[serde(default)] - pub(crate) completion_tokens: Option, - #[serde(default)] - pub(crate) total_tokens: Option, + pub(crate) finish_reason: Option, } #[derive(Deserialize)] From 0b18af57e8a560a0f9c887291c6d5d39eff81465 Mon Sep 17 00:00:00 2001 From: SwartzMss Date: Fri, 24 Jul 2026 07:29:01 +0800 Subject: [PATCH 08/19] fix: preserve evidence across repeated timeouts --- src/review/ai.rs | 236 ++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 223 insertions(+), 13 deletions(-) diff --git a/src/review/ai.rs b/src/review/ai.rs index 2dff55e..a3d85f1 100644 --- a/src/review/ai.rs +++ b/src/review/ai.rs @@ -420,7 +420,7 @@ async fn run_ai_review_single( continue 'request_mode; } if attempt < AI_HTTP_ATTEMPTS - && is_retryable_ai_http_response(status, &response_body_preview) + && is_retryable_ai_http_response(status, &body) { let err = AppError::ai_review( ReviewErrorCode::AiRequestFailed, @@ -844,6 +844,19 @@ fn is_timeout_ai_http_response(status: u16, body: &str) -> bool { || normalized.contains("timeout") } +fn should_enter_timeout_finalization(status: u16, body: &str) -> bool { + if matches!(status, 408 | 504) { + return true; + } + if status != 500 { + return false; + } + serde_json::from_str::(body) + .ok() + .and_then(|value| value.get("error")?.get("code")?.as_str().map(str::to_owned)) + .is_some_and(|code| matches!(code.as_str(), "request_timeout" | "RequestTimeout")) +} + struct AiReviewCompletion<'a> { client: &'a ureq::Agent, config: &'a AiReviewConfig, @@ -1269,7 +1282,7 @@ async fn complete_ai_review_response(context: AiReviewCompletion<'_>) -> AppResu if followup_attempt < AI_HTTP_ATTEMPTS && is_retryable_ai_http_response( current_response.status, - &response_body_preview, + ¤t_response.body, ) { let err = AppError::ai_review( @@ -1288,9 +1301,9 @@ async fn complete_ai_review_response(context: AiReviewCompletion<'_>) -> AppResu error = %err, "AI review context follow-up request failed, retrying" ); - if is_timeout_ai_http_response( + if should_enter_timeout_finalization( current_response.status, - &response_body_preview, + ¤t_response.body, ) { request_timeout_finalization( messages, @@ -1458,7 +1471,7 @@ fn request_timeout_finalization( use_tool_calls: bool, malformed_retry: bool, ) { - let evidence_summary = compact_tool_evidence(messages, base_message_count); + let evidence_summary = compact_or_preserve_tool_evidence(messages, base_message_count); messages.truncate(base_message_count); if let Some(evidence_summary) = evidence_summary { messages.push(ChatMessage { @@ -1484,6 +1497,20 @@ fn request_timeout_finalization( }); } +fn compact_or_preserve_tool_evidence( + messages: &[ChatMessage], + base_message_count: usize, +) -> Option { + compact_tool_evidence(messages, base_message_count).or_else(|| { + messages + .iter() + .skip(base_message_count) + .filter_map(|message| message.content.as_deref()) + .find(|content| content.contains("")) + .map(str::to_owned) + }) +} + fn compact_tool_evidence(messages: &[ChatMessage], base_message_count: usize) -> Option { let mut items = Vec::new(); let mut chars_used = 0_usize; @@ -2550,18 +2577,29 @@ mod tests { } #[test] - fn classifies_timeout_http_responses_for_finalization() { - assert!(is_timeout_ai_http_response(408, "")); - assert!(is_timeout_ai_http_response(504, "")); - assert!(is_timeout_ai_http_response( + fn enters_timeout_finalization_only_for_explicit_timeouts() { + assert!(should_enter_timeout_finalization(408, "")); + assert!(should_enter_timeout_finalization(504, "")); + assert!(should_enter_timeout_finalization( 500, - r#"{"error":"request timed out"}"# + r#"{"error":{"code":"request_timeout"}}"# )); - assert!(!is_timeout_ai_http_response( + assert!(should_enter_timeout_finalization( 500, - r#"{"error":"internal failure"}"# + r#"{"error":{"code":"RequestTimeout"}}"# + )); + for body in [ + r#"{"error":"this was not a timeout"}"#, + r#"{"error":"upstream timeout configuration is invalid"}"#, + r#"{"error":"timeout parameter is unsupported"}"#, + r#"{"error":{"code":"internal_error","message":"request timed out"}}"#, + ] { + assert!(!should_enter_timeout_finalization(500, body)); + } + assert!(!should_enter_timeout_finalization( + 503, + r#"{"error":{"code":"request_timeout"}}"# )); - assert!(!is_timeout_ai_http_response(503, "temporarily unavailable")); } #[test] @@ -3633,6 +3671,123 @@ mod tests { assert_eq!(contradictory_instruction_count.load(Ordering::SeqCst), 0); } + #[tokio::test] + async fn repeated_timeout_after_malformed_recovery_preserves_tool_evidence() { + let request_count = Arc::new(AtomicUsize::new(0)); + let first_timeout_finalized = Arc::new(AtomicUsize::new(0)); + let evidence_preserved = Arc::new(AtomicUsize::new(0)); + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap(); + let request_count_for_server = Arc::clone(&request_count); + let first_timeout_finalized_for_server = Arc::clone(&first_timeout_finalized); + let evidence_preserved_for_server = Arc::clone(&evidence_preserved); + tokio::spawn(async move { + loop { + let (mut stream, _) = listener.accept().await.unwrap(); + let request_count = Arc::clone(&request_count_for_server); + let first_timeout_finalized = Arc::clone(&first_timeout_finalized_for_server); + let evidence_preserved = Arc::clone(&evidence_preserved_for_server); + tokio::spawn(async move { + let request_index = request_count.fetch_add(1, Ordering::SeqCst) + 1; + let request = read_http_json_request(&mut stream).await; + let (status, body) = match request_index { + 1 => ( + "200 OK", + r#"{"choices":[{"finish_reason":"tool_calls","message":{"tool_calls":[{"id":"read_1","type":"function","function":{"name":"read_file","arguments":"{\"path\":\"src/lib.rs\"}"}}]}}]}"# + .to_owned(), + ), + 2 => ( + "500 Internal Server Error", + format!( + "{{\n \"error\": {{\"message\": \"{}\", \"code\": \"request_timeout\"}}\n}}", + "x".repeat(AI_RESPONSE_PREVIEW_CHARS + 100) + ), + ), + 3 => { + let messages = request["messages"].as_array().unwrap(); + let has_summary = messages.iter().any(|message| { + message["content"].as_str().is_some_and(|content| { + content.contains("") + }) + }); + let has_tool_message = + messages.iter().any(|message| message["role"] == "tool"); + if has_summary && !has_tool_message { + first_timeout_finalized.fetch_add(1, Ordering::SeqCst); + } + ( + "200 OK", + r#"{"choices":[{"finish_reason":"length","message":{"tool_calls":[{"id":"submit_bad","type":"function","function":{"name":"submit_review_findings","arguments":"{\"findings\":["}}]}}]}"# + .to_owned(), + ) + } + 4 => ( + "408 Request Timeout", + r#"{"error":{"code":"request_timeout"}}"#.to_owned(), + ), + 5 => { + let messages = request["messages"].as_array().unwrap(); + if messages.iter().any(|message| { + message["content"].as_str().is_some_and(|content| { + content.contains("") + && content.contains("panic!();") + }) + }) { + evidence_preserved.fetch_add(1, Ordering::SeqCst); + } + let tool_names = request["tools"] + .as_array() + .unwrap() + .iter() + .filter_map(|tool| tool["function"]["name"].as_str()) + .collect::>(); + assert_eq!(tool_names, ["submit_review_findings"]); + ( + "200 OK", + r#"{"choices":[{"finish_reason":"tool_calls","message":{"tool_calls":[{"id":"submit_ok","type":"function","function":{"name":"submit_review_findings","arguments":"{\"findings\":[]}"}}]}}]}"# + .to_owned(), + ) + } + other => panic!("unexpected request {other}"), + }; + let response = format!( + "HTTP/1.1 {status}\r\ncontent-type: application/json\r\ncontent-length: {}\r\n\r\n", + body.len() + ); + stream.write_all(response.as_bytes()).await.unwrap(); + stream.write_all(body.as_bytes()).await.unwrap(); + }); + } + }); + + let source = tempfile::tempdir().unwrap(); + std::fs::create_dir_all(source.path().join("src")).unwrap(); + std::fs::write(source.path().join("src/lib.rs"), "panic!();\n").unwrap(); + let config = AiReviewConfig { + base_url: format!("http://{addr}"), + ..test_ai_review_config() + }; + let changes = vec![GitLabChange { + old_path: "src/lib.rs".into(), + new_path: "src/lib.rs".into(), + new_file: false, + renamed_file: false, + deleted_file: false, + diff: "@@ -1,0 +1,1 @@\n+panic!();\n".into(), + }]; + + let findings = + run_ai_review_execution_with_context(&config, &changes, Some(source.path()), None) + .await + .result + .unwrap(); + + assert!(findings.is_empty()); + assert_eq!(request_count.load(Ordering::SeqCst), 5); + assert_eq!(first_timeout_finalized.load(Ordering::SeqCst), 1); + assert_eq!(evidence_preserved.load(Ordering::SeqCst), 1); + } + #[tokio::test] async fn retries_malformed_json_content_when_tools_are_accepted() { let request_count = Arc::new(AtomicUsize::new(0)); @@ -4695,6 +4850,61 @@ mod tests { assert!(messages.iter().all(|message| message.tool_calls.is_none())); } + #[test] + fn timeout_finalization_preserves_existing_evidence_summary() { + let mut messages = vec![ + ChatMessage { + role: "system".into(), + content: Some("system".into()), + tool_call_id: None, + tool_calls: None, + }, + ChatMessage { + role: "user".into(), + content: Some("current diff".into()), + tool_call_id: None, + tool_calls: None, + }, + ChatMessage { + role: "assistant".into(), + content: None, + tool_call_id: None, + tool_calls: Some(vec![OpenAiToolCall { + id: "call_1".into(), + call_type: "function".into(), + function: super::super::ai_schema::OpenAiToolCallFunction { + name: "read_file".into(), + arguments: r#"{"path":"src/lib.rs"}"#.into(), + }, + }]), + }, + ChatMessage { + role: "tool".into(), + content: Some(r#"{"content":"panic!();"}"#.into()), + tool_call_id: Some("call_1".into()), + tool_calls: None, + }, + ]; + let mut requested = false; + + request_timeout_finalization(&mut messages, 2, &mut requested, true, false); + request_timeout_finalization(&mut messages, 2, &mut requested, true, true); + + let summaries = messages + .iter() + .filter_map(|message| message.content.as_deref()) + .filter(|content| content.contains("")) + .collect::>(); + assert_eq!(summaries.len(), 1); + assert!(summaries[0].contains("panic!();")); + assert_eq!( + messages + .last() + .and_then(|message| message.content.as_deref()), + Some(MALFORMED_FINALIZATION_INSTRUCTION) + ); + } + #[test] fn previews_ai_response_without_splitting_utf8() { let preview = preview_log_text("中文\nabcdef", 5); From 1328a172a95bfd976f240f2c0af00892ad6df2e8 Mon Sep 17 00:00:00 2001 From: SwartzMss Date: Fri, 24 Jul 2026 11:04:33 +0800 Subject: [PATCH 09/19] fix: classify final AI timeout responses --- src/review/ai.rs | 210 +++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 195 insertions(+), 15 deletions(-) diff --git a/src/review/ai.rs b/src/review/ai.rs index a3d85f1..c50ec30 100644 --- a/src/review/ai.rs +++ b/src/review/ai.rs @@ -443,6 +443,8 @@ async fn run_ai_review_single( } let code = if matches!(status, 401 | 403) { ReviewErrorCode::PermissionDenied + } else if should_enter_timeout_finalization(status, &body) { + ReviewErrorCode::AiRequestTimeout } else { ReviewErrorCode::AiRequestFailed }; @@ -1311,6 +1313,7 @@ async fn complete_ai_review_response(context: AiReviewCompletion<'_>) -> AppResu &mut finalization_requested, use_tool_calls, malformed_finalization_retry_requested, + diff_only_fallback_requested, ); request_body = serialize_review_request_body( config, @@ -1340,6 +1343,7 @@ async fn complete_ai_review_response(context: AiReviewCompletion<'_>) -> AppResu &mut finalization_requested, use_tool_calls, malformed_finalization_retry_requested, + diff_only_fallback_requested, ); request_body = serialize_review_request_body( config, @@ -1377,6 +1381,7 @@ async fn complete_ai_review_response(context: AiReviewCompletion<'_>) -> AppResu &mut finalization_requested, use_tool_calls, malformed_finalization_retry_requested, + diff_only_fallback_requested, ); request_body = serialize_review_request_body(config, messages, use_tool_calls, false)?; @@ -1407,10 +1412,10 @@ async fn complete_ai_review_response(context: AiReviewCompletion<'_>) -> AppResu }) })?; if !(200..300).contains(&response.status) { - let code = if response.status == 504 && finalization_requested { - ReviewErrorCode::AiToolLoopTimeout - } else if matches!(response.status, 401 | 403) { + let code = if matches!(response.status, 401 | 403) { ReviewErrorCode::PermissionDenied + } else if should_enter_timeout_finalization(response.status, &response.body) { + ReviewErrorCode::AiToolLoopTimeout } else { ReviewErrorCode::AiRequestFailed }; @@ -1470,6 +1475,7 @@ fn request_timeout_finalization( requested: &mut bool, use_tool_calls: bool, malformed_retry: bool, + diff_only_fallback_requested: bool, ) { let evidence_summary = compact_or_preserve_tool_evidence(messages, base_message_count); messages.truncate(base_message_count); @@ -1485,10 +1491,15 @@ fn request_timeout_finalization( messages.push(ChatMessage { role: "user".into(), content: Some( - match (use_tool_calls, malformed_retry) { - (false, _) => JSON_FINALIZATION_INSTRUCTION, - (true, true) => MALFORMED_FINALIZATION_INSTRUCTION, - (true, false) => FINALIZATION_INSTRUCTION, + match ( + use_tool_calls, + malformed_retry, + diff_only_fallback_requested, + ) { + (false, _, _) => JSON_FINALIZATION_INSTRUCTION, + (true, true, _) => MALFORMED_FINALIZATION_INSTRUCTION, + (true, false, true) => DIFF_ONLY_FINALIZATION_INSTRUCTION, + (true, false, false) => FINALIZATION_INSTRUCTION, } .into(), ), @@ -2470,6 +2481,149 @@ mod tests { serde_json::from_slice(&data[body_start..expected_len]).unwrap() } + async fn run_initial_http_error(status: &str, body: &str) -> AppError { + let status = status.to_owned(); + let body = body.to_owned(); + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap(); + tokio::spawn(async move { + loop { + let (mut stream, _) = listener.accept().await.unwrap(); + let status = status.clone(); + let body = body.clone(); + tokio::spawn(async move { + let _ = read_http_json_request(&mut stream).await; + let response = format!( + "HTTP/1.1 {status}\r\ncontent-type: application/json\r\ncontent-length: {}\r\n\r\n", + body.len() + ); + stream.write_all(response.as_bytes()).await.unwrap(); + stream.write_all(body.as_bytes()).await.unwrap(); + }); + } + }); + let config = AiReviewConfig { + base_url: format!("http://{addr}"), + ..test_ai_review_config() + }; + let changes = vec![GitLabChange { + old_path: "src/lib.rs".into(), + new_path: "src/lib.rs".into(), + new_file: false, + renamed_file: false, + deleted_file: false, + diff: "@@ -1 +1 @@\n+panic!();\n".into(), + }]; + run_ai_review(&config, &changes).await.unwrap_err() + } + + #[tokio::test] + async fn final_initial_timeout_http_response_is_ai_request_timeout() { + for (status, body) in [ + ("408 Request Timeout", ""), + ("504 Gateway Timeout", ""), + ( + "500 Internal Server Error", + r#"{"error":{"code":"request_timeout"}}"#, + ), + ( + "500 Internal Server Error", + r#"{"error":{"code":"RequestTimeout"}}"#, + ), + ] { + let error = run_initial_http_error(status, body).await; + assert_eq!( + error.review_failure().map(|failure| failure.code), + Some(ReviewErrorCode::AiRequestTimeout), + "status={status}, body={body}" + ); + } + } + + async fn run_followup_http_errors( + first_followup: (&str, &str), + final_followup: (&str, &str), + ) -> (AppError, usize) { + let first_followup = (first_followup.0.to_owned(), first_followup.1.to_owned()); + let final_followup = (final_followup.0.to_owned(), final_followup.1.to_owned()); + let request_count = Arc::new(AtomicUsize::new(0)); + let request_count_for_server = Arc::clone(&request_count); + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap(); + tokio::spawn(async move { + loop { + let (mut stream, _) = listener.accept().await.unwrap(); + let request_count = Arc::clone(&request_count_for_server); + let first_followup = first_followup.clone(); + let final_followup = final_followup.clone(); + tokio::spawn(async move { + let request_index = request_count.fetch_add(1, Ordering::SeqCst) + 1; + let _ = read_http_json_request(&mut stream).await; + let (status, body) = match request_index { + 1 => ( + "200 OK".to_owned(), + r#"{"choices":[{"message":{"tool_calls":[{"id":"call_1","type":"function","function":{"name":"search_code","arguments":"{\"query\":\"panic\"}"}}]}}]}"#.to_owned(), + ), + 2 => first_followup, + _ => final_followup, + }; + let response = format!( + "HTTP/1.1 {status}\r\ncontent-type: application/json\r\ncontent-length: {}\r\n\r\n", + body.len() + ); + stream.write_all(response.as_bytes()).await.unwrap(); + stream.write_all(body.as_bytes()).await.unwrap(); + }); + } + }); + let config = AiReviewConfig { + base_url: format!("http://{addr}"), + ..test_ai_review_config() + }; + let changes = vec![GitLabChange { + old_path: "src/lib.rs".into(), + new_path: "src/lib.rs".into(), + new_file: false, + renamed_file: false, + deleted_file: false, + diff: "@@ -1 +1 @@\n+panic!();\n".into(), + }]; + let source = tempfile::tempdir().unwrap(); + let error = + run_ai_review_execution_with_context(&config, &changes, Some(source.path()), None) + .await + .result + .unwrap_err(); + (error, request_count.load(Ordering::SeqCst)) + } + + #[tokio::test] + async fn final_followup_timeout_http_response_is_tool_loop_timeout() { + for (first, final_response) in [ + (("408 Request Timeout", ""), ("408 Request Timeout", "")), + (("504 Gateway Timeout", ""), ("504 Gateway Timeout", "")), + ( + ( + "500 Internal Server Error", + r#"{"error":{"code":"request_timeout"}}"#, + ), + ( + "500 Internal Server Error", + r#"{"error":{"code":"request_timeout"}}"#, + ), + ), + (("503 Service Unavailable", ""), ("504 Gateway Timeout", "")), + ] { + let (error, request_count) = run_followup_http_errors(first, final_response).await; + assert_eq!(request_count, 3); + assert_eq!( + error.review_failure().map(|failure| failure.code), + Some(ReviewErrorCode::AiToolLoopTimeout), + "first={first:?}, final={final_response:?}" + ); + } + } + #[test] fn parses_openai_compatible_findings_from_assistant_content() { let response = r#" @@ -4277,8 +4431,10 @@ mod tests { async fn context_request_after_finalization_falls_back_to_diff_only() { let request_count = Arc::new(AtomicUsize::new(0)); let fallback_request_clean = Arc::new(AtomicUsize::new(0)); + let diff_only_instruction_count = Arc::new(AtomicUsize::new(0)); let request_count_for_server = Arc::clone(&request_count); let fallback_request_clean_for_server = Arc::clone(&fallback_request_clean); + let diff_only_instruction_count_for_server = Arc::clone(&diff_only_instruction_count); let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); let addr = listener.local_addr().unwrap(); tokio::spawn(async move { @@ -4286,11 +4442,16 @@ mod tests { let (mut stream, _) = listener.accept().await.unwrap(); let request_count = Arc::clone(&request_count_for_server); let fallback_request_clean = Arc::clone(&fallback_request_clean_for_server); + let diff_only_instruction_count = + Arc::clone(&diff_only_instruction_count_for_server); tokio::spawn(async move { let request_index = request_count.fetch_add(1, Ordering::SeqCst) + 1; let request = read_http_json_request(&mut stream).await; - let body = if request_index <= 2 { - r#"{"choices":[{"message":{"tool_calls":[{"id":"call_1","type":"function","function":{"name":"search_code","arguments":"{\"query\":\"panic\"}"}}]}}]}"# + let (status, body) = if request_index <= 2 { + ( + "200 OK", + r#"{"choices":[{"message":{"tool_calls":[{"id":"call_1","type":"function","function":{"name":"search_code","arguments":"{\"query\":\"panic\"}"}}]}}]}"#, + ) } else { let tool_names = request["tools"] .as_array() @@ -4316,10 +4477,28 @@ mod tests { }); fallback_request_clean .store(usize::from(!leaked_context_history), Ordering::SeqCst); - r#"{"choices":[{"message":{"tool_calls":[{"id":"submit_1","type":"function","function":{"name":"submit_review_findings","arguments":"{\"findings\":[]}"}}]}}]}"# + if request["messages"] + .as_array() + .unwrap() + .iter() + .any(|message| { + message["content"].as_str() + == Some(DIFF_ONLY_FINALIZATION_INSTRUCTION) + }) + { + diff_only_instruction_count.fetch_add(1, Ordering::SeqCst); + } + if request_index == 3 { + ("408 Request Timeout", "") + } else { + ( + "200 OK", + r#"{"choices":[{"message":{"tool_calls":[{"id":"submit_1","type":"function","function":{"name":"submit_review_findings","arguments":"{\"findings\":[]}"}}]}}]}"#, + ) + } }; let response = format!( - "HTTP/1.1 200 OK\r\ncontent-type: application/json\r\ncontent-length: {}\r\n\r\n", + "HTTP/1.1 {status}\r\ncontent-type: application/json\r\ncontent-length: {}\r\n\r\n", body.len() ); stream.write_all(response.as_bytes()).await.unwrap(); @@ -4349,8 +4528,9 @@ mod tests { assert!(execution.result.unwrap().is_empty()); assert_eq!(execution.coverage.unwrap().tool_calls_used, 1); - assert_eq!(request_count.load(Ordering::SeqCst), 3); + assert_eq!(request_count.load(Ordering::SeqCst), 4); assert_eq!(fallback_request_clean.load(Ordering::SeqCst), 1); + assert_eq!(diff_only_instruction_count.load(Ordering::SeqCst), 2); } #[tokio::test] @@ -4831,7 +5011,7 @@ mod tests { ]; let mut requested = false; - request_timeout_finalization(&mut messages, 2, &mut requested, true, false); + request_timeout_finalization(&mut messages, 2, &mut requested, true, false, false); assert!(requested); assert_eq!(messages.len(), 4); @@ -4887,8 +5067,8 @@ mod tests { ]; let mut requested = false; - request_timeout_finalization(&mut messages, 2, &mut requested, true, false); - request_timeout_finalization(&mut messages, 2, &mut requested, true, true); + request_timeout_finalization(&mut messages, 2, &mut requested, true, false, false); + request_timeout_finalization(&mut messages, 2, &mut requested, true, true, false); let summaries = messages .iter() From 87d5cbae8c33f446a16f8d7a5a667d7a5e26ad09 Mon Sep 17 00:00:00 2001 From: SwartzMss Date: Fri, 24 Jul 2026 13:18:11 +0800 Subject: [PATCH 10/19] fix: validate final AI findings payloads --- src/review/ai.rs | 171 ++++++++++++++++++++++++++++++++-------- src/review/ai_schema.rs | 1 - 2 files changed, 136 insertions(+), 36 deletions(-) diff --git a/src/review/ai.rs b/src/review/ai.rs index c50ec30..220e269 100644 --- a/src/review/ai.rs +++ b/src/review/ai.rs @@ -40,6 +40,7 @@ const MIN_CONTEXT_TOOL_RESULT_BYTES: usize = 28; const FINALIZATION_INSTRUCTION: &str = "上下文工具预算已用尽。放弃任何仍缺少证据的候选问题，不得继续请求 read_file、search_code 或 list_files。请立即提交最终审查结果并调用 submit_review_findings；没有已确认问题时提交空 findings。"; const DIFF_ONLY_FINALIZATION_INSTRUCTION: &str = "上下文工具已关闭。请只基于原始 diff 和已明确提供的信息完成审查，不得继续请求 read_file、search_code 或 list_files。放弃任何仍缺少证据的候选问题，并立即调用 submit_review_findings；没有已确认问题时提交空 findings。"; const MALFORMED_FINALIZATION_INSTRUCTION: &str = "上一次最终审查结果不是完整 JSON。不得继续请求 read_file、search_code 或 list_files。请压缩每条问题描述，放弃证据不足的问题，并立即重新提交一次完整 JSON；可用 submit_review_findings 时必须调用它，没有已确认问题时提交空 findings。"; +const MALFORMED_DIFF_ONLY_FINALIZATION_INSTRUCTION: &str = "上一次最终审查结果不是完整 JSON。上下文工具已关闭。请只基于原始 diff 和已明确提供的信息重新提交完整 JSON；不得请求或依赖 read_file、search_code 或 list_files。请压缩每条问题描述，放弃证据不足的问题，并立即调用 submit_review_findings；没有已确认问题时提交空 findings。"; const JSON_FINALIZATION_INSTRUCTION: &str = "请立即返回一个完整且精简的 JSON 对象，格式必须为 {\"findings\":[...]}。不得调用任何工具；没有已确认问题时返回 {\"findings\":[]}。"; #[derive(Clone, Copy, Debug, PartialEq, Eq)] @@ -952,8 +953,7 @@ async fn complete_ai_review_response(context: AiReviewCompletion<'_>) -> AppResu "AI review context tool calls completed" ); } - let payload = final_findings_payload(message)?; - match parse_openai_findings_payload(&config.id, &config.title, payload) { + match parse_final_findings_candidates(&config.id, &config.title, message) { Ok(findings) => return Ok(findings), Err(err) if !malformed_finalization_retry_requested => { malformed_finalization_retry_requested = true; @@ -1497,7 +1497,8 @@ fn request_timeout_finalization( diff_only_fallback_requested, ) { (false, _, _) => JSON_FINALIZATION_INSTRUCTION, - (true, true, _) => MALFORMED_FINALIZATION_INSTRUCTION, + (true, true, true) => MALFORMED_DIFF_ONLY_FINALIZATION_INSTRUCTION, + (true, true, false) => MALFORMED_FINALIZATION_INSTRUCTION, (true, false, true) => DIFF_ONLY_FINALIZATION_INSTRUCTION, (true, false, false) => FINALIZATION_INSTRUCTION, } @@ -2084,24 +2085,42 @@ fn parse_openai_message( title: &str, message: &OpenAiMessage, ) -> AppResult> { - let content = final_findings_payload(message)?; - parse_openai_findings_payload(review_id, title, content) + parse_final_findings_candidates(review_id, title, message) } -fn final_findings_payload(message: &OpenAiMessage) -> AppResult<&str> { - tool_call_arguments(message).or_else(|_| { - message - .content - .as_deref() - .map(str::trim) - .filter(|content| !content.is_empty()) - .ok_or_else(|| { - AppError::ai_review( - ReviewErrorCode::AiResponseParseFailed, - "AI review API returned no content", - ) - }) - }) +fn parse_final_findings_candidates( + review_id: &str, + title: &str, + message: &OpenAiMessage, +) -> AppResult> { + let mut last_error = None; + for tool_call in message + .tool_calls + .iter() + .filter(|tool_call| tool_call.function.name == "submit_review_findings") + { + match parse_openai_findings_payload(review_id, title, &tool_call.function.arguments) { + Ok(findings) => return Ok(findings), + Err(error) => last_error = Some(error), + } + } + if let Some(content) = message + .content + .as_deref() + .map(str::trim) + .filter(|content| !content.is_empty()) + { + match parse_openai_findings_payload(review_id, title, content) { + Ok(findings) => return Ok(findings), + Err(error) => last_error = Some(error), + } + } + Err(last_error.unwrap_or_else(|| { + AppError::ai_review( + ReviewErrorCode::AiResponseParseFailed, + "AI review API returned no valid findings payload", + ) + })) } fn parse_openai_findings_payload( @@ -2187,20 +2206,6 @@ fn extract_first_json_object(content: &str) -> Option<&str> { None } -fn tool_call_arguments(message: &OpenAiMessage) -> AppResult<&str> { - message - .tool_calls - .iter() - .find(|tool_call| tool_call.function.name == "submit_review_findings") - .map(|tool_call| tool_call.function.arguments.as_str()) - .ok_or_else(|| { - AppError::ai_review( - ReviewErrorCode::AiResponseParseFailed, - "AI review API returned no submit_review_findings tool call", - ) - }) -} - fn assistant_output_bytes(message: &OpenAiMessage) -> usize { message .content @@ -2783,6 +2788,21 @@ mod tests { assert_eq!(parsed.findings[0].message, "contains } brace"); } + #[test] + fn rejects_findings_payload_without_required_findings_field() { + for content in [r#"{}"#, r#"{"finding":[]}"#, r#"{"unexpected":"value"}"#] { + let error = match parse_ai_findings_response(content) { + Ok(_) => panic!("missing findings field was accepted: {content}"), + Err(error) => error, + }; + assert_eq!( + error.review_failure().map(|failure| failure.code), + Some(ReviewErrorCode::AiResponseParseFailed), + "content={content}" + ); + } + } + #[test] fn treats_unknown_ai_severity_as_error() { let response = r#" @@ -3465,6 +3485,58 @@ mod tests { assert_eq!(findings[0].new_line, Some(12)); } + #[test] + fn uses_valid_content_when_submit_tool_arguments_are_malformed() { + let response = r#"{ + "choices":[{ + "message":{ + "content":"{\"findings\":[]}", + "tool_calls":[{ + "type":"function", + "function":{ + "name":"submit_review_findings", + "arguments":"{\"findings\":[" + } + }] + } + }] + }"#; + + let findings = parse_openai_response("ai-review", "AI Review", response).unwrap(); + + assert!(findings.is_empty()); + } + + #[test] + fn uses_later_valid_submit_tool_arguments() { + let response = r#"{ + "choices":[{ + "message":{ + "tool_calls":[ + { + "type":"function", + "function":{ + "name":"submit_review_findings", + "arguments":"{\"findings\":[" + } + }, + { + "type":"function", + "function":{ + "name":"submit_review_findings", + "arguments":"{\"findings\":[]}" + } + } + ] + } + }] + }"#; + + let findings = parse_openai_response("ai-review", "AI Review", response).unwrap(); + + assert!(findings.is_empty()); + } + #[test] fn reuses_shared_ai_http_client() { let client_one = shared_ai_http_client().unwrap() as *const ureq::Agent; @@ -3571,7 +3643,7 @@ mod tests { tokio::spawn(async move { request_count.fetch_add(1, Ordering::SeqCst); let _request = read_http_json_request(&mut stream).await; - let body = r#"{"choices":[{"finish_reason":"length","message":{"tool_calls":[{"id":"submit_bad","type":"function","function":{"name":"submit_review_findings","arguments":"{\"findings\":["}}]}}]}"#; + let body = r#"{"choices":[{"finish_reason":"stop","message":{"tool_calls":[{"id":"submit_bad","type":"function","function":{"name":"submit_review_findings","arguments":"{}"}}]}}]}"#; let response = format!( "HTTP/1.1 200 OK\r\ncontent-type: application/json\r\ncontent-length: {}\r\n\r\n", body.len() @@ -3689,7 +3761,7 @@ mod tests { 1 => ("400 Bad Request", r#"{"error":"tools unsupported"}"#), 2 => ( "200 OK", - r#"{"choices":[{"finish_reason":"length","message":{"content":"{\"findings\":["}}]}"#, + r#"{"choices":[{"finish_reason":"stop","message":{"content":"{}"}}]}"#, ), 3 => { assert_eq!(request["response_format"]["type"], "json_object"); @@ -5085,6 +5157,35 @@ mod tests { ); } + #[test] + fn malformed_diff_only_timeout_finalization_preserves_both_constraints() { + let mut messages = vec![ + ChatMessage { + role: "system".into(), + content: Some("system".into()), + tool_call_id: None, + tool_calls: None, + }, + ChatMessage { + role: "user".into(), + content: Some("current diff".into()), + tool_call_id: None, + tool_calls: None, + }, + ]; + let mut requested = true; + + request_timeout_finalization(&mut messages, 2, &mut requested, true, true, true); + + let instruction = messages + .last() + .and_then(|message| message.content.as_deref()) + .unwrap(); + assert!(instruction.contains("上一次最终审查结果不是完整 JSON")); + assert!(instruction.contains("只基于原始 diff 和已明确提供的信息")); + assert!(instruction.contains("不得请求或依赖 read_file、search_code 或 list_files")); + } + #[test] fn previews_ai_response_without_splitting_utf8() { let preview = preview_log_text("中文\nabcdef", 5); diff --git a/src/review/ai_schema.rs b/src/review/ai_schema.rs index 4eb38fa..bef4e0c 100644 --- a/src/review/ai_schema.rs +++ b/src/review/ai_schema.rs @@ -95,7 +95,6 @@ pub(crate) struct OpenAiToolCallFunction { #[derive(Deserialize)] pub(crate) struct AiFindingsResponse { - #[serde(default)] pub(crate) findings: Vec, } From b2c3e19c0dfff8dbbf80a746f80eaf84730a0d6f Mon Sep 17 00:00:00 2001 From: SwartzMss Date: Fri, 24 Jul 2026 13:45:24 +0800 Subject: [PATCH 11/19] fix: harden AI findings candidate validation --- src/review/ai.rs | 199 ++++++++++++++++++++++++++++++++++------- src/review/ai_tools.rs | 8 +- 2 files changed, 171 insertions(+), 36 deletions(-) diff --git a/src/review/ai.rs b/src/review/ai.rs index 220e269..8718d60 100644 --- a/src/review/ai.rs +++ b/src/review/ai.rs @@ -36,6 +36,7 @@ const AI_HTTP_ATTEMPTS: usize = 2; const TOOL_ARGUMENT_SUMMARY_CHARS: usize = 160; const TIMEOUT_EVIDENCE_SUMMARY_CHARS: usize = 6000; const TIMEOUT_EVIDENCE_ITEM_CHARS: usize = 1500; +const MAX_FINDINGS_JSON_CANDIDATES: usize = 8; const MIN_CONTEXT_TOOL_RESULT_BYTES: usize = 28; const FINALIZATION_INSTRUCTION: &str = "上下文工具预算已用尽。放弃任何仍缺少证据的候选问题，不得继续请求 read_file、search_code 或 list_files。请立即提交最终审查结果并调用 submit_review_findings；没有已确认问题时提交空 findings。"; const DIFF_ONLY_FINALIZATION_INSTRUCTION: &str = "上下文工具已关闭。请只基于原始 diff 和已明确提供的信息完成审查，不得继续请求 read_file、search_code 或 list_files。放弃任何仍缺少证据的候选问题，并立即调用 submit_review_findings；没有已确认问题时提交空 findings。"; @@ -953,6 +954,12 @@ async fn complete_ai_review_response(context: AiReviewCompletion<'_>) -> AppResu "AI review context tool calls completed" ); } + if !has_final_findings_candidate(message) { + return Err(AppError::ai_review( + ReviewErrorCode::AiResponseParseFailed, + "AI review API returned no findings payload", + )); + } match parse_final_findings_candidates(&config.id, &config.title, message) { Ok(findings) => return Ok(findings), Err(err) if !malformed_finalization_retry_requested => { @@ -2094,13 +2101,14 @@ fn parse_final_findings_candidates( message: &OpenAiMessage, ) -> AppResult> { let mut last_error = None; + let mut valid_candidates = Vec::new(); for tool_call in message .tool_calls .iter() .filter(|tool_call| tool_call.function.name == "submit_review_findings") { match parse_openai_findings_payload(review_id, title, &tool_call.function.arguments) { - Ok(findings) => return Ok(findings), + Ok(findings) => valid_candidates.push(findings), Err(error) => last_error = Some(error), } } @@ -2111,10 +2119,23 @@ fn parse_final_findings_candidates( .filter(|content| !content.is_empty()) { match parse_openai_findings_payload(review_id, title, content) { - Ok(findings) => return Ok(findings), + Ok(findings) => valid_candidates.push(findings), Err(error) => last_error = Some(error), } } + if let Some(first) = valid_candidates.first() { + if valid_candidates + .iter() + .skip(1) + .all(|candidate| candidate == first) + { + return Ok(first.clone()); + } + return Err(AppError::ai_review( + ReviewErrorCode::AiResponseParseFailed, + "AI review returned conflicting findings payloads", + )); + } Err(last_error.unwrap_or_else(|| { AppError::ai_review( ReviewErrorCode::AiResponseParseFailed, @@ -2123,6 +2144,17 @@ fn parse_final_findings_candidates( })) } +fn has_final_findings_candidate(message: &OpenAiMessage) -> bool { + message + .tool_calls + .iter() + .any(|tool_call| tool_call.function.name == "submit_review_findings") + || message + .content + .as_deref() + .is_some_and(|content| !content.trim().is_empty()) +} + fn parse_openai_findings_payload( review_id: &str, title: &str, @@ -2140,44 +2172,56 @@ fn parse_openai_findings_payload( parsed_findings = parsed.findings.len(), "AI review assistant findings parsed" ); - Ok(parsed - .findings - .into_iter() - .filter(|finding| !finding.path.trim().is_empty() && !finding.message.trim().is_empty()) - .map(|finding| Finding { + let mut findings = Vec::with_capacity(parsed.findings.len()); + for finding in parsed.findings { + if finding.path.trim().is_empty() + || finding.message.trim().is_empty() + || finding.title.trim().is_empty() + || finding.line == 0 + { + return Err(AppError::ai_review( + ReviewErrorCode::AiResponseParseFailed, + "AI review returned a finding that failed semantic validation", + )); + } + findings.push(Finding { rule_id: format!("ai:{review_id}"), severity: parse_severity(&finding.severity), path: finding.path.trim().replace('\\', "/"), new_line: Some(finding.line), title: non_empty_or(finding.title, title), message: finding.message.trim().to_string(), - }) - .collect()) + }); + } + Ok(findings) } fn parse_ai_findings_response(content: &str) -> AppResult { match serde_json::from_str(content) { Ok(parsed) => Ok(parsed), Err(strict_error) => { - let Some(json_content) = extract_first_json_object(content) else { - return Err(AppError::ai_review( - ReviewErrorCode::AiResponseParseFailed, - strict_error.to_string(), - )); - }; - serde_json::from_str(json_content).map_err(|err| { - AppError::ai_review(ReviewErrorCode::AiResponseParseFailed, err.to_string()) - }) + let mut last_error = strict_error; + for json_content in extract_json_objects(content, MAX_FINDINGS_JSON_CANDIDATES) { + match serde_json::from_str(json_content) { + Ok(parsed) => return Ok(parsed), + Err(error) => last_error = error, + } + } + Err(AppError::ai_review( + ReviewErrorCode::AiResponseParseFailed, + last_error.to_string(), + )) } } } -fn extract_first_json_object(content: &str) -> Option<&str> { - let start = content.find('{')?; +fn extract_json_objects(content: &str, limit: usize) -> Vec<&str> { + let mut objects = Vec::new(); + let mut start = None; let mut depth = 0_usize; let mut in_string = false; let mut escaped = false; - for (offset, ch) in content[start..].char_indices() { + for (offset, ch) in content.char_indices() { if in_string { if escaped { escaped = false; @@ -2192,18 +2236,30 @@ fn extract_first_json_object(content: &str) -> Option<&str> { } match ch { '"' => in_string = true, - '{' => depth += 1, + '{' => { + if depth == 0 { + start = Some(offset); + } + depth += 1; + } '}' => { - depth = depth.checked_sub(1)?; + let Some(next_depth) = depth.checked_sub(1) else { + continue; + }; + depth = next_depth; if depth == 0 { - let end = start + offset + ch.len_utf8(); - return Some(&content[start..end]); + if let Some(start) = start.take() { + objects.push(&content[start..offset + ch.len_utf8()]); + if objects.len() == limit { + break; + } + } } } _ => {} } } - None + objects } fn assistant_output_bytes(message: &OpenAiMessage) -> usize { @@ -2788,6 +2844,15 @@ mod tests { assert_eq!(parsed.findings[0].message, "contains } brace"); } + #[test] + fn parses_later_valid_json_object_from_assistant_content() { + let content = "Provider metadata: {}\nFinal answer:\n{\"findings\":[]}"; + + let parsed = parse_ai_findings_response(content).unwrap(); + + assert!(parsed.findings.is_empty()); + } + #[test] fn rejects_findings_payload_without_required_findings_field() { for content in [r#"{}"#, r#"{"finding":[]}"#, r#"{"unexpected":"value"}"#] { @@ -2803,6 +2868,23 @@ mod tests { } } + #[test] + fn rejects_semantically_invalid_nonempty_findings() { + for payload in [ + r#"{"findings":[{"path":"","line":10,"severity":"error","title":"Bug","message":""}]}"#, + r#"{"findings":[{"path":"src/lib.rs","line":0,"severity":"error","title":"Bug","message":"Issue"}]}"#, + r#"{"findings":[{"path":"src/lib.rs","line":10,"severity":"error","title":" ","message":"Issue"}]}"#, + ] { + let error = + parse_openai_findings_payload("ai-review", "AI Review", payload).unwrap_err(); + assert_eq!( + error.review_failure().map(|failure| failure.code), + Some(ReviewErrorCode::AiResponseParseFailed), + "payload={payload}" + ); + } + } + #[test] fn treats_unknown_ai_severity_as_error() { let response = r#" @@ -3508,7 +3590,7 @@ mod tests { } #[test] - fn uses_later_valid_submit_tool_arguments() { + fn rejects_conflicting_valid_submit_tool_arguments() { let response = r#"{ "choices":[{ "message":{ @@ -3517,14 +3599,14 @@ mod tests { "type":"function", "function":{ "name":"submit_review_findings", - "arguments":"{\"findings\":[" + "arguments":"{\"findings\":[]}" } }, { "type":"function", "function":{ "name":"submit_review_findings", - "arguments":"{\"findings\":[]}" + "arguments":"{\"findings\":[{\"path\":\"src/lib.rs\",\"line\":10,\"severity\":\"error\",\"title\":\"Bug\",\"message\":\"Real issue\"}]}" } } ] @@ -3532,9 +3614,12 @@ mod tests { }] }"#; - let findings = parse_openai_response("ai-review", "AI Review", response).unwrap(); - - assert!(findings.is_empty()); + let error = parse_openai_response("ai-review", "AI Review", response).unwrap_err(); + assert_eq!( + error.review_failure().map(|failure| failure.code), + Some(ReviewErrorCode::AiResponseParseFailed) + ); + assert!(error.to_string().contains("conflicting findings payloads")); } #[test] @@ -3680,6 +3765,56 @@ mod tests { assert_eq!(request_count.load(Ordering::SeqCst), 2); } + #[tokio::test] + async fn missing_final_findings_payload_does_not_retry() { + let request_count = Arc::new(AtomicUsize::new(0)); + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap(); + let request_count_for_server = Arc::clone(&request_count); + tokio::spawn(async move { + loop { + let (mut stream, _) = listener.accept().await.unwrap(); + let request_count = Arc::clone(&request_count_for_server); + tokio::spawn(async move { + request_count.fetch_add(1, Ordering::SeqCst); + let _request = read_http_json_request(&mut stream).await; + let body = r#"{"choices":[{"finish_reason":"stop","message":{"content":" ","tool_calls":[]}}]}"#; + let response = format!( + "HTTP/1.1 200 OK\r\ncontent-type: application/json\r\ncontent-length: {}\r\n\r\n", + body.len() + ); + stream.write_all(response.as_bytes()).await.unwrap(); + stream.write_all(body.as_bytes()).await.unwrap(); + }); + } + }); + + let config = AiReviewConfig { + base_url: format!("http://{addr}"), + ..test_ai_review_config() + }; + let changes = vec![GitLabChange { + old_path: "src/lib.rs".into(), + new_path: "src/lib.rs".into(), + new_file: false, + renamed_file: false, + deleted_file: false, + diff: "@@ -1,0 +1,1 @@\n+panic!();\n".into(), + }]; + + let execution = run_ai_review_execution_with_context(&config, &changes, None, None).await; + + assert_eq!( + execution + .result + .unwrap_err() + .review_failure() + .map(|failure| failure.code), + Some(ReviewErrorCode::AiResponseParseFailed) + ); + assert_eq!(request_count.load(Ordering::SeqCst), 1); + } + #[tokio::test] async fn malformed_findings_recovery_rejects_context_tools_without_third_request() { let request_count = Arc::new(AtomicUsize::new(0)); diff --git a/src/review/ai_tools.rs b/src/review/ai_tools.rs index 382c5ba..d090085 100644 --- a/src/review/ai_tools.rs +++ b/src/review/ai_tools.rs @@ -111,14 +111,14 @@ pub(crate) fn review_findings_tool() -> OpenAiTool { "items": { "type": "object", "properties": { - "path": { "type": "string" }, - "line": { "type": "integer" }, + "path": { "type": "string", "minLength": 1 }, + "line": { "type": "integer", "minimum": 1 }, "severity": { "type": "string", "enum": ["error"] }, - "title": { "type": "string" }, - "message": { "type": "string" } + "title": { "type": "string", "minLength": 1 }, + "message": { "type": "string", "minLength": 1 } }, "required": ["path", "line", "severity", "title", "message"], "additionalProperties": false From 9471de352af8431d9787a9fbf8dfe9c183ccf957 Mon Sep 17 00:00:00 2001 From: SwartzMss Date: Fri, 24 Jul 2026 14:09:04 +0800 Subject: [PATCH 12/19] docs: define bounded findings candidate pipeline --- ...26-07-23-ai-malformed-json-retry-design.md | 51 +++++++++++++++++-- 1 file changed, 47 insertions(+), 4 deletions(-) diff --git a/docs/superpowers/specs/2026-07-23-ai-malformed-json-retry-design.md b/docs/superpowers/specs/2026-07-23-ai-malformed-json-retry-design.md index 209d893..e13a2b9 100644 --- a/docs/superpowers/specs/2026-07-23-ai-malformed-json-retry-design.md +++ b/docs/superpowers/specs/2026-07-23-ai-malformed-json-retry-design.md @@ -14,6 +14,8 @@ assistant content 是被截断的 JSON。当前实现只重试传输、超时和 - 仅在最终 findings 发生 `AiResponseParseFailed` 时执行一次恢复请求。 - 恢复请求不得重放残缺 tool call，也不得形成无限重试。 - 第二次仍失败时保留现有结构化错误行为。 +- 不把缺失、冲突或语义无效的 findings 误判为 clean review。 +- 对响应体和最终候选数量设置统一上限。 ## 非目标 @@ -21,6 +23,7 @@ assistant content 是被截断的 JSON。当前实现只重试传输、超时和 - 不尝试补齐、修剪或猜测残缺 JSON。 - 不改变 HTTP、超时、工具预算以及 diff-only fallback 的既有重试语义。 - 不重跑已经完成的上下文工具调用。 +- 不合并相互冲突的多个最终 findings payload。 ## 设计 @@ -39,6 +42,38 @@ usage 内的 prompt、completion 和 total token 均为可选值。缺失或供 未知或缺失字段记录为空，不作为错误。 +### 有界响应读取 + +AI HTTP 响应体最多读取 4 MiB。超过上限时返回结构化 +`AiResponseParseFailed`，不继续反序列化，也不进入 malformed-finalization +retry。该上限同时约束初始请求和 context follow-up，避免单个超大 payload 在 +HTTP body、OpenAI DTO 和规范化 findings 之间形成内存放大。 + +### 统一最终候选管线 + +最终消息只通过一个有界管线处理： + +1. 按消息顺序发现所有 `submit_review_findings` arguments； +2. 对非空 assistant content，从每个 `{` 独立尝试启动 JSON object 解析； +3. tool 与 content 共用最多 8 个最终候选，超过上限直接返回协议错误； +4. 每个候选独立完成 JSON 结构解析和 finding 语义验证； +5. 收集全部合法候选并进行规范化一致性比较； +6. 没有合法候选时返回最后一个解析错误；多个合法候选不一致时返回 + `AiResponseParseFailed`；全部一致时返回首个候选的原始顺序。 + +content 扫描不在 JSON object 外维护引号或大括号状态。每个 `{` 都是独立且有界 +的解析起点，因此前置残缺引号或未闭合 object 不会吞掉后续合法候选。 + +finding 必须满足： + +- `path`、`title`、`message` 去除空白后非空； +- `line >= 1`； +- `severity` 字段必须存在，且忽略大小写后严格等于 `error`。 + +工具 Schema 同步声明字符串 `minLength: 1`、行号 `minimum: 1` 和唯一支持的 +severity。规范化比较对 findings 的副本按 path、line、title、message 和 severity +排序并去重，避免仅因顺序不同产生冲突；最终输出保留首个候选的原始顺序。 + ### 定向 finalization 重试 最终消息进入 `parse_openai_message` 后，如果错误码是 @@ -46,7 +81,7 @@ usage 内的 prompt、completion 和 total token 均为可选值。缺失或供 1. 丢弃该残缺 assistant 消息，不把它加入对话历史； 2. 保留原始 system/user 消息和已成功完成的工具上下文； -3. 追加一条可信 system 指令，说明上次最终参数不是完整 JSON，要求立即重新调用 +3. 追加一条 `role=system` 的可信指令，说明上次最终参数不是完整 JSON，要求立即重新调用 `submit_review_findings`，压缩描述并放弃证据不足的问题； 4. 关闭 `read_file`、`search_code` 和 `list_files`，只提供 `submit_review_findings`； @@ -60,9 +95,11 @@ usage 内的 prompt、completion 和 total token 均为可选值。缺失或供 ### 错误边界 -只对最终 findings 的结构化解析失败启用该恢复路径。外层 OpenAI 响应无法解析、 -无 choices、无 content、非 2xx、超时和其他错误继续沿用现有处理逻辑，避免扩大 -重试范围或掩盖协议错误。 +只对“至少存在一个最终候选，但所有候选均结构或语义无效”的情况启用该恢复路径。 +外层 OpenAI 响应无法解析、无 choices、无最终候选、候选超限、响应体超限、非 +2xx、超时和其他错误继续沿用现有处理逻辑，避免扩大重试范围或掩盖协议错误。 +多个合法但不一致的候选属于明确冲突，直接返回 `AiResponseParseFailed`，不消耗 +malformed-finalization retry。 ## 测试 @@ -73,5 +110,11 @@ usage 内的 prompt、completion 和 total token 均为可选值。缺失或供 3. 两次最终 findings 均残缺时返回解析错误，总请求次数有明确上限。 4. 恢复请求不包含残缺 assistant tool call，且不再暴露上下文工具。 5. 正常完整响应不产生额外请求。 +6. content 内空与非空合法 JSON 冲突时不返回 clean；语义无效候选后仍可使用合法候选； + 两个相同候选成功。 +7. severity 缺失、空、warning 或未知值均失败；不同 findings 顺序仍视为一致。 +8. tool 与 content 候选共享 8 个上限；前置残缺引号或 object 不阻断后续合法 JSON。 +9. 超过 4 MiB 的初始或 follow-up 响应体被拒绝。 +10. malformed recovery 指令以 system role 发送。 完成后运行格式检查、相关单元测试、完整 `cargo test` 和 Clippy。 From c43a86e4f1510272d5fb662edf46d912945183b3 Mon Sep 17 00:00:00 2001 From: SwartzMss Date: Fri, 24 Jul 2026 14:14:25 +0800 Subject: [PATCH 13/19] docs: plan bounded findings pipeline --- ...-24-bounded-findings-candidate-pipeline.md | 365 ++++++++++++++++++ 1 file changed, 365 insertions(+) create mode 100644 docs/superpowers/plans/2026-07-24-bounded-findings-candidate-pipeline.md diff --git a/docs/superpowers/plans/2026-07-24-bounded-findings-candidate-pipeline.md b/docs/superpowers/plans/2026-07-24-bounded-findings-candidate-pipeline.md new file mode 100644 index 0000000..3a81029 --- /dev/null +++ b/docs/superpowers/plans/2026-07-24-bounded-findings-candidate-pipeline.md @@ -0,0 +1,365 @@ +# Bounded Findings Candidate Pipeline Implementation Plan + +> **For Codex:** REQUIRED SUB-SKILL: Use superpowers:executing-plans to implement this plan task-by-task. + +**Goal:** Prevent false clean reviews and unbounded response processing by parsing every bounded final-findings candidate through one strict pipeline, retrying only genuinely malformed candidates, and preserving trusted recovery instructions. + +**Architecture:** Replace the current “tool calls plus one content payload” parser with a candidate collector that shares one eight-candidate budget across tool arguments and independently discovered JSON objects in assistant content. Each candidate is structurally parsed, semantically validated, and normalized before an order-insensitive consistency check. Return an explicit retry classification so missing payloads, candidate overflow, and protocol failures do not consume malformed recovery. + +**Tech Stack:** Rust, serde/serde_json, ureq, Tokio tests, existing mock HTTP server helpers. + +--- + +### Task 1: Lock down strict finding semantics + +**Files:** +- Modify: `src/review/ai_schema.rs:102-110` +- Modify: `src/review/ai.rs:2160-2200` +- Test: `src/review/ai.rs:2860-2910` + +**Step 1: Write the failing tests** + +Replace the existing unknown-severity coercion test and add table-driven cases asserting `AiResponseParseFailed` for: + +```rust +[ + r#"{"findings":[{"path":"src/lib.rs","line":10,"title":"Bug","message":"Issue"}]}"#, + r#"{"findings":[{"path":"src/lib.rs","line":10,"severity":"","title":"Bug","message":"Issue"}]}"#, + r#"{"findings":[{"path":"src/lib.rs","line":10,"severity":"warning","title":"Bug","message":"Issue"}]}"#, + r#"{"findings":[{"path":"src/lib.rs","line":10,"severity":"garbage","title":"Bug","message":"Issue"}]}"#, +] +``` + +Keep the existing empty path/message/title and zero-line semantic cases. + +**Step 2: Run the focused tests to verify they fail** + +Run: + +```bash +cargo test review::ai::tests::rejects_invalid_ai_finding +cargo test review::ai::tests::rejects_missing_or_unsupported_ai_severity +``` + +Expected: missing/unsupported severity cases are accepted or coerced before the implementation. + +**Step 3: Implement strict severity parsing** + +- Remove `#[serde(default)]` from `AiFinding::severity`. +- Reject any severity that is not equal to `error` ignoring ASCII case. +- Remove `parse_severity`; construct `Severity::Error` only after validation succeeds. +- Keep title validation strict rather than falling back to the configured review title. + +**Step 4: Run focused tests** + +Run the two focused commands from Step 2. + +Expected: PASS. + +**Step 5: Commit** + +```bash +git add src/review/ai.rs src/review/ai_schema.rs +git commit -m "fix: validate final finding severity" +``` + +### Task 2: Parse all bounded tool and content candidates + +**Files:** +- Modify: `src/review/ai.rs:2098-2260` +- Test: `src/review/ai.rs:2780-2900` +- Test: `src/review/ai.rs:3500-3650` + +**Step 1: Write failing parser tests** + +Add tests covering: + +1. Assistant content containing `{"findings":[]}` followed by a non-empty valid findings object returns a conflicting-payload error. +2. A semantically invalid object followed by a valid object returns the valid findings. +3. Two identical valid content objects succeed. +4. An unfinished object prefix followed by valid JSON succeeds. +5. An unfinished quote outside any candidate followed by valid JSON succeeds. +6. Two tool candidates with the same findings in opposite order succeed and preserve the first candidate’s output order. +7. A valid empty tool candidate followed by a non-empty tool candidate returns a conflicting-payload error. + +**Step 2: Run focused parser tests to verify they fail** + +Run: + +```bash +cargo test review::ai::tests::content_candidates +cargo test review::ai::tests::tool_candidates +cargo test review::ai::tests::candidate_order +``` + +Expected: at least the multi-object, malformed-prefix, and order-independent cases fail. + +**Step 3: Introduce a unified candidate pipeline** + +Implement: + +```rust +const MAX_FINAL_FINDINGS_CANDIDATES: usize = 8; + +enum FinalFindingsParseFailure { + Malformed(AppError), + Protocol(AppError), +} +``` + +Then: + +- Count matching `submit_review_findings` calls against the shared budget. +- Discover assistant-content JSON candidates independently from each `{` using a bounded serde deserializer attempt, so an earlier unclosed object or quote cannot own global parser state. +- Count every syntactically complete content object that is submitted to structural parsing against the same budget. +- Parse and semantically validate every candidate independently. +- Keep the last malformed error only when no valid candidate exists. +- Return `Protocol` immediately if total candidates exceeds eight. +- Return `Malformed` for candidate JSON/semantic failures and conflicts. +- Return `Protocol` for no candidate. + +Avoid returning the first structurally valid `AiFindingsResponse`; every discovered object must reach the outer consistency decision. + +**Step 4: Canonicalize only for comparison** + +Build a canonical clone for each valid `Vec`: + +```rust +canonical.sort_by(|left, right| { + ( + &left.path, + left.new_line, + &left.title, + &left.message, + &left.severity, + ) + .cmp(&( + &right.path, + right.new_line, + &right.title, + &right.message, + &right.severity, + )) +}); +canonical.dedup(); +``` + +If `Severity` lacks `Ord`, use a stable explicit severity rank or omit it because accepted AI candidates can only contain `Error`. Compare canonical clones, but return the first valid candidate unchanged. + +**Step 5: Run focused parser tests** + +Run the commands from Step 2. + +Expected: PASS. + +**Step 6: Commit** + +```bash +git add src/review/ai.rs +git commit -m "fix: unify bounded findings candidates" +``` + +### Task 3: Restrict malformed retry to retryable parse failures + +**Files:** +- Modify: `src/review/ai.rs:940-995` +- Test: `src/review/ai.rs:3600-3750` + +**Step 1: Write failing request-count tests** + +Using the existing mock HTTP request counter, add: + +- No content, blank content, or unknown tools without submit content: one request and `AiResponseParseFailed`. +- Nine submit tool candidates: one request and protocol failure. +- A combined total of nine tool/content candidates: one request and protocol failure. +- One malformed candidate followed by a valid recovery response: two requests and success. + +**Step 2: Run focused completion tests to verify they fail** + +Run: + +```bash +cargo test review::ai::tests::does_not_retry_missing_findings_payload +cargo test review::ai::tests::does_not_retry_excess_final_findings_candidates +cargo test review::ai::tests::retries_malformed_findings_candidate_once +``` + +Expected: overflow is not yet classified and malformed/protocol errors are not yet separated. + +**Step 3: Wire explicit failure classification into the completion loop** + +- Remove the separate `has_final_findings_candidate` precheck. +- Retry exactly once only for `FinalFindingsParseFailure::Malformed`. +- Return the contained `AppError` immediately for `Protocol`. +- On a second malformed result, return the error without another request. +- Keep warning logs specific to malformed candidate recovery. + +**Step 4: Run focused tests** + +Run the commands from Step 2. + +Expected: PASS and request counts match. + +**Step 5: Commit** + +```bash +git add src/review/ai.rs +git commit -m "fix: bound malformed findings recovery" +``` + +### Task 4: Make malformed recovery instructions trusted + +**Files:** +- Modify: `src/review/ai.rs:970-985` +- Modify: `src/review/ai.rs:1479-1520` +- Test: `src/review/ai.rs:3650-3750` +- Test: `src/review/ai.rs:5270-5330` + +**Step 1: Write failing role tests** + +Assert that: + +- The initial malformed recovery instruction uses `role == "system"`. +- Timeout reconstruction uses `system` for both `MALFORMED_FINALIZATION_INSTRUCTION` and `MALFORMED_DIFF_ONLY_FINALIZATION_INSTRUCTION`. +- Normal, JSON-content, and ordinary diff-only finalization roles remain unchanged. + +**Step 2: Run focused tests to verify they fail** + +Run: + +```bash +cargo test review::ai::tests::malformed_recovery_instruction_uses_system_role +cargo test review::ai::tests::timeout_finalization_preserves_recovery_role +``` + +Expected: malformed recovery currently uses `user`. + +**Step 3: Implement role selection** + +- Push the direct malformed-recovery `ChatMessage` with `role: "system"`. +- In `request_timeout_finalization`, select both instruction and role. Use `system` exactly when `malformed_retry` is true, including the diff-only combination. +- Do not change compacted tool-evidence messages or ordinary finalization roles. + +**Step 4: Run focused tests** + +Run the commands from Step 2. + +Expected: PASS. + +**Step 5: Commit** + +```bash +git add src/review/ai.rs +git commit -m "fix: trust malformed recovery instructions" +``` + +### Task 5: Limit HTTP response bodies to 4 MiB + +**Files:** +- Modify: `src/review/ai_http.rs:1-12` +- Modify: `src/review/ai_http.rs:175-220` +- Test: `src/review/ai_http.rs` + +**Step 1: Write failing reader tests** + +Extract a reader-level helper and test with `std::io::Cursor`: + +- Exactly 4 MiB succeeds. +- 4 MiB plus one byte returns `AiRequestFailed` with a response-size message. +- A reader error still maps through the existing timeout/non-timeout handling. + +**Step 2: Run focused HTTP tests to verify they fail** + +Run: + +```bash +cargo test review::ai_http::tests::accepts_response_body_at_limit +cargo test review::ai_http::tests::rejects_response_body_over_limit +``` + +Expected: helper/limit does not exist. + +**Step 3: Implement bounded body reading** + +Add: + +```rust +const MAX_AI_RESPONSE_BODY_BYTES: u64 = 4 * 1024 * 1024; +``` + +Read from `response.into_reader().take(MAX_AI_RESPONSE_BODY_BYTES + 1)`, reject if the collected buffer exceeds the limit, and convert with `String::from_utf8`. Preserve timeout mapping for I/O errors and classify oversize/invalid UTF-8 as `AiRequestFailed`. Ensure `is_retryable_ai_error` does not treat the deterministic oversize error as retryable. + +**Step 4: Run focused HTTP tests** + +Run the commands from Step 2. + +Expected: PASS. + +**Step 5: Commit** + +```bash +git add src/review/ai_http.rs +git commit -m "fix: cap AI response body size" +``` + +### Task 6: Full verification and PR update + +**Files:** +- Modify if needed: `docs/superpowers/specs/2026-07-23-ai-malformed-json-retry-design.md` +- Verify: all changed Rust files + +**Step 1: Format and run static checks** + +Run: + +```bash +cargo fmt --check +cargo clippy --all-targets --all-features -- -D warnings +git diff --check +``` + +Expected: PASS. + +**Step 2: Run the full test suite** + +Run: + +```bash +cargo test +``` + +Expected: all unit, binary, and end-to-end tests pass. If sandbox networking prevents local mock-server binds, rerun the same command with the already-approved escalated Cargo test permission. + +**Step 3: Review the complete branch diff** + +Run: + +```bash +git status --short +git diff origin/main...HEAD --stat +git diff origin/main...HEAD -- src/review/ai.rs src/review/ai_schema.rs src/review/ai_http.rs +``` + +Confirm: + +- No false clean path remains for missing/invalid/conflicting findings. +- Candidate count and response size are bounded. +- Only malformed candidates consume recovery. +- Malformed recovery is a system instruction. + +**Step 4: Commit any verification-only corrections** + +```bash +git add src/review/ai.rs src/review/ai_schema.rs src/review/ai_http.rs docs/superpowers/specs/2026-07-23-ai-malformed-json-retry-design.md +git commit -m "test: cover bounded findings recovery" +``` + +Skip this commit if the worktree is already clean. + +**Step 5: Push and update the PR** + +```bash +git push +``` + +Update PR #42’s summary and test evidence to mention strict severity, unified eight-candidate processing, the 4 MiB body cap, and trusted malformed-recovery instructions. From d05374af34bb782b25dea6c79244f98f652b7ec0 Mon Sep 17 00:00:00 2001 From: SwartzMss Date: Fri, 24 Jul 2026 14:18:12 +0800 Subject: [PATCH 14/19] fix: validate final finding severity --- src/review/ai.rs | 53 +++++++++++++++-------------------------- src/review/ai_schema.rs | 1 - 2 files changed, 19 insertions(+), 35 deletions(-) diff --git a/src/review/ai.rs b/src/review/ai.rs index 8718d60..3830d9c 100644 --- a/src/review/ai.rs +++ b/src/review/ai.rs @@ -2157,7 +2157,7 @@ fn has_final_findings_candidate(message: &OpenAiMessage) -> bool { fn parse_openai_findings_payload( review_id: &str, - title: &str, + _title: &str, content: &str, ) -> AppResult> { info!( @@ -2178,6 +2178,7 @@ fn parse_openai_findings_payload( || finding.message.trim().is_empty() || finding.title.trim().is_empty() || finding.line == 0 + || !finding.severity.trim().eq_ignore_ascii_case("error") { return Err(AppError::ai_review( ReviewErrorCode::AiResponseParseFailed, @@ -2186,10 +2187,10 @@ fn parse_openai_findings_payload( } findings.push(Finding { rule_id: format!("ai:{review_id}"), - severity: parse_severity(&finding.severity), + severity: Severity::Error, path: finding.path.trim().replace('\\', "/"), new_line: Some(finding.line), - title: non_empty_or(finding.title, title), + title: finding.title.trim().to_string(), message: finding.message.trim().to_string(), }); } @@ -2282,22 +2283,6 @@ fn telemetry_usage_token(usage: Option<&serde_json::Value>, field: &str) -> Opti .and_then(serde_json::Value::as_u64) } -fn parse_severity(value: &str) -> Severity { - match value.trim().to_ascii_lowercase().as_str() { - "error" => Severity::Error, - _ => Severity::Error, - } -} - -fn non_empty_or(value: String, fallback: &str) -> String { - let trimmed = value.trim(); - if trimmed.is_empty() { - fallback.to_string() - } else { - trimmed.to_string() - } -} - fn preview_log_text(value: &str, max_chars: usize) -> String { let mut preview = String::new(); let mut truncated = false; @@ -2886,21 +2871,21 @@ mod tests { } #[test] - fn treats_unknown_ai_severity_as_error() { - let response = r#" -{ - "choices": [{ - "message": { - "content": "{\"findings\":[{\"path\":\"src/lib.rs\",\"line\":12,\"severity\":\"warning\",\"title\":\"Bug\",\"message\":\"This is a bug.\"}]}" - } - }] -} -"#; - - let findings = parse_openai_response("ai-review", "AI Review", response).unwrap(); - - assert_eq!(findings.len(), 1); - assert_eq!(findings[0].severity, Severity::Error); + fn rejects_missing_or_unsupported_ai_severity() { + for payload in [ + r#"{"findings":[{"path":"src/lib.rs","line":10,"title":"Bug","message":"Issue"}]}"#, + r#"{"findings":[{"path":"src/lib.rs","line":10,"severity":"","title":"Bug","message":"Issue"}]}"#, + r#"{"findings":[{"path":"src/lib.rs","line":10,"severity":"warning","title":"Bug","message":"Issue"}]}"#, + r#"{"findings":[{"path":"src/lib.rs","line":10,"severity":"garbage","title":"Bug","message":"Issue"}]}"#, + ] { + let error = + parse_openai_findings_payload("ai-review", "AI Review", payload).unwrap_err(); + assert_eq!( + error.review_failure().map(|failure| failure.code), + Some(ReviewErrorCode::AiResponseParseFailed), + "payload={payload}" + ); + } } #[test] diff --git a/src/review/ai_schema.rs b/src/review/ai_schema.rs index bef4e0c..c79523f 100644 --- a/src/review/ai_schema.rs +++ b/src/review/ai_schema.rs @@ -102,7 +102,6 @@ pub(crate) struct AiFindingsResponse { pub(crate) struct AiFinding { pub(crate) path: String, pub(crate) line: u32, - #[serde(default)] pub(crate) severity: String, #[serde(default)] pub(crate) title: String, From c84cc5f24387bebabdf49d6c1aea14911359b68a Mon Sep 17 00:00:00 2001 From: SwartzMss Date: Fri, 24 Jul 2026 14:21:09 +0800 Subject: [PATCH 15/19] fix: unify bounded findings candidates --- src/review/ai.rs | 228 ++++++++++++++++++++++++++++++++++------------- 1 file changed, 168 insertions(+), 60 deletions(-) diff --git a/src/review/ai.rs b/src/review/ai.rs index 3830d9c..5e8dad1 100644 --- a/src/review/ai.rs +++ b/src/review/ai.rs @@ -36,7 +36,7 @@ const AI_HTTP_ATTEMPTS: usize = 2; const TOOL_ARGUMENT_SUMMARY_CHARS: usize = 160; const TIMEOUT_EVIDENCE_SUMMARY_CHARS: usize = 6000; const TIMEOUT_EVIDENCE_ITEM_CHARS: usize = 1500; -const MAX_FINDINGS_JSON_CANDIDATES: usize = 8; +const MAX_FINAL_FINDINGS_CANDIDATES: usize = 8; const MIN_CONTEXT_TOOL_RESULT_BYTES: usize = 28; const FINALIZATION_INSTRUCTION: &str = "上下文工具预算已用尽。放弃任何仍缺少证据的候选问题，不得继续请求 read_file、search_code 或 list_files。请立即提交最终审查结果并调用 submit_review_findings；没有已确认问题时提交空 findings。"; const DIFF_ONLY_FINALIZATION_INSTRUCTION: &str = "上下文工具已关闭。请只基于原始 diff 和已明确提供的信息完成审查，不得继续请求 read_file、search_code 或 list_files。放弃任何仍缺少证据的候选问题，并立即调用 submit_review_findings；没有已确认问题时提交空 findings。"; @@ -2102,12 +2102,17 @@ fn parse_final_findings_candidates( ) -> AppResult> { let mut last_error = None; let mut valid_candidates = Vec::new(); + let mut candidate_count = 0_usize; for tool_call in message .tool_calls .iter() .filter(|tool_call| tool_call.function.name == "submit_review_findings") { - match parse_openai_findings_payload(review_id, title, &tool_call.function.arguments) { + candidate_count += 1; + if candidate_count > MAX_FINAL_FINDINGS_CANDIDATES { + return Err(too_many_final_findings_candidates_error()); + } + match parse_single_findings_candidate(review_id, title, &tool_call.function.arguments) { Ok(findings) => valid_candidates.push(findings), Err(error) => last_error = Some(error), } @@ -2118,16 +2123,30 @@ fn parse_final_findings_candidates( .map(str::trim) .filter(|content| !content.is_empty()) { - match parse_openai_findings_payload(review_id, title, content) { - Ok(findings) => valid_candidates.push(findings), - Err(error) => last_error = Some(error), + let content_candidates = extract_json_objects(content); + if content_candidates.is_empty() { + last_error = Some(AppError::ai_review( + ReviewErrorCode::AiResponseParseFailed, + "AI review assistant content contained no complete JSON object", + )); + } + for content_candidate in content_candidates { + candidate_count += 1; + if candidate_count > MAX_FINAL_FINDINGS_CANDIDATES { + return Err(too_many_final_findings_candidates_error()); + } + match parse_single_findings_candidate(review_id, title, content_candidate) { + Ok(findings) => valid_candidates.push(findings), + Err(error) => last_error = Some(error), + } } } if let Some(first) = valid_candidates.first() { + let canonical_first = canonical_findings(first); if valid_candidates .iter() .skip(1) - .all(|candidate| candidate == first) + .all(|candidate| canonical_findings(candidate) == canonical_first) { return Ok(first.clone()); } @@ -2144,6 +2163,37 @@ fn parse_final_findings_candidates( })) } +fn too_many_final_findings_candidates_error() -> AppError { + AppError::ai_review( + ReviewErrorCode::AiResponseParseFailed, + format!( + "AI review returned more than {MAX_FINAL_FINDINGS_CANDIDATES} final findings candidates" + ), + ) +} + +fn canonical_findings(findings: &[Finding]) -> Vec { + let mut canonical = findings.to_vec(); + canonical.sort_by(|left, right| { + ( + &left.path, + left.new_line, + &left.title, + &left.message, + &left.rule_id, + ) + .cmp(&( + &right.path, + right.new_line, + &right.title, + &right.message, + &right.rule_id, + )) + }); + canonical.dedup(); + canonical +} + fn has_final_findings_candidate(message: &OpenAiMessage) -> bool { message .tool_calls @@ -2155,9 +2205,10 @@ fn has_final_findings_candidate(message: &OpenAiMessage) -> bool { .is_some_and(|content| !content.trim().is_empty()) } +#[cfg(test)] fn parse_openai_findings_payload( review_id: &str, - _title: &str, + title: &str, content: &str, ) -> AppResult> { info!( @@ -2167,6 +2218,25 @@ fn parse_openai_findings_payload( "AI review assistant content received" ); let parsed = parse_ai_findings_response(content)?; + normalize_ai_findings(review_id, title, parsed) +} + +fn parse_single_findings_candidate( + review_id: &str, + title: &str, + content: &str, +) -> AppResult> { + let parsed = serde_json::from_str(content).map_err(|error| { + AppError::ai_review(ReviewErrorCode::AiResponseParseFailed, error.to_string()) + })?; + normalize_ai_findings(review_id, title, parsed) +} + +fn normalize_ai_findings( + review_id: &str, + _title: &str, + parsed: AiFindingsResponse, +) -> AppResult> { info!( ai_review_id = %review_id, parsed_findings = parsed.findings.len(), @@ -2197,67 +2267,37 @@ fn parse_openai_findings_payload( Ok(findings) } +#[cfg(test)] fn parse_ai_findings_response(content: &str) -> AppResult { - match serde_json::from_str(content) { - Ok(parsed) => Ok(parsed), - Err(strict_error) => { - let mut last_error = strict_error; - for json_content in extract_json_objects(content, MAX_FINDINGS_JSON_CANDIDATES) { - match serde_json::from_str(json_content) { - Ok(parsed) => return Ok(parsed), - Err(error) => last_error = error, - } - } - Err(AppError::ai_review( - ReviewErrorCode::AiResponseParseFailed, - last_error.to_string(), - )) + let mut last_error = None; + for json_content in extract_json_objects(content) { + match serde_json::from_str(json_content) { + Ok(parsed) => return Ok(parsed), + Err(error) => last_error = Some(error), } } + Err(AppError::ai_review( + ReviewErrorCode::AiResponseParseFailed, + last_error.map_or_else( + || "AI review assistant content contained no complete JSON object".into(), + |error| error.to_string(), + ), + )) } -fn extract_json_objects(content: &str, limit: usize) -> Vec<&str> { +fn extract_json_objects(content: &str) -> Vec<&str> { let mut objects = Vec::new(); - let mut start = None; - let mut depth = 0_usize; - let mut in_string = false; - let mut escaped = false; - for (offset, ch) in content.char_indices() { - if in_string { - if escaped { - escaped = false; - continue; - } - match ch { - '\\' => escaped = true, - '"' => in_string = false, - _ => {} - } + let mut skip_until = 0_usize; + for (start, ch) in content.char_indices() { + if ch != '{' || start < skip_until { continue; } - match ch { - '"' => in_string = true, - '{' => { - if depth == 0 { - start = Some(offset); - } - depth += 1; - } - '}' => { - let Some(next_depth) = depth.checked_sub(1) else { - continue; - }; - depth = next_depth; - if depth == 0 { - if let Some(start) = start.take() { - objects.push(&content[start..offset + ch.len_utf8()]); - if objects.len() == limit { - break; - } - } - } - } - _ => {} + let mut stream = + serde_json::Deserializer::from_str(&content[start..]).into_iter::(); + if matches!(stream.next(), Some(Ok(_))) { + let end = start + stream.byte_offset(); + objects.push(&content[start..end]); + skip_until = end; } } objects @@ -3607,6 +3647,74 @@ mod tests { assert!(error.to_string().contains("conflicting findings payloads")); } + #[test] + fn content_candidates_detect_conflicts_and_skip_invalid_objects() { + let conflicting = r#"{ + "choices":[{"message":{"content": + "{\"findings\":[]}\n{\"findings\":[{\"path\":\"src/lib.rs\",\"line\":10,\"severity\":\"error\",\"title\":\"Bug\",\"message\":\"Real issue\"}]}" + }}] + }"#; + let error = parse_openai_response("ai-review", "AI Review", conflicting).unwrap_err(); + assert!(error.to_string().contains("conflicting findings payloads")); + + let invalid_then_valid = r#"{ + "choices":[{"message":{"content": + "{\"findings\":[{\"path\":\"\",\"line\":10,\"severity\":\"error\",\"title\":\"Invalid\",\"message\":\"\"}]}\n{\"findings\":[{\"path\":\"src/lib.rs\",\"line\":10,\"severity\":\"error\",\"title\":\"Valid\",\"message\":\"Real issue\"}]}" + }}] + }"#; + let findings = parse_openai_response("ai-review", "AI Review", invalid_then_valid).unwrap(); + assert_eq!(findings.len(), 1); + assert_eq!(findings[0].path, "src/lib.rs"); + + let identical = r#"{ + "choices":[{"message":{"content": + "{\"findings\":[]}\n{\"findings\":[]}" + }}] + }"#; + assert!(parse_openai_response("ai-review", "AI Review", identical) + .unwrap() + .is_empty()); + } + + #[test] + fn content_candidates_recover_after_malformed_prefixes() { + for content in [ + "Draft: {\"findings\":[\nFinal:\n{\"findings\":[]}", + "Provider message: \"unfinished quote\n{\"findings\":[]}", + ] { + let message = OpenAiMessage { + content: Some(content.into()), + tool_calls: Vec::new(), + }; + assert!( + parse_openai_message("ai-review", "AI Review", &message) + .unwrap() + .is_empty(), + "content={content}" + ); + } + } + + #[test] + fn candidate_order_is_ignored_but_first_output_order_is_preserved() { + let response = r#"{ + "choices":[{"message":{"tool_calls":[ + {"type":"function","function":{"name":"submit_review_findings","arguments":"{\"findings\":[{\"path\":\"b.rs\",\"line\":2,\"severity\":\"error\",\"title\":\"B\",\"message\":\"Issue B\"},{\"path\":\"a.rs\",\"line\":1,\"severity\":\"error\",\"title\":\"A\",\"message\":\"Issue A\"}]}"}}, + {"type":"function","function":{"name":"submit_review_findings","arguments":"{\"findings\":[{\"path\":\"a.rs\",\"line\":1,\"severity\":\"error\",\"title\":\"A\",\"message\":\"Issue A\"},{\"path\":\"b.rs\",\"line\":2,\"severity\":\"error\",\"title\":\"B\",\"message\":\"Issue B\"}]}"}} + ]}}] + }"#; + + let findings = parse_openai_response("ai-review", "AI Review", response).unwrap(); + + assert_eq!( + findings + .iter() + .map(|finding| finding.path.as_str()) + .collect::>(), + ["b.rs", "a.rs"] + ); + } + #[test] fn reuses_shared_ai_http_client() { let client_one = shared_ai_http_client().unwrap() as *const ureq::Agent; From c560bce8f73151cd8753c649cc3f6ba2063343d0 Mon Sep 17 00:00:00 2001 From: SwartzMss Date: Fri, 24 Jul 2026 14:22:32 +0800 Subject: [PATCH 16/19] fix: bound malformed findings recovery --- src/review/ai.rs | 128 ++++++++++++++++++++++++++++++++++++----------- 1 file changed, 99 insertions(+), 29 deletions(-) diff --git a/src/review/ai.rs b/src/review/ai.rs index 5e8dad1..b970969 100644 --- a/src/review/ai.rs +++ b/src/review/ai.rs @@ -954,15 +954,11 @@ async fn complete_ai_review_response(context: AiReviewCompletion<'_>) -> AppResu "AI review context tool calls completed" ); } - if !has_final_findings_candidate(message) { - return Err(AppError::ai_review( - ReviewErrorCode::AiResponseParseFailed, - "AI review API returned no findings payload", - )); - } match parse_final_findings_candidates(&config.id, &config.title, message) { Ok(findings) => return Ok(findings), - Err(err) if !malformed_finalization_retry_requested => { + Err(FinalFindingsParseFailure::Malformed(err)) + if !malformed_finalization_retry_requested => + { malformed_finalization_retry_requested = true; finalization_requested = true; warn!( @@ -983,7 +979,7 @@ async fn complete_ai_review_response(context: AiReviewCompletion<'_>) -> AppResu tool_calls: None, }); } - Err(err) => return Err(err), + Err(err) => return Err(err.into_error()), } } else { if finalization_requested { @@ -2093,13 +2089,27 @@ fn parse_openai_message( message: &OpenAiMessage, ) -> AppResult> { parse_final_findings_candidates(review_id, title, message) + .map_err(FinalFindingsParseFailure::into_error) +} + +enum FinalFindingsParseFailure { + Malformed(AppError), + Protocol(AppError), +} + +impl FinalFindingsParseFailure { + fn into_error(self) -> AppError { + match self { + Self::Malformed(error) | Self::Protocol(error) => error, + } + } } fn parse_final_findings_candidates( review_id: &str, title: &str, message: &OpenAiMessage, -) -> AppResult> { +) -> Result, FinalFindingsParseFailure> { let mut last_error = None; let mut valid_candidates = Vec::new(); let mut candidate_count = 0_usize; @@ -2110,7 +2120,9 @@ fn parse_final_findings_candidates( { candidate_count += 1; if candidate_count > MAX_FINAL_FINDINGS_CANDIDATES { - return Err(too_many_final_findings_candidates_error()); + return Err(FinalFindingsParseFailure::Protocol( + too_many_final_findings_candidates_error(), + )); } match parse_single_findings_candidate(review_id, title, &tool_call.function.arguments) { Ok(findings) => valid_candidates.push(findings), @@ -2133,7 +2145,9 @@ fn parse_final_findings_candidates( for content_candidate in content_candidates { candidate_count += 1; if candidate_count > MAX_FINAL_FINDINGS_CANDIDATES { - return Err(too_many_final_findings_candidates_error()); + return Err(FinalFindingsParseFailure::Protocol( + too_many_final_findings_candidates_error(), + )); } match parse_single_findings_candidate(review_id, title, content_candidate) { Ok(findings) => valid_candidates.push(findings), @@ -2150,17 +2164,18 @@ fn parse_final_findings_candidates( { return Ok(first.clone()); } - return Err(AppError::ai_review( + return Err(FinalFindingsParseFailure::Malformed(AppError::ai_review( ReviewErrorCode::AiResponseParseFailed, "AI review returned conflicting findings payloads", - )); + ))); } - Err(last_error.unwrap_or_else(|| { - AppError::ai_review( + match last_error { + Some(error) => Err(FinalFindingsParseFailure::Malformed(error)), + None => Err(FinalFindingsParseFailure::Protocol(AppError::ai_review( ReviewErrorCode::AiResponseParseFailed, - "AI review API returned no valid findings payload", - ) - })) + "AI review API returned no findings payload", + ))), + } } fn too_many_final_findings_candidates_error() -> AppError { @@ -2194,17 +2209,6 @@ fn canonical_findings(findings: &[Finding]) -> Vec { canonical } -fn has_final_findings_candidate(message: &OpenAiMessage) -> bool { - message - .tool_calls - .iter() - .any(|tool_call| tool_call.function.name == "submit_review_findings") - || message - .content - .as_deref() - .is_some_and(|content| !content.trim().is_empty()) -} - #[cfg(test)] fn parse_openai_findings_payload( review_id: &str, @@ -3908,6 +3912,72 @@ mod tests { assert_eq!(request_count.load(Ordering::SeqCst), 1); } + #[tokio::test] + async fn excess_final_findings_candidates_do_not_retry() { + let request_count = Arc::new(AtomicUsize::new(0)); + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap(); + let request_count_for_server = Arc::clone(&request_count); + tokio::spawn(async move { + loop { + let (mut stream, _) = listener.accept().await.unwrap(); + let request_count = Arc::clone(&request_count_for_server); + tokio::spawn(async move { + request_count.fetch_add(1, Ordering::SeqCst); + let _request = read_http_json_request(&mut stream).await; + let tool_calls = (0..=MAX_FINAL_FINDINGS_CANDIDATES) + .map(|index| { + serde_json::json!({ + "id": format!("submit_{index}"), + "type": "function", + "function": { + "name": "submit_review_findings", + "arguments": "{\"findings\":[]}" + } + }) + }) + .collect::>(); + let body = serde_json::json!({ + "choices": [{ + "finish_reason": "tool_calls", + "message": {"tool_calls": tool_calls} + }] + }) + .to_string(); + let response = format!( + "HTTP/1.1 200 OK\r\ncontent-type: application/json\r\ncontent-length: {}\r\n\r\n", + body.len() + ); + stream.write_all(response.as_bytes()).await.unwrap(); + stream.write_all(body.as_bytes()).await.unwrap(); + }); + } + }); + + let config = AiReviewConfig { + base_url: format!("http://{addr}"), + ..test_ai_review_config() + }; + let changes = vec![GitLabChange { + old_path: "src/lib.rs".into(), + new_path: "src/lib.rs".into(), + new_file: false, + renamed_file: false, + deleted_file: false, + diff: "@@ -1,0 +1,1 @@\n+panic!();\n".into(), + }]; + + let execution = run_ai_review_execution_with_context(&config, &changes, None, None).await; + let error = execution.result.unwrap_err(); + + assert_eq!( + error.review_failure().map(|failure| failure.code), + Some(ReviewErrorCode::AiResponseParseFailed) + ); + assert!(error.to_string().contains("more than 8")); + assert_eq!(request_count.load(Ordering::SeqCst), 1); + } + #[tokio::test] async fn malformed_findings_recovery_rejects_context_tools_without_third_request() { let request_count = Arc::new(AtomicUsize::new(0)); From a0528c054254f3a0bbdbcb930d1dda8a1816b42a Mon Sep 17 00:00:00 2001 From: SwartzMss Date: Fri, 24 Jul 2026 14:24:02 +0800 Subject: [PATCH 17/19] fix: trust malformed recovery instructions --- src/review/ai.rs | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/src/review/ai.rs b/src/review/ai.rs index b970969..8a9580e 100644 --- a/src/review/ai.rs +++ b/src/review/ai.rs @@ -973,7 +973,7 @@ async fn complete_ai_review_response(context: AiReviewCompletion<'_>) -> AppResu "AI review final findings payload was malformed; retrying finalization once" ); messages.push(ChatMessage { - role: "user".into(), + role: "system".into(), content: Some(MALFORMED_FINALIZATION_INSTRUCTION.into()), tool_call_id: None, tool_calls: None, @@ -1491,8 +1491,9 @@ fn request_timeout_finalization( }); } *requested = true; + let role = if malformed_retry { "system" } else { "user" }; messages.push(ChatMessage { - role: "user".into(), + role: role.into(), content: Some( match ( use_tool_calls, @@ -3769,9 +3770,10 @@ mod tests { .count(); malformed_call_leaked.store(leaked, Ordering::SeqCst); assert!(messages.iter().any(|message| { - message["content"].as_str().is_some_and(|content| { - content.contains("JSON") && content.contains("完整") - }) + message["role"] == "system" + && message["content"].as_str().is_some_and(|content| { + content.contains("JSON") && content.contains("完整") + }) })); r#"{"choices":[{"finish_reason":"tool_calls","message":{"tool_calls":[{"id":"submit_ok","type":"function","function":{"name":"submit_review_findings","arguments":"{\"findings\":[{\"path\":\"src/lib.rs\",\"line\":1,\"severity\":\"error\",\"title\":\"Possible panic\",\"message\":\"Avoid panic here.\"}]}"}}]}}]}"# } @@ -5453,6 +5455,10 @@ mod tests { .and_then(|message| message.content.as_deref()), Some(MALFORMED_FINALIZATION_INSTRUCTION) ); + assert_eq!( + messages.last().map(|message| message.role.as_str()), + Some("system") + ); } #[test] @@ -5482,6 +5488,10 @@ mod tests { assert!(instruction.contains("上一次最终审查结果不是完整 JSON")); assert!(instruction.contains("只基于原始 diff 和已明确提供的信息")); assert!(instruction.contains("不得请求或依赖 read_file、search_code 或 list_files")); + assert_eq!( + messages.last().map(|message| message.role.as_str()), + Some("system") + ); } #[test] From 4adc74e479667791b14df6fb3de59961d9536e00 Mon Sep 17 00:00:00 2001 From: SwartzMss Date: Fri, 24 Jul 2026 14:26:04 +0800 Subject: [PATCH 18/19] fix: cap AI response body size --- src/review/ai_http.rs | 77 ++++++++++++++++++++++++++++++++++++------- 1 file changed, 66 insertions(+), 11 deletions(-) diff --git a/src/review/ai_http.rs b/src/review/ai_http.rs index ecc7c73..61c60b3 100644 --- a/src/review/ai_http.rs +++ b/src/review/ai_http.rs @@ -4,13 +4,15 @@ use crate::{ }; use std::{ error::Error, - io, + io::{self, Read}, sync::OnceLock, thread, time::{Duration, Instant}, }; use tracing::{info, warn, Span}; +const MAX_AI_RESPONSE_BODY_BYTES: u64 = 4 * 1024 * 1024; + pub(crate) static AI_HTTP_CLIENT: OnceLock> = OnceLock::new(); pub(crate) struct AiReviewHttpResponse { @@ -198,7 +200,7 @@ fn perform_ai_review_http_attempt_blocking( "AI review blocking API response headers received" ); let body_started = Instant::now(); - let body = match response.into_string() { + let body = match read_ai_response_body(response.into_reader(), timeout_code) { Ok(body) => body, Err(err) => { warn!( @@ -210,15 +212,7 @@ fn perform_ai_review_http_attempt_blocking( error = %err, "AI review blocking API response body read failed" ); - let code = if is_io_timeout(&err) { - timeout_code - } else { - ReviewErrorCode::AiRequestFailed - }; - return Err(AppError::ai_review( - code, - format!("AI review blocking API response body read failed: {err}"), - )); + return Err(err); } }; info!( @@ -232,6 +226,36 @@ fn perform_ai_review_http_attempt_blocking( Ok(AiReviewHttpResponse { status, body }) } +fn read_ai_response_body(reader: impl Read, timeout_code: ReviewErrorCode) -> AppResult { + let mut bytes = Vec::new(); + reader + .take(MAX_AI_RESPONSE_BODY_BYTES + 1) + .read_to_end(&mut bytes) + .map_err(|error| { + let code = if is_io_timeout(&error) { + timeout_code + } else { + ReviewErrorCode::AiRequestFailed + }; + AppError::ai_review( + code, + format!("AI review blocking API response body read failed: {error}"), + ) + })?; + if bytes.len() as u64 > MAX_AI_RESPONSE_BODY_BYTES { + return Err(AppError::ai_review( + ReviewErrorCode::AiRequestFailed, + format!("AI review API response body exceeded {MAX_AI_RESPONSE_BODY_BYTES} bytes"), + )); + } + String::from_utf8(bytes).map_err(|error| { + AppError::ai_review( + ReviewErrorCode::AiRequestFailed, + format!("AI review API response body was not valid UTF-8: {error}"), + ) + }) +} + fn ureq_response_from_result( result: Result, timeout_code: ReviewErrorCode, @@ -263,3 +287,34 @@ fn is_ureq_timeout(err: &ureq::Error) -> bool { fn is_io_timeout(err: &io::Error) -> bool { err.kind() == io::ErrorKind::TimedOut } + +#[cfg(test)] +mod tests { + use super::*; + use std::io::Cursor; + + #[test] + fn accepts_response_body_at_limit() { + let body = vec![b'a'; MAX_AI_RESPONSE_BODY_BYTES as usize]; + + let parsed = + read_ai_response_body(Cursor::new(body), ReviewErrorCode::AiRequestTimeout).unwrap(); + + assert_eq!(parsed.len() as u64, MAX_AI_RESPONSE_BODY_BYTES); + } + + #[test] + fn rejects_response_body_over_limit() { + let body = vec![b'a'; MAX_AI_RESPONSE_BODY_BYTES as usize + 1]; + + let error = read_ai_response_body(Cursor::new(body), ReviewErrorCode::AiRequestTimeout) + .unwrap_err(); + + assert_eq!( + error.review_failure().map(|failure| failure.code), + Some(ReviewErrorCode::AiRequestFailed) + ); + assert!(error.to_string().contains("exceeded")); + assert!(!is_retryable_ai_error(&error)); + } +} From 251e55bb39e85234b921285bcf7cb8e9ce868501 Mon Sep 17 00:00:00 2001 From: SwartzMss Date: Fri, 24 Jul 2026 14:29:09 +0800 Subject: [PATCH 19/19] test: cover bounded findings recovery --- src/review/ai.rs | 22 ++++++++++++++++++++++ tests/e2e_review.rs | 10 +++++----- 2 files changed, 27 insertions(+), 5 deletions(-) diff --git a/src/review/ai.rs b/src/review/ai.rs index 8a9580e..d6a97c6 100644 --- a/src/review/ai.rs +++ b/src/review/ai.rs @@ -3720,6 +3720,28 @@ mod tests { ); } + #[test] + fn tool_and_content_candidates_share_one_limit() { + let tool_calls = (0..MAX_FINAL_FINDINGS_CANDIDATES) + .map(|index| OpenAiToolCall { + id: format!("submit_{index}"), + call_type: "function".into(), + function: super::super::ai_schema::OpenAiToolCallFunction { + name: "submit_review_findings".into(), + arguments: r#"{"findings":[]}"#.into(), + }, + }) + .collect(); + let message = OpenAiMessage { + content: Some(r#"{"findings":[]}"#.into()), + tool_calls, + }; + + let error = parse_openai_message("ai-review", "AI Review", &message).unwrap_err(); + + assert!(error.to_string().contains("more than 8")); + } + #[test] fn reuses_shared_ai_http_client() { let client_one = shared_ai_http_client().unwrap() as *const ureq::Agent; diff --git a/tests/e2e_review.rs b/tests/e2e_review.rs index a5f66c4..9f21aa8 100644 --- a/tests/e2e_review.rs +++ b/tests/e2e_review.rs @@ -131,7 +131,7 @@ async fn reviews_merge_request_and_records_state() { "findings": [{ "path": "src/lib.rs", "line": 1, - "severity": "warning", + "severity": "error", "title": "Avoid unwrap", "message": "Do not unwrap." }] @@ -237,14 +237,14 @@ async fn continues_publishing_review_comments_after_one_comment_fails() { { "path": "src/lib.rs", "line": 1, - "severity": "warning", + "severity": "error", "title": "AI finding one", "message": "First finding." }, { "path": "src/lib.rs", "line": 2, - "severity": "warning", + "severity": "error", "title": "AI finding two", "message": "Second finding." } @@ -548,7 +548,7 @@ async fn reviews_merge_request_with_ai_review() { "findings": [{ "path": "src/lib.rs", "line": 1, - "severity": "warning", + "severity": "error", "title": "Avoid unwrap", "message": "Handle the None case instead of unwrapping." }] @@ -1983,7 +1983,7 @@ async fn manual_note_runs_ai_review() { "findings": [{ "path": "src/lib.rs", "line": 1, - "severity": "warning", + "severity": "error", "title": "Manual AI finding", "message": "This manual trigger should publish." }]