diff --git a/conductor/tracks/ai_loop_regressions_20260614/plan.md b/conductor/tracks/ai_loop_regressions_20260614/plan.md new file mode 100644 index 00000000..d7104d90 --- /dev/null +++ b/conductor/tracks/ai_loop_regressions_20260614/plan.md @@ -0,0 +1,189 @@ +# 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: +1. **Red**: write the failing test (TDD red phase). +2. **Verify red**: run the test; confirm it fails for the right reason. +3. **Green**: implement the fix; run the test; confirm it passes. +4. **Verify green**: run the full suite to confirm no regression. +5. **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 content`. (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.: + ```python + # 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 `...` 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 `...` tags and prepend to `response_text`. Alternatively, set `response_text = f"\n{reasoning_content}\n\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"\n{reasoning_content}\n\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 `` 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 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. **`` (half-width) marker support in thinking_parser (deferred from `ai_loop_regressions_20260614`)** — user screenshot showed `...` format; current `parse_thinking_trace` requires ``. 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, 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, `` half-width marker) + 1 planned track (`public_api_migration_20260606`)