ed/manual_slop
Private
Public Access
0
0
Fork 0
Code Activity

conductor(track): plan for doeh_test_thinking_cleanup_20260615 (TDD-style, 5 phases, 16 tasks)

Browse Source
This commit is contained in:
ed
2026-06-15 12:20:06 -04:00
parent 925e366cdd
commit 304f469663
1 changed files with 251 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files
conductor/tracks/doeh_test_thinking_cleanup_20260615/plan.md
+251
View File
@@ -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 <thinking> wrap" OR "Gemini SDK outputs thinking as <thought>...</thought> tags; parser needs extension" OR "Gemini SDK already wraps in <thinking>; 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 `<thinking>...</thinking>` 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 `<think>` Half-Width Marker Support (G15)
**Focus:** Extend `parse_thinking_trace` to also match the half-width `<think>...</think>` 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 `<think>` to the alternation in the existing `tag_pattern`. The current regex is:
```python
tag_pattern = re.compile(r'<(thinking|thought)>(.*?)</\1>', re.DOTALL | re.IGNORECASE)
```
Extend to:
```python
tag_pattern = re.compile(r'<(thinking|thought|think)>(.*?)</\1>', re.DOTALL | re.IGNORECASE)
```
The closing `</think>` 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 `<thinking>` and `<thought>` still match).
- **COMMIT:** `fix(thinking_parser): add <think> (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("<think>thinking content</think>\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 <think> 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