Plan: AI Loop Regressions (MiniMax, Gemini, Gemini CLI, DeepSeek)

Track: ai_loop_regressions_20260614 Spec: spec.md Status: Active (plan approved 2026-06-14)

TDD Protocol (MANDATORY)

For each phase, the order is:

Red: write the failing test (TDD red phase).
Verify red: run the test; confirm it fails for the right reason.
Green: implement the fix; run the test; confirm it passes.
Verify green: run the full suite to confirm no regression.
Commit: one atomic commit per task with a clear message.

Per the project rule (see AGENTS.md "Critical Anti-Patterns"), the test file must be created BEFORE the implementation. The 1-space indentation rule is in effect (see conductor/product-guidelines.md "AI-Optimized Compact Style").

Phase 1: Root-Cause Verification (TDD Red)

Focus: Write 3 sets of failing tests that reproduce the 3 bugs. Each test must fail for the documented reason (not a typo or import error). All tests committed in separate atomic commits so Tier 2 can verify red → green for each one.

Task 1.1: Create tests/test_ai_loop_regressions_20260614.py with the FR1 test scaffold
- WHERE: tests/test_ai_loop_regressions_20260614.py (new file)
- WHAT: Add the 3 FR1 tests (mock ai_client.send to return "", then assert that event_queue.put("response", ...) was called with status="error" and the error message in the text). Use 1-space indentation. Use existing test fixtures from tests/conftest.py (e.g., mock_app for the controller, vlogger for log capture).
- HOW: Mock ai_client.send_result to return Result(data="", errors=[ErrorInfo(kind=ErrorKind.NETWORK, message="connection refused")]). Call controller._handle_request_event(event). Assert that the event queue received a response entry with status="error" and text containing "connection refused". Assert that _ai_status is f"error: {ui_message}".
- SAFETY: Do not make real network calls; use mocks. The event queue is lock-protected; ensure the test drains it before asserting.
- VERIFY: uv run pytest tests/test_ai_loop_regressions_20260614.py::test_fr1_error_becomes_discussion_entry — should FAIL with AssertionError (current code puts status="done" not status="error").
- COMMIT: test(ai_loop): add FR1 tests for error-becomes-discussion-entry (TDD red)
Task 1.2: Add the FR2 test scaffold
- WHERE: tests/test_ai_loop_regressions_20260614.py (append to existing file)
- WHAT: Add 2 FR2 tests. (a) test_fr2_no_provider_error_in_source — walks the AST of src/app_controller.py and asserts no ProviderError references exist (uses ast module). (b) test_fr2_api_endpoint_handles_send_result_error — calls the /api/v1/generate endpoint with a mock that returns Result(data="", errors=[...]) and asserts it returns a 502 with the error message in the detail field.
- HOW: For (a), use ast.walk on ast.parse(open("src/app_controller.py").read()) and look for ast.Attribute nodes where attr == "ProviderError". For (b), use httpx.AsyncClient or requests with the running FastAPI app, or test the function directly.
- SAFETY: AST scan is read-only; no side effects.
- VERIFY: uv run pytest tests/test_ai_loop_regressions_20260614.py::test_fr2_no_provider_error_in_source — should FAIL with AssertionError (3 references currently exist at lines 305, 313, 3692).
- COMMIT: test(ai_loop): add FR2 tests for dead ProviderError clause removal (TDD red)
Task 1.3: Add the FR3 test scaffold
- WHERE: tests/test_ai_loop_regressions_20260614.py (append to existing file)
- WHAT: Add 2 FR3 tests. (a) test_fr3_minimax_thinking_in_returned_text — mocks _send_minimax's _minimax_client to return a NormalizedResponse with text="actual response" and reasoning_details=[{"text": "thinking content"}]. Calls ai_client._send_minimax(...) and asserts result.data contains <thinking>thinking content</thinking>. (b) test_fr3_minimax_thinking_parsed_by_thinking_parser — calls thinking_parser.parse_thinking_trace(result.data) and asserts 1 segment is found with the expected content.
- HOW: Use unittest.mock.MagicMock to construct a fake OpenAI-compatible client that returns a ChatCompletion object with the reasoning_details attribute. See tests/test_deepseek_provider.py:test_deepseek_reasoner_payload_verification for the existing mock pattern.
- SAFETY: No network calls. The mock's reasoning_details attribute is a list of dicts; the extractor in _send_minimax accesses choice.message.reasoning_details[0].get("text", "").
- VERIFY: uv run pytest tests/test_ai_loop_regressions_20260614.py::test_fr3_minimax_thinking_in_returned_text — should FAIL with AssertionError (current _send_minimax doesn't include thinking tags in result.data).
- COMMIT: test(ai_loop): add FR3 tests for MiniMax thinking-mono rendering (TDD red)
Task 1.4: Verify all 3 test groups fail for the right reason
- Command: uv run pytest tests/test_ai_loop_regressions_20260614.py -v 2>&1 | tee tests/artifacts/ai_loop_regressions_phase1_red.log
- EXPECTED: 7+ tests, all FAILING with the documented reasons (not import errors, not syntax errors, not missing fixtures).
- ACTION: If any test fails for the WRONG reason (e.g., ImportError, SyntaxError, missing fixture), fix the test and re-run before proceeding. Do NOT proceed to Phase 2 with a test that doesn't fail for the documented reason.
- COMMIT: No new commit; this is a verification step.

Phase 2: Fix FR1 (Bug #2 — Error Response Becomes a Discussion Entry)

Focus: Update _handle_request_event in src/app_controller.py:3677-3697 to call send_result() and route errors to the discussion panel. The streaming path is preserved.

Task 2.1: Update _handle_request_event to use send_result() and route errors
- WHERE: src/app_controller.py:3677-3697 (the _handle_request_event method's try block)
- WHAT: Replace ai_client.send(...) with ai_client.send_result(...). Branch on result.ok:
  - If result.ok: existing path — event_queue.put("response", {"text": result.data, "status": "done", "role": "AI"}) + _ai_status = "done".
  - If not result.ok: route the error — pick the highest-severity ErrorInfo (first in result.errors), build ui_message = err.ui_message() (or just err.message if ui_message() doesn't exist on the dataclass — check src/result_types.py for the actual method name; if not present, use a string format like f"[{err.kind.name}] {err.message}"), then event_queue.put("response", {"text": ui_message, "status": "error", "role": "Vendor API"}) + _ai_status = f"error: {ui_message}".
- HOW: Use manual-slop_edit_file with old_string and new_string. Preserve the 1-space indentation. Preserve the streaming behavior — the stream_callback=lambda text: self._on_ai_stream(text) is unchanged; the fix only changes the final return-value handling.
- SAFETY: The _pending_history_adds_lock in _on_comms_entry is unchanged. The thread safety is preserved (the streaming callback runs on the AI client thread; the final result handling runs on the same thread that called send_result).
- REFERENCES: See docs/guide_ai_client.md "Data-Oriented Error Handling > Public API > send_result() migration" for the canonical call shape; see conductor/code_styleguides/error_handling.md §3.1 for the Result-handling pattern.
- VERIFY: uv run pytest tests/test_ai_loop_regressions_20260614.py::test_fr1_error_becomes_discussion_entry tests/test_ai_loop_regressions_20260614.py::test_fr1_success_still_works tests/test_ai_loop_regressions_20260614.py::test_fr1_ai_status_updated — should now PASS.
- COMMIT: fix(ai_loop): route send_result() errors to Discussion Hub as error entries (FR1, Bug #2)
Task 2.2: Add a live_gui regression test for the error path
- WHERE: tests/test_live_gui_ai_loop_error_path.py (new file; small, ~50 lines)
- WHAT: A live_gui-fixture test that mocks ai_client.send_result to return an error result, then triggers a Gen+Send via client.push_event("custom_callback", {"callback": "_handle_generate_send", "args": []}), and polls get_value("disc_entries") until the last entry is an error entry with the expected text.
- HOW: Use the live_gui session-scoped fixture from tests/conftest.py. The ApiHookClient.push_event method is used to trigger the Gen+Send flow. The poll pattern is the standard for _ in range(20): ... if client.get_value("disc_entries")[-1].get("status") == "error": break; time.sleep(0.5) (max 10s).
- SAFETY: Use monkeypatch to inject the mock; do not modify ai_client.send_result directly. Do not pollute other tests' state.
- VERIFY: uv run pytest tests/test_live_gui_ai_loop_error_path.py — should PASS.
- COMMIT: test(ai_loop): add live_gui test for error-becomes-discussion-entry (FR1 verification)
Task 2.3: Verify no regression in other providers
- Command: uv run pytest tests/test_deepseek_provider.py tests/test_ai_client_cli.py tests/test_gemini_cli_integration.py tests/test_gemini_cli_adapter.py 2>&1 | tee tests/artifacts/ai_loop_regressions_phase2_sweep.log
- EXPECTED: All existing tests still pass; no new failures.
- ACTION: If any test fails, STOP and report to the user. Do not attempt a 3rd fix without the user's direction (per AGENTS.md "Process Anti-Patterns #1 — The Deduction Loop").
- COMMIT: No new commit; this is a verification step.

Phase 3: Fix FR2 (Bug #1 — Replace Dead `except ProviderError` Clauses)

Focus: Remove the 3 dead except ai_client.ProviderError clauses in src/app_controller.py:305, 313, 3692. Replace with the new send_result() + if not result.ok: pattern (approach B per user direction).

Task 3.1: Replace the 3 sites in src/app_controller.py
- WHERE: src/app_controller.py:305 (in _api_generate for /api/v1/generate endpoint), src/app_controller.py:313 (in _api_generate_sync for /api/v1/generate_sync endpoint), src/app_controller.py:3692 (in _handle_request_event — but this is the SAME site as Task 2.1; the Phase 2 fix already routes the error correctly, so the Phase 3 work for this site is a no-op or a comment update only).
- WHAT: For sites 305 and 313: change the call to ai_client.send_result(...), branch on result.ok:
  - If not result.ok: raise HTTPException(status_code=502, detail=err.ui_message()) for the API error response.
  - Else: existing return path.
- For site 3692: this was already replaced in Task 2.1; the Phase 3 work is a docstring update to reference the data-oriented error handling styleguide.
- HOW: Use manual-slop_edit_file with old_string and new_string. For each of the 3 sites, replace the try: ... except ai_client.ProviderError as e: ... except Exception as e: ... block with result = ai_client.send_result(...); if not result.ok: err = result.errors[0]; raise HTTPException(status_code=502, detail=err.ui_message()).
- SAFETY: HTTP sites return HTTPException; this is the standard pattern. The _handle_request_event site (3692) was already changed in Phase 2.
- REFERENCES: See docs/guide_app_controller.md for the API endpoint pattern; see conductor/code_styleguides/error_handling.md §3.1 for the Result-handling pattern.
- VERIFY: uv run pytest tests/test_ai_loop_regressions_20260614.py::test_fr2_no_provider_error_in_source tests/test_ai_loop_regressions_20260614.py::test_fr2_api_endpoint_handles_send_result_error — should now PASS.
- VERIFY (AST scan): grep -n "ProviderError" src/app_controller.py — should return no matches.
- COMMIT: fix(ai_loop): replace dead ProviderError except clauses with send_result() pattern (FR2, Bug #1)
Task 3.2: Add a comment / docstring to the _handle_request_event site referencing the styleguide
- WHERE: src/app_controller.py:_handle_request_event (the function docstring or a comment at the FR1-fix site)
- WHAT: Add a one-line reference to the data-oriented error handling styleguide, e.g.:
```
# FR2 / Bug #1: per conductor/code_styleguides/error_handling.md §3.1 (AND over OR),
# we check result.ok instead of catching a ProviderError exception.
```
- HOW: Use manual-slop_edit_file to add the comment after the result = ai_client.send_result(...) line.
- SAFETY: Comments are minimal per the project's no-comments rule (see conductor/product-guidelines.md); this one is justified because it documents a non-obvious architectural decision.
- VERIFY: grep -n "AND over OR" src/app_controller.py — should return 1 match.
- COMMIT: Same commit as 3.1; no new commit.
Task 3.3: Verify all FR2 tests pass and no other tests regress
- Command: uv run pytest tests/test_ai_loop_regressions_20260614.py tests/test_ai_client_result.py tests/test_deprecation_warnings.py 2>&1 | tee tests/artifacts/ai_loop_regressions_phase3_sweep.log
- EXPECTED: All FR2 tests PASS; existing test_ai_client_result.py and test_deprecation_warnings.py still pass (they were already updated for the Result API).
- COMMIT: No new commit; this is a verification step.

Phase 4: Fix FR3 (Bug #3 — MiniMax Thinking Mono Rendering)

Focus: Wrap reasoning_content in <thinking>...</thinking> tags in the returned text, mirroring DeepSeek's pattern at src/ai_client.py:2117-2118.

Task 4.1: Implement the thinking-wrap in run_with_tool_loop (preferred) or _send_minimax
- WHERE: src/ai_client.py:797-836 (run_with_tool_loop body) — preferred location because it's a shared helper and the fix benefits any provider that uses reasoning_extractor (currently MiniMax and Llama llama-3.1-405b-reasoning). Alternative: src/ai_client.py:2418-2443 (_send_minimax body) — only fixes MiniMax.
- WHAT: In run_with_tool_loop, after the for _round_idx in range(MAX_TOOL_ROUNDS + 2): loop, BEFORE returning response_text, check if reasoning_content is non-empty. If yes, wrap it in <thinking>...</thinking> tags and prepend to response_text. Alternatively, set response_text = f"<thinking>\n{reasoning_content}\n</thinking>\n\n{response_text}" at the END of each round.
- HOW: Use manual-slop_edit_file with old_string and new_string. The change is ~3 lines.
- SAFETY: DeepSeek ALREADY does this wrap inline (at lines 2117-2118). The fix here is for the OTHER providers that use reasoning_extractor (MiniMax, Llama). The fix must be conditional — it should NOT overwrite DeepSeek's existing wrap (which is already there). Check the existing code: DeepSeek's full_assistant_text = thinking_tags + assistant_text is set BEFORE the response is added to history. The run_with_tool_loop does NOT know about this; it only sees response.text. So the fix needs to be in the run_with_tool_loop's response_text return — but only for providers that haven't already wrapped.
- CLEANEST APPROACH: Add a new keyword argument wrap_reasoning_in_text: bool = False to run_with_tool_loop (default False to preserve existing behavior for providers that wrap inline). In _send_minimax, pass wrap_reasoning_in_text=caps.reasoning (True when reasoning is enabled). In run_with_tool_loop, when wrap_reasoning_in_text and reasoning_content, prepend f"<thinking>\n{reasoning_content}\n</thinking>\n\n" to response_text at the end of each round.
- REFERENCES: See src/ai_client.py:2117-2118 for DeepSeek's pattern. See src/thinking_parser.py:9 for the regex that will match the <thinking> tag.
- VERIFY: uv run pytest tests/test_ai_loop_regressions_20260614.py::test_fr3_minimax_thinking_in_returned_text tests/test_ai_loop_regressions_20260614.py::test_fr3_minimax_thinking_parsed_by_thinking_parser — should now PASS.
- VERIFY (DeepSeek not regressed): uv run pytest tests/test_deepseek_provider.py — all tests should still pass (DeepSeek's inline wrap happens BEFORE the run_with_tool_loop sees the response, so the new wrap_reasoning_in_text is unused).
- COMMIT: fix(ai_loop): wrap MiniMax reasoning in <thinking> tags for parse_thinking_trace (FR3, Bug #3)
Task 4.2: Verify MiniMax wrap is conditional and other providers unaffected
- Command: uv run pytest tests/test_deepseek_provider.py tests/test_llama_provider.py tests/test_grok_provider.py tests/test_qwen_provider.py tests/test_anthropic_provider.py 2>&1 | tee tests/artifacts/ai_loop_regressions_phase4_sweep.log
- EXPECTED: All existing tests pass. The 13 regressions from the parent track's public_api_migration_20260606 may still be present (out of scope; deferred to that track).
- COMMIT: No new commit; this is a verification step.
Task 4.3: Add a live_gui regression test for MiniMax thinking-mono rendering
- WHERE: tests/test_live_gui_minimax_thinking.py (new file; small, ~60 lines)
- WHAT: A live_gui-fixture test that mocks the MiniMax client to return reasoning content, triggers a Gen+Send, and polls get_value("disc_entries") for an entry with a non-empty thinking_segments field.
- HOW: Use the same pattern as tests/test_live_gui_ai_loop_error_path.py (Task 2.2). The poll target is the last disc_entries entry's thinking_segments list (not status).
- SAFETY: Mock injection via monkeypatch.
- VERIFY: uv run pytest tests/test_live_gui_minimax_thinking.py — should PASS.
- COMMIT: test(ai_loop): add live_gui test for MiniMax thinking-mono rendering (FR3 verification)

Phase 5: Regression Sweep + Documentation

Focus: Full test suite sweep, doc note for the 2 deferred follow-ups.

Task 5.1: Run the full test suite
- Command: uv run pytest tests/ 2>&1 | tee tests/artifacts/ai_loop_regressions_phase5_full_suite.log
- EXPECTED: All tests pass. The 13 pre-existing regressions from data_oriented_error_handling_20260606 (test_llama_provider.py: 3, test_llama_ollama_native.py: 4, test_grok_provider.py: 3, test_minimax_provider.py: 2, test_live_gui_integration_v2.py: 1) may still be present — these are the planned work of public_api_migration_20260606, not this track.
- ACTION: If NEW failures appear (not in the 13 pre-existing), STOP and report to the user. Do not attempt a fix without the user's direction.
- COMMIT: No new commit; this is a verification step.
Task 5.2: Add the 2 follow-up notes to docs/guide_ai_client.md
- WHERE: docs/guide_ai_client.md "See Also" section (or the equivalent end-of-doc section)
- WHAT: Add 3 new bullets:
  1. Gemini / Gemini CLI thinking-format compatibility (deferred from ai_loop_regressions_20260614) — the user's complaint included Gemini; the likely cause is a format mismatch between the Gemini SDK output and parse_thinking_trace. Empirically investigate by running a Gemini request that produces reasoning and inspecting the raw resp.text. See conductor/tracks/ai_loop_regressions_20260614/spec.md §13.1.
  2. <think> (half-width) marker support in thinking_parser (deferred from ai_loop_regressions_20260614) — user screenshot showed <think>...</think> format; current parse_thinking_trace requires <thinking>. The change is small (~3 lines in src/thinking_parser.py:9). See conductor/tracks/ai_loop_regressions_20260614/spec.md §13.2.
  3. Public API Result Migration (planned, separate track public_api_migration_20260606) — the 5 production + 63 test call sites not migrated in this track.
- HOW: Use manual-slop_edit_file with the existing "See Also" section as the anchor.
- COMMIT: docs(ai_client): add 2 follow-up notes for ai_loop_regressions_20260614 (Gemini thinking, <think> marker)
Task 5.3: Update metadata.json to mark the track complete
- WHERE: conductor/tracks/ai_loop_regressions_20260614/metadata.json
- WHAT: Change "status": "active" to "status": "completed". Update verification_criteria to reflect what was actually verified.
- HOW: Direct file edit.
- COMMIT: conductor(track): mark ai_loop_regressions_20260614 as completed
Task 5.4: Conductor — User Manual Verification (Protocol in workflow.md)
- Action: Announce the track is complete. Provide the user with the acceptance test from spec.md §12. Briefly summarize the 3 fixes and the 2 deferred follow-ups.

Summary

Total tasks: 17 (across 5 phases)
Total commits: ~14 (1 test scaffold + 3 red test commits + 3 fix commits + 2 live_gui test commits + 1 doc commit + 1 metadata commit + 3 verification steps with no commit)
Total estimated effort: 1-2 days of Tier 2 work
Dependencies: None (independent track; no blocked_by)
Follow-up tracks: 2 deferred investigations (Gemini thinking format, <think> half-width marker) + 1 planned track (public_api_migration_20260606)

21 KiB Raw Blame History