From 304f4696634f13bc94b44d9945d0bdbd7d6f6026 Mon Sep 17 00:00:00 2001 From: Ed_ Date: Mon, 15 Jun 2026 12:20:06 -0400 Subject: [PATCH] conductor(track): plan for doeh_test_thinking_cleanup_20260615 (TDD-style, 5 phases, 16 tasks) --- .../plan.md | 251 ++++++++++++++++++ 1 file changed, 251 insertions(+) create mode 100644 conductor/tracks/doeh_test_thinking_cleanup_20260615/plan.md diff --git a/conductor/tracks/doeh_test_thinking_cleanup_20260615/plan.md b/conductor/tracks/doeh_test_thinking_cleanup_20260615/plan.md new file mode 100644 index 00000000..ffe2c9b8 --- /dev/null +++ b/conductor/tracks/doeh_test_thinking_cleanup_20260615/plan.md @@ -0,0 +1,251 @@ +# Plan: Data-Oriented Error Handling Test & Thinking-Parser Cleanup + +**Track:** `doeh_test_thinking_cleanup_20260615` +**Spec:** `spec.md` +**Status:** Active (plan approved 2026-06-15) + +## TDD Protocol (MANDATORY) + +For each phase, the order is: +1. **Red**: verify the test/failure is present (TDD red phase — for Phase 1, the failure is already in the test suite; for Phase 2, the 11 tests are already red). +2. **Green**: implement the fix; run the test; confirm it passes. +3. **Verify green**: run the full suite to confirm no regression. +4. **Commit**: one atomic commit per task with a clear message. + +Per the project rule (see `AGENTS.md` "Critical Anti-Patterns"), per-task atomic commits. The 1-space indentation rule is in effect (see `conductor/product-guidelines.md` "AI-Optimized Compact Style"). + +--- + +## Phase 1: CRITICAL — Fix `_api_generate` NameError (G1) + +**Focus:** Restore the `context_to_send` variable definition that the `ai_loop_regressions_20260614` FR2 fix accidentally removed. This is a production bug that breaks `/api/v1/generate` for all callers. + +- [ ] **Task 1.1**: Verify the NameError is reproducible + - **Command:** `uv run pytest tests/test_headless_service.py::TestHeadlessAPI::test_generate_endpoint -v 2>&1 | tee tests/artifacts/doeh_cleanup_phase1_red.log` + - **EXPECTED:** 500 error with `NameError: name 'context_to_send' is not defined` at `src/app_controller.py:278` + - **NOTE:** This is the existing canary test — no new test needed. + - **COMMIT:** No new commit; this is a verification step. + +- [ ] **Task 1.2**: Fix `_api_generate` by adding back the missing `context_to_send` definition + - **WHERE:** `src/app_controller.py:265-295` (the `_api_generate` function) + - **WHAT:** Add 2-3 lines BEFORE the `result = ai_client.send_result(...)` call at line 278. The added block is: + ```python + with controller._disc_entries_lock: + has_ai_response = any(e.get("role") == "AI" for e in controller.disc_entries) + context_to_send = stable_md if not has_ai_response else "" + ``` + - **HOW:** Use `manual-slop_edit_file` with `old_string` (the existing `result = ai_client.send_result(context_to_send, ...)` line) and `new_string` (the 2-line block + the `result = ...` line). The 1-space indentation rule is in effect. + - **SAFETY:** The added lines preserve the original logic from before the FR2 fix. The `_disc_entries_lock` is the same lock the original code used; no new race condition. + - **REFERENCES:** See `docs/guide_app_controller.md` "AI Loop Lifecycle" section for the canonical pattern. + - **VERIFY:** `uv run pytest tests/test_headless_service.py::TestHeadlessAPI::test_generate_endpoint -v` returns 200. + - **COMMIT:** `fix(app_controller): restore context_to_send definition in _api_generate (CRITICAL regression from ai_loop_regressions_20260614)` + +- [ ] **Task 1.3**: Verify no regression in the other _api_generate and _handle_request_event paths + - **Command:** `uv run pytest tests/test_headless_service.py tests/test_api_read_endpoints.py tests/test_api_control_endpoints.py -v 2>&1 | tee tests/artifacts/doeh_cleanup_phase1_sweep.log` + - **EXPECTED:** All other headless service tests pass (test_health_endpoint, test_status_endpoint_*, test_pending_actions_endpoint, test_confirm_action_endpoint, test_list_sessions_endpoint, test_get_context_endpoint). + - **COMMIT:** No new commit; this is a verification step. + +--- + +## Phase 2: Fix 10 Test Mock Bugs (G2-G12) + 1 Mock Shape Fix (G13) + 1 Headless Service Test (G14) + +**Focus:** Mechanical fixes for the 11 pre-existing test mock bugs introduced by the `data_oriented_error_handling_20260606` refactor. Each fix is 1-2 lines. + +### 2A: test_grok_provider.py (3 fixes: G3, G4, G5) + +- [ ] **Task 2.1**: Fix `test_send_grok_uses_xai_endpoint` (G3) + - **WHERE:** `tests/test_grok_provider.py:13-23` + - **WHAT:** Change `assert result == "hi from grok"` to `assert result.ok and result.data == "hi from grok"`. + - **HOW:** Use `manual-slop_edit_file` with `old_string` and `new_string`. 1-space indentation. + - **VERIFY:** `uv run pytest tests/test_grok_provider.py::test_send_grok_uses_xai_endpoint` passes. + - **COMMIT:** `test(grok): adapt test_send_grok_uses_xai_endpoint to Result API (doeh cleanup)` + +- [ ] **Task 2.2**: Fix `test_grok_web_search_adds_search_parameters_to_extra_body` (G4) + - **WHERE:** `tests/test_grok_provider.py:30-44` + - **WHAT:** Change `assert len(captured_kwargs) == 1` and `captured_kwargs[0]["extra_body"]` to check across all kwargs with `any()`. The tool loop calls the mock multiple times. + - **HOW:** Use `manual-slop_edit_file`. Change: + ```python + assert len(captured_kwargs) == 1 + eb = captured_kwargs[0]["extra_body"] + ``` + to: + ```python + assert any(kw.get("extra_body") is not None and kw["extra_body"].get("search_parameters", {}).get("mode") == "auto" for kw in captured_kwargs), f"web_search extra_body not found in {captured_kwargs}" + ``` + - **VERIFY:** `uv run pytest tests/test_grok_provider.py::test_grok_web_search_adds_search_parameters_to_extra_body` passes. + - **COMMIT:** `test(grok): adapt test_grok_web_search to multi-call tool loop (doeh cleanup)` + +- [ ] **Task 2.3**: Fix `test_grok_x_search_adds_x_source_to_extra_body` (G5) + - **WHERE:** `tests/test_grok_provider.py:46-57` + - **WHAT:** Same pattern as Task 2.2 — change `captured_kwargs[0]["extra_body"]["search_parameters"]["sources"]` to `any()` across all kwargs. + - **HOW:** Same as Task 2.2. + - **VERIFY:** `uv run pytest tests/test_grok_provider.py::test_grok_x_search_adds_x_source_to_extra_body` passes. + - **COMMIT:** `test(grok): adapt test_grok_x_search to multi-call tool loop (doeh cleanup)` + +### 2B: test_llama_provider.py (3 fixes: G5, G6, G7) + +- [ ] **Task 2.4**: Fix `test_send_llama_openrouter_backend` (G5) and `test_send_llama_custom_url` (G6) and `test_send_llama_ollama_backend` (G7) + - **WHERE:** `tests/test_llama_provider.py:24-29, 43-49, 62-67` + - **WHAT:** For each, change the assertion pattern to handle `Result[str]`: + - `assert result == "hi from openrouter"` → `assert result.ok and result.data == "hi from openrouter"` + - `assert result == "hi from custom"` → `assert result.ok and result.data == "hi from custom"` + - `assert "hi from ollama" in result` → `assert result.ok and "hi from ollama" in result.data` + - **HOW:** Use `manual-slop_edit_file` per test. + - **VERIFY:** `uv run pytest tests/test_llama_provider.py` all 3 pass. + - **COMMIT:** `test(llama): adapt 3 tests to Result API (doeh cleanup)` + +### 2C: test_llama_ollama_native.py (4 fixes: G8, G9, G10, G11) + +- [ ] **Task 2.5**: Fix all 4 tests in `test_llama_ollama_native.py` + - **WHERE:** `tests/test_llama_ollama_native.py:70-83, 88-99, 107-117, 122-134` + - **WHAT:** For each, change `assert "text" in result` to `assert result.ok and "text" in result.data`. + - **HOW:** Use `manual-slop_edit_file` per test. + - **VERIFY:** `uv run pytest tests/test_llama_ollama_native.py` all 4 pass. + - **COMMIT:** `test(llama_native): adapt 4 tests to Result API (doeh cleanup)` + +### 2D: test_ai_client_tool_loop_builder.py (1 fix: G12) + +- [ ] **Task 2.6**: Fix the mock shape to return `Result[NormalizedResponse]` (G12) + - **WHERE:** `tests/test_ai_client_tool_loop_builder.py:33` + - **WHAT:** Wrap the mock's return values in `Result(data=...)`. The current `side_effect=[tool_response, final]` returns raw `NormalizedResponse`, but `_default_send` now does `if not res.ok:` expecting `Result[NormalizedResponse]`. + - **HOW:** Use `manual-slop_edit_file`. Add `from src.result_types import Result` to imports, then change: + ```python + patch("src.openai_compatible.send_openai_compatible", side_effect=[tool_response, final]) + ``` + to: + ```python + patch("src.openai_compatible.send_openai_compatible", side_effect=[Result(data=tool_response), Result(data=final)]) + ``` + - **VERIFY:** `uv run pytest tests/test_ai_client_tool_loop_builder.py` passes. + - **COMMIT:** `test(ai_client_tool_loop): adapt mock to return Result[NormalizedResponse] (doeh cleanup)` + +### 2E: test_headless_service.py (1 fix: G14) + +- [ ] **Task 2.7**: Fix `test_generate_endpoint` mock to use `send_result` (G14) + - **WHERE:** `tests/test_headless_service.py:57-63` + - **WHAT:** Change `patch('src.ai_client.send', return_value="AI Response")` to `patch('src.ai_client.send_result', return_value=Result(data="AI Response"))`. Add `from src.result_types import Result` if not already imported. + - **HOW:** Use `manual-slop_edit_file`. + - **NOTE:** This test will only pass after Phase 1's G1 fix is in place. The Task ordering is: G1 first (Phase 1), then G14 (this task). + - **VERIFY:** `uv run pytest tests/test_headless_service.py::TestHeadlessAPI::test_generate_endpoint` returns 200. + - **COMMIT:** `test(headless_service): adapt test_generate_endpoint to send_result (doeh cleanup)` + +### 2F: Phase 2 verification + +- [ ] **Task 2.8**: Verify all 11 fixes pass together + - **Command:** `uv run pytest tests/test_grok_provider.py tests/test_llama_provider.py tests/test_llama_ollama_native.py tests/test_ai_client_tool_loop_builder.py tests/test_headless_service.py -v 2>&1 | tee tests/artifacts/doeh_cleanup_phase2_sweep.log` + - **EXPECTED:** All 11 previously-failing tests now pass. + - **COMMIT:** No new commit; this is a verification step. + +--- + +## Phase 3: Fix Gemini / Gemini CLI Thinking-Format Compatibility (G14) + +**Focus:** Empirical investigation of the Gemini SDK's thinking output format. Decide between a normalization pass in `_send_gemini*` and a parser extension in `parse_thinking_trace`. + +- [ ] **Task 3.1**: Empirically investigate the Gemini SDK output format + - **APPROACH:** + 1. Read `src/ai_client.py:_send_gemini` (lines 1538-1781) to understand how `resp.text` is built. + 2. Read `src/ai_client.py:_send_gemini_cli` (lines 1783-1897) to understand the CLI adapter output. + 3. If a real Gemini API key is available, run a Gemini request that produces reasoning and inspect `resp.text`. If not, read the google-genai SDK docs to determine the format. + 4. Document the finding in the commit message (e.g., "Gemini SDK outputs thinking as plain text before the response; needs wrap" OR "Gemini SDK outputs thinking as ... tags; parser needs extension" OR "Gemini SDK already wraps in ; the issue is elsewhere"). + - **OUTPUT:** A 1-paragraph finding in the commit message. + - **COMMIT:** No new commit; this is an investigation step. + +- [ ] **Task 3.2**: Implement the fix based on the investigation + - **WHERE:** Either `src/ai_client.py:_send_gemini`, `src/ai_client.py:_send_gemini_cli`, OR `src/thinking_parser.py:9` + - **WHAT:** Based on the finding, apply one of: + - **Option A (normalization)**: Add a normalization pass that wraps thinking content in `...` tags before returning from `_send_gemini*`. This is the same pattern as DeepSeek (line 2117-2118) and MiniMax (added in `ai_loop_regressions_20260614`). + - **Option B (parser extension)**: Extend the `tag_pattern` regex in `src/thinking_parser.py:9` to match the new format. + - **HOW:** Use `manual-slop_edit_file`. The change is small (~5-10 lines). + - **VERIFY:** A new test (in `tests/test_gemini_thinking_format.py` or added to an existing test) demonstrates the fix. + - **COMMIT:** `fix(ai_client): normalize Gemini thinking output format for parse_thinking_trace (doeh cleanup)` OR `fix(thinking_parser): extend regex to match Gemini output format (doeh cleanup)` + +- [ ] **Task 3.3**: Add a regression test for the Gemini thinking fix + - **WHERE:** `tests/test_gemini_thinking_format.py` (new file) or an addition to `tests/test_gemini_cli_integration.py` + - **WHAT:** Mock a Gemini response with thinking content, run through the new pipeline, assert `parse_thinking_trace` extracts 1 ThinkingSegment. + - **HOW:** Use `MagicMock` for the Gemini client. Follow the pattern in `tests/test_ai_loop_regressions_20260614.py::test_fr3_minimax_thinking_in_returned_text`. + - **VERIFY:** `uv run pytest tests/test_gemini_thinking_format.py` passes. + - **COMMIT:** `test(gemini): add regression test for thinking-format fix (doeh cleanup)` + +--- + +## Phase 4: Add `` Half-Width Marker Support (G15) + +**Focus:** Extend `parse_thinking_trace` to also match the half-width `...` form (the closing tag is the same). Small change. + +- [ ] **Task 4.1**: Extend the `tag_pattern` regex + - **WHERE:** `src/thinking_parser.py:9` + - **WHAT:** Add `` to the alternation in the existing `tag_pattern`. The current regex is: + ```python + tag_pattern = re.compile(r'<(thinking|thought)>(.*?)', re.DOTALL | re.IGNORECASE) + ``` + Extend to: + ```python + tag_pattern = re.compile(r'<(thinking|thought|think)>(.*?)', re.DOTALL | re.IGNORECASE) + ``` + The closing `` matches because the regex uses backreference `\1` which matches the captured tag. + - **HOW:** Use `manual-slop_edit_file`. + - **VERIFY:** Run existing `tests/test_thinking_trace.py` — all 5+ tests still pass (the existing tags `` and `` still match). + - **COMMIT:** `fix(thinking_parser): add (half-width) marker support (doeh cleanup)` + +- [ ] **Task 4.2**: Add 1+ new tests for the half-width marker + - **WHERE:** `tests/test_thinking_trace.py` (existing file) + - **WHAT:** Add `test_parse_half_width_think_tag` that asserts `parse_thinking_trace("thinking content\n\nresponse")` returns 1 segment with the right content and the response stripped. + - **HOW:** Use `manual-slop_edit_file`. Follow the existing test style in the file. + - **VERIFY:** `uv run pytest tests/test_thinking_trace.py` — all 5+ existing + 1 new test pass. + - **COMMIT:** `test(thinking_trace): add test for half-width marker (doeh cleanup)` + +--- + +## Phase 5: Housekeeping + Regression Sweep + Docs (G16, G17, FR8) + +**Focus:** Clean up the state.toml duplicate-key bug, update tracks.md, run the full suite, update the docs. + +- [ ] **Task 5.1**: Fix `state.toml` duplicate keys (G16) + - **WHERE:** `conductor/tracks/ai_loop_regressions_20260614/state.toml` lines 23-26 and 46-58 + - **WHAT:** Delete the duplicate "pending" entries for `phase_2..5` and `t2_1..t5_4`. Keep the "completed" entries with the actual commit SHAs at lines 18-22 and 29-45. + - **HOW:** Use `manual-slop_edit_file`. Delete lines 23-26 (4 lines: phase_2, phase_3, phase_4, phase_5 pending) and lines 46-58 (13 lines: t2_1..t5_4 pending). + - **VERIFY:** `uv run python -c "import tomllib; tomllib.load(open('conductor/tracks/ai_loop_regressions_20260614/state.toml','rb'))"` succeeds (no `TOMLDecodeError`). + - **COMMIT:** `conductor(state): fix duplicate keys in ai_loop_regressions_20260614 state.toml` + +- [ ] **Task 5.2**: Update `tracks.md` row 24 to reflect completion (G17) + - **WHERE:** `conductor/tracks.md:41` + - **WHAT:** Update the status column to reflect the track's completion on 2026-06-15. Either: + - **Option A (status column update)**: Change `spec ✓, plan ✓, ready to start` to `spec ✓, plan ✓, shipped 2026-06-15 (doeh_test_thinking_cleanup tracks 2 followups)`. + - **Option B (move to recently completed)**: Move the row to a "Recently Completed (post-Phase 8)" section. This is the more consistent pattern. + - **HOW:** Use `manual-slop_edit_file`. Recommend Option B for consistency. + - **VERIFY:** `git diff conductor/tracks.md` shows the change. + - **COMMIT:** `conductor: mark ai_loop_regressions_20260614 as completed in tracks.md (blocks archival)` + +- [ ] **Task 5.3**: Run the full test suite + - **Command:** `uv run pytest tests/ 2>&1 | tee tests/artifacts/doeh_cleanup_phase5_full_suite.log` + - **EXPECTED:** All tests pass. The 2 UI Polish tests (`test_discussion_truncate_layout`, `test_log_management_refresh`) may still fail (out of scope). The RAG test (`test_rag_phase4_final_verify`) may still fail (pre-existing). All other tests should be green. + - **ACTION:** If NEW failures appear (not in the known-out-of-scope list), STOP and report to the user. + - **COMMIT:** No new commit; this is a verification step. + +- [ ] **Task 5.4**: Add 2 cross-references to `docs/guide_ai_client.md` "See Also" section (FR8) + - **WHERE:** `docs/guide_ai_client.md` "See Also" section + - **WHAT:** Add 2 new bullets: + 1. **`doeh_test_thinking_cleanup_20260615` (this track)** — fixed the `_api_generate` NameError regression and 11 pre-existing test mock bugs from the data_oriented_error_handling refactor. + 2. **Public API Result Migration (planned, separate track `public_api_migration_20260606`)** — removes the deprecated `ai_client.send()` and migrates the remaining 5 production + ~50 test call sites to `send_result()`. + - **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 doeh_test_thinking_cleanup_20260615` + +- [ ] **Task 5.5**: Update `metadata.json` to mark the track complete + - **WHERE:** `conductor/tracks/doeh_test_thinking_cleanup_20260615/metadata.json` + - **WHAT:** Change `"status": "active"` to `"status": "completed"`. Add `"completed_at": "2026-06-15"` (or the actual completion date). Update `verification_criteria` to reflect what was actually verified. + - **HOW:** Direct file edit. + - **COMMIT:** `conductor(track): mark doeh_test_thinking_cleanup_20260615 as completed` + +- [ ] **Task 5.6**: Conductor — User Manual Verification (Protocol in workflow.md) + - **ACTION:** Announce the track is complete. Provide the user with a summary of the 18 fixes (1 critical + 11 test mock + 2 deferred bug + 4 housekeeping) and note the 4 deferred items (§12.1-12.4 in spec.md). + +--- + +## Summary + +- **Total tasks:** 16 (across 5 phases) +- **Total commits:** ~15 (1 critical fix + 6 test mock fixes + 1 gemini fix + 1 gemini test + 1 thinking regex + 1 thinking test + 1 state.toml + 1 tracks.md + 1 docs + 1 metadata + 4 verification steps with no commit) +- **Total estimated effort:** 5-8 hours of Tier 2 work (0.5-1 day) +- **Dependencies:** None (independent track; no `blocked_by`) +- **Out of scope (noted in spec §12)**: public_api_migration, live_gui_mock_injection, RAG flakiness, UI Polish phases