Private
Public Access
archive: doeh test thinking cleanup
This commit is contained in:
@@ -1,326 +0,0 @@
|
||||
{
|
||||
"track_id": "doeh_test_thinking_cleanup_20260615",
|
||||
"name": "Data-Oriented Error Handling Test & Thinking-Parser Cleanup",
|
||||
"initialized": "2026-06-15",
|
||||
"owner": "tier2-tech-lead",
|
||||
"priority": "high",
|
||||
"status": "completed",
|
||||
"type": "bugfix + test_cleanup + refactor + documentation",
|
||||
"scope": {
|
||||
"new_files": [
|
||||
"tests/test_gemini_thinking_format.py"
|
||||
],
|
||||
"modified_files": [
|
||||
"src/app_controller.py",
|
||||
"src/ai_client.py",
|
||||
"src/thinking_parser.py",
|
||||
"tests/test_llama_provider.py",
|
||||
"tests/test_llama_ollama_native.py",
|
||||
"tests/test_grok_provider.py",
|
||||
"tests/test_ai_client_tool_loop_builder.py",
|
||||
"tests/test_headless_service.py",
|
||||
"tests/test_thinking_trace.py",
|
||||
"conductor/tracks/ai_loop_regressions_20260614/state.toml",
|
||||
"conductor/tracks.md",
|
||||
"docs/guide_ai_client.md"
|
||||
]
|
||||
},
|
||||
"blocked_by": [],
|
||||
"blocks": [],
|
||||
"estimated_phases": 5,
|
||||
"spec": "spec.md",
|
||||
"plan": "plan.md",
|
||||
|
||||
"regressions_and_deferred_items": [
|
||||
{
|
||||
"id": "G1_api_generate_name_error",
|
||||
"severity": "CRITICAL",
|
||||
"category": "production_regression",
|
||||
"introduced_by": "ai_loop_regressions_20260614 commit 2b7b571a (FR2 fix)",
|
||||
"file_line": "src/app_controller.py:265-295",
|
||||
"symptom": "/api/v1/generate returns HTTP 500 with NameError: name 'context_to_send' is not defined",
|
||||
"fix_phase": 1,
|
||||
"fix_size_lines": 3,
|
||||
"fix": "Add back the 2 lines that were removed: with controller._disc_entries_lock: has_ai_response = ... and context_to_send = stable_md if not has_ai_response else ''"
|
||||
},
|
||||
{
|
||||
"id": "G2_grok_uses_xai_endpoint",
|
||||
"severity": "high",
|
||||
"category": "test_mock_bug",
|
||||
"introduced_by": "data_oriented_error_handling_20260606 commit 64b787b8 (ProviderError removal + _send_* rename)",
|
||||
"file_line": "tests/test_grok_provider.py:13",
|
||||
"fix_phase": 2,
|
||||
"fix": "Change `assert result == 'hi from grok'` to `assert result.ok and result.data == 'hi from grok'`"
|
||||
},
|
||||
{
|
||||
"id": "G3_grok_web_search",
|
||||
"severity": "high",
|
||||
"category": "test_mock_bug",
|
||||
"introduced_by": "data_oriented_error_handling_20260606 (tool loop refactor)",
|
||||
"file_line": "tests/test_grok_provider.py:30",
|
||||
"symptom": "captured_kwargs has 12 entries instead of 1 (tool loop calls multiple times)",
|
||||
"fix_phase": 2,
|
||||
"fix": "Change `assert len(captured_kwargs) == 1` and `captured_kwargs[0][...]` to check across all kwargs with any()"
|
||||
},
|
||||
{
|
||||
"id": "G4_grok_x_search",
|
||||
"severity": "high",
|
||||
"category": "test_mock_bug",
|
||||
"introduced_by": "data_oriented_error_handling_20260606 (tool loop refactor)",
|
||||
"file_line": "tests/test_grok_provider.py:46",
|
||||
"fix_phase": 2,
|
||||
"fix": "Same as G3 — change captured_kwargs[0] to any() across all kwargs"
|
||||
},
|
||||
{
|
||||
"id": "G5_llama_openrouter",
|
||||
"severity": "high",
|
||||
"category": "test_mock_bug",
|
||||
"introduced_by": "data_oriented_error_handling_20260606 (Result API)",
|
||||
"file_line": "tests/test_llama_provider.py:24",
|
||||
"fix_phase": 2,
|
||||
"fix": "Change `assert result == 'hi from openrouter'` to `assert result.ok and result.data == 'hi from openrouter'`"
|
||||
},
|
||||
{
|
||||
"id": "G6_llama_custom_url",
|
||||
"severity": "high",
|
||||
"category": "test_mock_bug",
|
||||
"introduced_by": "data_oriented_error_handling_20260606 (Result API)",
|
||||
"file_line": "tests/test_llama_provider.py:43",
|
||||
"fix_phase": 2,
|
||||
"fix": "Same as G5"
|
||||
},
|
||||
{
|
||||
"id": "G7_llama_ollama_backend",
|
||||
"severity": "high",
|
||||
"category": "test_mock_bug",
|
||||
"introduced_by": "data_oriented_error_handling_20260606 (Result API)",
|
||||
"file_line": "tests/test_llama_provider.py:62",
|
||||
"fix_phase": 2,
|
||||
"fix": "Change `assert 'hi from ollama' in result` to `assert result.ok and 'hi from ollama' in result.data`"
|
||||
},
|
||||
{
|
||||
"id": "G8_llama_native_calls_ollama_chat",
|
||||
"severity": "high",
|
||||
"category": "test_mock_bug",
|
||||
"introduced_by": "data_oriented_error_handling_20260606 (Result API)",
|
||||
"file_line": "tests/test_llama_ollama_native.py:70",
|
||||
"fix_phase": 2,
|
||||
"fix": "Same as G7"
|
||||
},
|
||||
{
|
||||
"id": "G9_llama_native_preserves_thinking",
|
||||
"severity": "high",
|
||||
"category": "test_mock_bug",
|
||||
"introduced_by": "data_oriented_error_handling_20260606 (Result API)",
|
||||
"file_line": "tests/test_llama_ollama_native.py:88",
|
||||
"fix_phase": 2,
|
||||
"fix": "Same as G7"
|
||||
},
|
||||
{
|
||||
"id": "G10_llama_routes_to_native",
|
||||
"severity": "high",
|
||||
"category": "test_mock_bug",
|
||||
"introduced_by": "data_oriented_error_handling_20260606 (Result API)",
|
||||
"file_line": "tests/test_llama_ollama_native.py:107",
|
||||
"fix_phase": 2,
|
||||
"fix": "Same as G7"
|
||||
},
|
||||
{
|
||||
"id": "G11_llama_keeps_openai_path",
|
||||
"severity": "high",
|
||||
"category": "test_mock_bug",
|
||||
"introduced_by": "data_oriented_error_handling_20260606 (Result API)",
|
||||
"file_line": "tests/test_llama_ollama_native.py:122",
|
||||
"fix_phase": 2,
|
||||
"fix": "Same as G7"
|
||||
},
|
||||
{
|
||||
"id": "G12_ai_client_tool_loop_builder",
|
||||
"severity": "high",
|
||||
"category": "test_mock_shape_bug",
|
||||
"introduced_by": "data_oriented_error_handling_20260606 commit 3aa7bdca (NormalizedResponse return shape)",
|
||||
"file_line": "tests/test_ai_client_tool_loop_builder.py:33",
|
||||
"symptom": "_default_send does `if not res.ok:` expecting Result[NormalizedResponse]; mock returns raw NormalizedResponse",
|
||||
"fix_phase": 2,
|
||||
"fix": "Wrap the mock return in Result(data=...) — Result(data=tool_response), Result(data=final)"
|
||||
},
|
||||
{
|
||||
"id": "G13_headless_service_test_generate",
|
||||
"severity": "high",
|
||||
"category": "test_mock_bug",
|
||||
"introduced_by": "data_oriented_error_handling_20260606 (Result API)",
|
||||
"file_line": "tests/test_headless_service.py:57",
|
||||
"symptom": "Mocks ai_client.send (deprecated); production now uses send_result. Test returns 500 due to G1 NameError + mock mismatch.",
|
||||
"fix_phase": 2,
|
||||
"fix": "Change `patch('src.ai_client.send', return_value='AI Response')` to `patch('src.ai_client.send_result', return_value=Result(data='AI Response'))`; update assertion to use .data"
|
||||
},
|
||||
{
|
||||
"id": "G14_gemini_thinking_format",
|
||||
"severity": "medium",
|
||||
"category": "deferred_bug",
|
||||
"introduced_by": "pre-existing limitation (not from data_oriented_error_handling refactor)",
|
||||
"file_line": "src/ai_client.py:_send_gemini (lines 1538-1781), _send_gemini_cli (lines 1783-1897)",
|
||||
"symptom": "User complained that thinking monologues don't render for Gemini requests",
|
||||
"fix_phase": 3,
|
||||
"fix": "Empirical investigation: run a Gemini request that produces thinking, inspect resp.text, decide between (a) normalization pass in _send_gemini* or (b) extend parse_thinking_trace"
|
||||
},
|
||||
{
|
||||
"id": "G15_think_half_width_marker",
|
||||
"severity": "low",
|
||||
"category": "deferred_bug",
|
||||
"introduced_by": "pre-existing limitation (not from data_oriented_error_handling refactor)",
|
||||
"file_line": "src/thinking_parser.py:9",
|
||||
"symptom": "User screenshot 1 showed <think>...</think> format (half-width); current regex requires <thinking> (full-width)",
|
||||
"fix_phase": 4,
|
||||
"fix": "Extend the tag_pattern regex at line 9 to also match <think>...</think>"
|
||||
},
|
||||
{
|
||||
"id": "G16_state_toml_duplicates",
|
||||
"severity": "low",
|
||||
"category": "housekeeping",
|
||||
"introduced_by": "ai_loop_regressions_20260614 commit 01075222",
|
||||
"file_line": "conductor/tracks/ai_loop_regressions_20260614/state.toml lines 23-26 and 46-58",
|
||||
"symptom": "Python's tomllib.load() raises TOMLDecodeError: Cannot overwrite a value",
|
||||
"fix_phase": 5,
|
||||
"fix": "Delete the duplicate pending entries; keep only the completed entries with commit SHAs"
|
||||
},
|
||||
{
|
||||
"id": "G17_tracks_md_row_24",
|
||||
"severity": "low",
|
||||
"category": "housekeeping",
|
||||
"introduced_by": "ai_loop_regressions_20260614 (track shipped but tracks.md not updated)",
|
||||
"file_line": "conductor/tracks.md:41",
|
||||
"symptom": "Track row still says 'spec ✓, plan ✓, ready to start' though the track shipped on 2026-06-15",
|
||||
"fix_phase": 5,
|
||||
"fix": "Update status column or move to Recently Completed section"
|
||||
}
|
||||
],
|
||||
|
||||
"deferred_to_followup_tracks": [
|
||||
{
|
||||
"id": "public_api_migration_20260606",
|
||||
"title": "Public API Result Migration",
|
||||
"description": "Removes the deprecated ai_client.send() and migrates the remaining 5 production call sites + ~50 test call sites to send_result(). This track handles 11 of the 63 tests; the other ~50 are deferred.",
|
||||
"blocks_field_in_tracks_md": true,
|
||||
"track_status": "planned; not yet specced"
|
||||
},
|
||||
{
|
||||
"id": "live_gui_mock_injection_20260615",
|
||||
"title": "Live GUI Mock Injection Infrastructure",
|
||||
"description": "Infrastructure for mock injection into the live_gui subprocess. Unblocks proper end-to-end live_gui + AI client tests (the ai_loop_regressions_20260614 smoke tests only verify Hook API substrate reachability).",
|
||||
"blocks_field_in_tracks_md": false,
|
||||
"track_status": "recommended; not yet specced"
|
||||
},
|
||||
{
|
||||
"id": "test_rag_phase4_final_verify_fix",
|
||||
"title": "test_rag_phase4_final_verify RAG flakiness fix",
|
||||
"description": "Pre-existing RAG subsystem issue ('NoneType' object has no attribute 'get'). The error is in RAG config lookup code, not AI client code. A partial fix was attempted in commit 16412ad5 (RAG Phase 4 dim-mismatch recovery). Recommended as a separate RAG track.",
|
||||
"blocks_field_in_tracks_md": false,
|
||||
"track_status": "pre-existing; not caused by either data_oriented_error_handling or ai_loop_regressions tracks"
|
||||
},
|
||||
{
|
||||
"id": "ui_polish_five_issues_20260302",
|
||||
"title": "UI Polish Five Issues",
|
||||
"description": "The 2 unrelated test failures (test_discussion_truncate_layout, test_log_management_refresh) are Phase 2 and Phase 3 of the UI Polish track. That track has its own spec and plan.",
|
||||
"blocks_field_in_tracks_md": true,
|
||||
"track_status": "ready to start; spec/plan in place; not caused by data_oriented_error_handling refactor"
|
||||
}
|
||||
],
|
||||
|
||||
"verification_criteria": {
|
||||
"g1_api_generate_returns_200": "uv run pytest tests/test_headless_service.py::TestHeadlessAPI::test_generate_endpoint returns 200 (proves G1 fix)",
|
||||
"g2_g12_test_mock_fixes_pass": "Full batched test suite has 11 fewer failures than the pre-track baseline (G2-G12)",
|
||||
"g13_tool_loop_builder_passes": "uv run pytest tests/test_ai_client_tool_loop_builder.py::test_run_with_tool_loop_calls_request_builder_each_round passes",
|
||||
"g14_headless_service_test_passes": "uv run pytest tests/test_headless_service.py::TestHeadlessAPI::test_generate_endpoint returns 200 (after G1 + G13 fixes)",
|
||||
"g15_gemini_thinking_format_investigated": "Phase 3 produces an empirical finding (either normalization pass in _send_gemini* or parser extension) + live_gui or unit test demonstrates the fix",
|
||||
"g16_half_width_marker_supported": "tests/test_thinking_trace.py has 1+ new test for <think>...</think> marker; all existing tests still pass",
|
||||
"g17_state_toml_parseable": "python -c 'import tomllib; tomllib.load(open(\"conductor/tracks/ai_loop_regressions_20260614/state.toml\",\"rb\"))' succeeds",
|
||||
"g18_tracks_md_row_24_updated": "Row 24 in conductor/tracks.md reflects the track's completion (status column or section move)",
|
||||
"full_suite_green": "uv run pytest tests/ shows no new failures beyond the deferred test_rag_phase4_final_verify and the 2 UI Polish tests",
|
||||
"docs_updated": "docs/guide_ai_client.md 'See Also' section has 2 new cross-references: (1) this cleanup track; (2) public_api_migration_20260606"
|
||||
},
|
||||
|
||||
"fr_to_phase_mapping": {
|
||||
"FR1_fix_api_generate_name_error": {
|
||||
"phase": 1,
|
||||
"fix_files": ["src/app_controller.py:265-295"],
|
||||
"test_files": ["tests/test_headless_service.py::TestHeadlessAPI::test_generate_endpoint"],
|
||||
"min_test_count": 1
|
||||
},
|
||||
"FR2_FR3_test_mock_fixes": {
|
||||
"phase": 2,
|
||||
"fix_files": [
|
||||
"tests/test_llama_provider.py",
|
||||
"tests/test_llama_ollama_native.py",
|
||||
"tests/test_grok_provider.py",
|
||||
"tests/test_ai_client_tool_loop_builder.py",
|
||||
"tests/test_headless_service.py"
|
||||
],
|
||||
"min_test_count": 11
|
||||
},
|
||||
"FR4_gemini_thinking_format": {
|
||||
"phase": 3,
|
||||
"fix_files": ["src/ai_client.py:_send_gemini", "src/ai_client.py:_send_gemini_cli", "src/thinking_parser.py:9"],
|
||||
"test_files": ["tests/test_gemini_thinking_format.py (new)"],
|
||||
"min_test_count": 1
|
||||
},
|
||||
"FR5_think_half_width_marker": {
|
||||
"phase": 4,
|
||||
"fix_files": ["src/thinking_parser.py:9"],
|
||||
"test_files": ["tests/test_thinking_trace.py"],
|
||||
"min_test_count": 1
|
||||
},
|
||||
"FR6_state_toml_cleanup": {
|
||||
"phase": 5,
|
||||
"fix_files": ["conductor/tracks/ai_loop_regressions_20260614/state.toml"],
|
||||
"min_test_count": 0
|
||||
},
|
||||
"FR7_tracks_md_update": {
|
||||
"phase": 5,
|
||||
"fix_files": ["conductor/tracks.md"],
|
||||
"min_test_count": 0
|
||||
},
|
||||
"FR8_regression_sweep_and_docs": {
|
||||
"phase": 5,
|
||||
"fix_files": ["docs/guide_ai_client.md"],
|
||||
"min_test_count": 0
|
||||
}
|
||||
},
|
||||
|
||||
"estimated_effort": {
|
||||
"phase_1": "10 min — 1 critical regression fix + 1 test verification",
|
||||
"phase_2": "1.5 hours — 11 mechanical test mock fixes across 5 files",
|
||||
"phase_3": "2-4 hours — empirical Gemini investigation + fix (uncertain duration depending on finding)",
|
||||
"phase_4": "30 min — 1 regex extension + 1+ new test",
|
||||
"phase_5": "1 hour — 4 housekeeping tasks (state.toml, tracks.md, sweep, docs)",
|
||||
"total": "5-8 hours of Tier 2 work (0.5-1 day)"
|
||||
},
|
||||
|
||||
"risk_register": {
|
||||
"R1_api_generate_fix_breaks_fr2_fr3": {
|
||||
"likelihood": "low",
|
||||
"impact": "high",
|
||||
"mitigation": "Fix only ADDS lines; doesn't modify existing logic. Function semantics match pre-ai_loop_regressions_20260614 state."
|
||||
},
|
||||
"R2_test_mock_fixes_introduce_subtle_failures": {
|
||||
"likelihood": "low",
|
||||
"impact": "low",
|
||||
"mitigation": "Pattern is mechanical (assert result.ok then assert result.data); failure messages are clear if a test has a real bug"
|
||||
},
|
||||
"R3_gemini_investigation_needs_real_credentials": {
|
||||
"likelihood": "medium",
|
||||
"impact": "medium",
|
||||
"mitigation": "Use a mock client that returns a realistic Gemini response with thinking content if real credentials unavailable; document the format assumption"
|
||||
},
|
||||
"R4_think_regex_greedy": {
|
||||
"likelihood": "low",
|
||||
"impact": "low",
|
||||
"mitigation": "Use re.DOTALL + non-greedy .*? (consistent with existing pattern); existing 5+ tests catch regressions"
|
||||
},
|
||||
"R5_state_toml_cleanup_deletes_wrong_lines": {
|
||||
"likelihood": "very_low",
|
||||
"impact": "high",
|
||||
"mitigation": "Only delete the duplicate 'pending' entries; the 'completed' entries with commit SHAs must be preserved. Fix is mechanical and verifiable by re-running tomllib.load()"
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -1,251 +0,0 @@
|
||||
# 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.
|
||||
|
||||
- [x] **Task 1.1**: Verify the NameError is reproducible [7b323e3]
|
||||
- **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.
|
||||
|
||||
- [x] **Task 1.2**: Fix `_api_generate` by adding back the missing `context_to_send` definition [7b323e3]
|
||||
- **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)`
|
||||
|
||||
- [x] **Task 1.3**: Verify no regression in the other _api_generate and _handle_request_event paths [7b323e3]
|
||||
- **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
|
||||
@@ -1,277 +0,0 @@
|
||||
# Track: Data-Oriented Error Handling Test & Thinking-Parser Cleanup
|
||||
|
||||
**Status:** Active (spec approved 2026-06-15)
|
||||
**Initialized:** 2026-06-15
|
||||
**Owner:** Tier 2 Tech Lead
|
||||
**Priority:** High (1 critical production regression + 10+ test mock fixes + 2 deferred bugs)
|
||||
|
||||
---
|
||||
|
||||
## 1. Overview
|
||||
|
||||
This track is the **cleanup follow-up** to two previously-completed tracks: `data_oriented_error_handling_20260606` (shipped 2026-06-12) and `ai_loop_regressions_20260614` (shipped 2026-06-15). It consolidates 3 categories of remaining work into a single deliverable:
|
||||
|
||||
1. **A new production regression** introduced by `ai_loop_regressions_20260614` commit `2b7b571a` (FR2 fix): the `_api_generate` function in `src/app_controller.py:265-295` references an undefined variable `context_to_send`, causing `/api/v1/generate` to return HTTP 500 on every call. This bug was not caught by the previous track's smoke tests (which only verified Hook API substrate reachability) and was missed in the Tier 1 review (which relied on the test pass count, not direct code inspection of the FR2 diff).
|
||||
|
||||
2. **10 pre-existing test mock bugs** from the `data_oriented_error_handling_20260606` refactor: tests that call `_send_<vendor>()` and assert against raw `str` return values, while the production code now returns `Result[str]`. Mechanical fixes (`assert result.ok and result.data == "x"` instead of `assert result == "x"`).
|
||||
|
||||
3. **2 deferred bugs** from `ai_loop_regressions_20260614` spec §13: Gemini / Gemini CLI thinking-format compatibility (Bug #4) and `<think>` (half-width) marker support in `thinking_parser` (Bug #5).
|
||||
|
||||
Plus 2 housekeeping items discovered during Tier 1 review of `ai_loop_regressions_20260614`: the duplicate-key bug in that track's `state.toml` (which makes the file unparseable by Python's `tomllib`), and the `tracks.md` row 24 that was never updated to mark the track complete.
|
||||
|
||||
This track does NOT include (deferred to separate tracks — see §13):
|
||||
- The `public_api_migration_20260606` follow-up (5 production + 63 test call sites not migrated to `send_result()`)
|
||||
- A `live_gui_mock_injection` infrastructure track (would unblock proper end-to-end live_gui + AI client tests)
|
||||
- Pre-existing RAG flakiness (`test_rag_phase4_final_verify`)
|
||||
- The UI Polish Five Issues phases (2 unrelated test failures covered by that track)
|
||||
|
||||
## 2. Goals (Priority Order)
|
||||
|
||||
| Priority | Goal | Rationale |
|
||||
|---|---|---|
|
||||
| **A (critical)** | Fix the `_api_generate` `NameError` regression introduced by `ai_loop_regressions_20260614` commit `2b7b571a` | Production bug: `/api/v1/generate` returns HTTP 500 on every call. The fix is small (~3 lines: add back the `_disc_entries_lock` acquisition and `context_to_send = stable_md if not has_ai_response else ""`). A failing test (`test_headless_service.test_generate_endpoint`) is the canary. |
|
||||
| **A (primary value)** | Fix the 10 pre-existing test mock bugs from `data_oriented_error_handling_20260606` | The test suite has 10+ red tests that are all the same mechanical pattern. Fixing them gets the test suite back to green. Each test is a 1-line change (use `result.data` or `result.ok` checks). |
|
||||
| **B (architectural)** | Investigate and fix the Gemini / Gemini CLI thinking-format compatibility (Bug #4) | The user complained that thinking monologues don't render for Gemini. Empirical investigation needed: run a Gemini request, inspect `resp.text`, determine if a normalization pass is needed in `_send_gemini*`. |
|
||||
| **B (architectural)** | Add `<think>` (half-width) marker support to `thinking_parser.py` | User screenshot 1 showed `<think>...</think>` format. The current regex at `src/thinking_parser.py:9` requires the full-width `<thinking>`. Small change (~3 lines + tests). |
|
||||
| **C (housekeeping)** | Fix the `state.toml` duplicate-key bug in `ai_loop_regressions_20260614` | The state file is unparseable by Python's `tomllib` due to TOML §3.3.1 "Cannot overwrite a value". The fix is deleting lines 23-26 and 46-58. This blocks archival of the parent track. |
|
||||
| **C (housekeeping)** | Update `conductor/tracks.md` row 24 to reflect completion of `ai_loop_regressions_20260614` | The track was completed on 2026-06-15 but the row still says "spec ✓, plan ✓, ready to start". |
|
||||
| **C (verification)** | Full test suite sweep + `docs/guide_ai_client.md` "See Also" section update | Document the new `Result` API test patterns and the deferred items. |
|
||||
|
||||
### 2.1 Non-Goals (this track)
|
||||
|
||||
- **Not** migrating the remaining 5 production + 63 test call sites to `send_result()`. That is `public_api_migration_20260606`, a separate planned track with its own scope. This track only fixes the broken `_api_generate` site (which is the only newly-introduced production regression) and the 10+ tests that would be touched by the public_api migration.
|
||||
- **Not** introducing a `live_gui_mock_injection` infrastructure. That's a separate concern (test infrastructure) requiring subprocess mock injection. Recommended as its own track.
|
||||
- **Not** fixing the pre-existing RAG flakiness in `test_rag_phase4_final_verify`. That test had a partial fix in commit `16412ad5` (RAG Phase 4 dim-mismatch) and a subsequent failure with `'NoneType' object has no attribute 'get'`. This is a RAG subsystem concern, not an AI client test mock concern.
|
||||
- **Not** fixing `test_discussion_truncate_layout.py::test_keep_pairs_input_uses_adequate_width` and `test_log_management_refresh.py::test_refresh_registry_button_calls_load_registry`. These are Phase 2 and Phase 3 of the UI Polish Five Issues track, which has its own plan and spec. The 2 failing tests are correctly identified as out-of-scope here.
|
||||
- **Not** adding a CI gate or audit script. The existing `scripts/audit_*.py` scripts don't check for this category of regression (test mocks that don't match the new return types).
|
||||
- **Not** removing the deprecated `ai_client.send()` shim. That's `public_api_migration_20260606`.
|
||||
|
||||
## 3. Current State Audit (as of commit `515ef933`)
|
||||
|
||||
### 3.1 Already Implemented (DO NOT re-implement)
|
||||
|
||||
- **`src/result_types.py`**: `Result[T]`, `ErrorInfo`, `ErrorKind` dataclasses exist; the new convention is fully established.
|
||||
- **`src/ai_client.py:send_result()`** (lines 2970-3092): the new public entry point, returns `Result[str]`. Routes to `_send_<vendor>_result()` per provider.
|
||||
- **`src/ai_client.py:send()`** (lines 2907-2968): the `@deprecated` shim, returns `result.data` (empty string on error).
|
||||
- **`src/ai_client.py:_send_*_result()`** (9 vendors): all return `Result[str]`.
|
||||
- **`src/ai_client.py:run_with_tool_loop()`** (lines 734-836): now has `wrap_reasoning_in_text: bool = False` kwarg (added by `ai_loop_regressions_20260614` FR3 fix).
|
||||
- **`src/app_controller.py:_handle_request_event`** (lines 3673-3697): uses `send_result()` + `result.ok` branching (fixed by `ai_loop_regressions_20260614` FR1).
|
||||
- **`src/app_controller.py:_api_generate_sync`** (line 3692): also updated by FR1 (the 2nd `except ProviderError` site was already replaced; the `try`/`except` was also restructured).
|
||||
- **`src/thinking_parser.py:parse_thinking_trace()`** (lines 8-54): supports `<thinking>`, `<thought>`, and `Thinking:` prefix markers.
|
||||
|
||||
### 3.2 Gaps to Fill (This Track's Scope)
|
||||
|
||||
#### G1: `_api_generate` NameError regression (CRITICAL)
|
||||
|
||||
**File:line**: `src/app_controller.py:265-295` (the `_api_generate` function)
|
||||
**Bug introduced by**: `ai_loop_regressions_20260614` commit `2b7b571a` (FR2 fix)
|
||||
**Symptom**: `/api/v1/generate` returns HTTP 500 with `NameError: name 'context_to_send' is not defined`
|
||||
**Root cause**: The FR2 fix removed the `try:` block (which contained the `with controller._disc_entries_lock:` acquisition and the `context_to_send = stable_md if not has_ai_response else ""` assignment) and replaced it with a `send_result()` call that still references `context_to_send`. The variable definition was lost.
|
||||
|
||||
The current state at `src/app_controller.py:278`:
|
||||
```python
|
||||
result = ai_client.send_result(context_to_send, user_msg, base_dir, ...) # context_to_send is undefined
|
||||
```
|
||||
|
||||
The fix needs to add back the 2 lines BEFORE line 278:
|
||||
```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 ""
|
||||
```
|
||||
|
||||
**Failing test**: `tests/test_headless_service.py::TestHeadlessAPI::test_generate_endpoint` (currently returns 500).
|
||||
|
||||
#### G2-G11: 10 pre-existing test mock bugs from `data_oriented_error_handling_20260606`
|
||||
|
||||
All have the same root cause: the tests were written before the refactor when `_send_<vendor>()` returned `str`; the production code now returns `Result[str]`. The fix is mechanical: change `assert result == "x"` to `assert result.ok and result.data == "x"`, and `assert "text" in result` to `assert result.ok and "text" in result.data`.
|
||||
|
||||
| # | File:line | Test | Current assertion | Fix |
|
||||
|---|---|---|---|---|
|
||||
| **G2** | `tests/test_llama_provider.py:22` | `test_send_grok_uses_xai_endpoint` (wait, this is in test_grok_provider) | `assert result == "hi from grok"` | `assert result.ok and result.data == "hi from grok"` |
|
||||
| **G3** | `tests/test_grok_provider.py:13` | `test_send_grok_uses_xai_endpoint` | `assert result == "hi from grok"` | `assert result.ok and result.data == "hi from grok"` |
|
||||
| **G4** | `tests/test_grok_provider.py:30` | `test_grok_web_search_adds_search_parameters_to_extra_body` | `assert len(captured_kwargs) == 1` (got 12) | Loop now calls the mock 12 times; update to `assert any(kw["extra_body"] is not None and kw["extra_body"].get("search_parameters", {}).get("mode") == "auto" for kw in captured_kwargs)` |
|
||||
| **G5** | `tests/test_grok_provider.py:46` | `test_grok_x_search_adds_x_source_to_extra_body` | `assert captured_kwargs[0]["extra_body"]["search_parameters"]["sources"] == [{"type": "x"}]` | Same as G4 — change to check across all kwargs |
|
||||
| **G6** | `tests/test_llama_provider.py:24` | `test_send_llama_openrouter_backend` | `assert result == "hi from openrouter"` | `assert result.ok and result.data == "hi from openrouter"` |
|
||||
| **G7** | `tests/test_llama_provider.py:43` | `test_send_llama_custom_url` | `assert result == "hi from custom"` | `assert result.ok and result.data == "hi from custom"` |
|
||||
| **G8** | `tests/test_llama_provider.py:62` | `test_send_llama_ollama_backend` | `assert "hi from ollama" in result` | `assert result.ok and "hi from ollama" in result.data` |
|
||||
| **G9** | `tests/test_llama_ollama_native.py:70` | `test_send_llama_native_calls_ollama_chat_when_localhost` | `assert "hi from native ollama" in result` | `assert result.ok and "hi from native ollama" in result.data` |
|
||||
| **G10** | `tests/test_llama_ollama_native.py:88` | `test_send_llama_native_preserves_thinking_field` | `assert "I thought about it" in result` | `assert result.ok and "I thought about it" in result.data` |
|
||||
| **G11** | `tests/test_llama_ollama_native.py:107` | `test_send_llama_routes_to_native_when_localhost` | `assert "via native" in result` | `assert result.ok and "via native" in result.data` |
|
||||
| **G12** | `tests/test_llama_ollama_native.py:122` | `test_send_llama_keeps_openai_path_for_non_local` | `assert "via openrouter" in result` | `assert result.ok and "via openrouter" in result.data` |
|
||||
| **G13** | `tests/test_ai_client_tool_loop_builder.py:22` | `test_run_with_tool_loop_calls_request_builder_each_round` | Mock returns raw `NormalizedResponse`; `_default_send` now does `if not res.ok:` expecting `Result[NormalizedResponse]` | Wrap the mock return in `Result(data=...)` |
|
||||
| **G14** | `tests/test_headless_service.py:57` | `test_generate_endpoint` | Mocks `ai_client.send` (deprecated); production now uses `send_result`. Plus the G1 NameError. | Update mock to `ai_client.send_result` returning `Result(data="AI Response")`; this test will pass after G1 is fixed |
|
||||
|
||||
#### G15: Gemini / Gemini CLI thinking-format compatibility (Bug #4 deferred from `ai_loop_regressions_20260614`)
|
||||
|
||||
**File:line**: `src/ai_client.py:_send_gemini` (lines 1538-1781) and `src/ai_client.py:_send_gemini_cli` (lines 1783-1897), possibly `src/thinking_parser.py:9`
|
||||
**Symptom**: User reported thinking monologues don't render for Gemini. The current `parse_thinking_trace` regex matches `<thinking>`, `<thought>`, and `Thinking:` prefix. The Gemini SDK may emit a different format.
|
||||
**Investigation needed**: empirically run a Gemini request that produces reasoning and inspect the raw `resp.text`. If the format is incompatible, add a normalization pass.
|
||||
|
||||
#### G16: `<think>` (half-width) marker support (Bug #5 deferred from `ai_loop_regressions_20260614`)
|
||||
|
||||
**File:line**: `src/thinking_parser.py:9` (the regex at line 9)
|
||||
**Symptom**: User screenshot 1 showed `<think>This is DWARF debug info, not the actual disassembly...</think>` — the half-width form. The current regex doesn't match this.
|
||||
**Fix**: extend the `tag_pattern` to also match `<think>...</think>` (the closing tag is the same).
|
||||
|
||||
#### G17: `state.toml` duplicate-key bug (housekeeping, blocks `ai_loop_regressions_20260614` archival)
|
||||
|
||||
**File:line**: `conductor/tracks/ai_loop_regressions_20260614/state.toml` lines 23-26 and 46-58
|
||||
**Symptom**: Python's `tomllib.load()` raises `TOMLDecodeError: Cannot overwrite a value (at line 23, column 123)`
|
||||
**Fix**: Delete the duplicate `phase_2..5` and `t2_1..t5_4` entries (the "pending" duplicates of the "completed" entries that already have the correct commit SHAs).
|
||||
|
||||
#### G18: `tracks.md` row 24 not updated (housekeeping)
|
||||
|
||||
**File:line**: `conductor/tracks.md:41`
|
||||
**Symptom**: Track 24 still shows "spec ✓, plan ✓, ready to start" though the track shipped on 2026-06-15.
|
||||
**Fix**: Update the status column to reflect completion, OR move the row to a "Recently Completed" section (per existing convention used by `qwen_llama_grok_integration_20260606`).
|
||||
|
||||
## 4. Functional Requirements
|
||||
|
||||
### FR1: Fix `_api_generate` NameError (G1)
|
||||
|
||||
`_api_generate` in `src/app_controller.py:265-295` must:
|
||||
1. Have `context_to_send` properly defined before the `send_result()` call.
|
||||
2. Continue to use the `_disc_entries_lock` for thread-safe access to `disc_entries`.
|
||||
3. Continue to use the `if not result.ok: raise HTTPException(502, ...)` pattern from the FR2 fix.
|
||||
|
||||
The fix is 2-3 lines added before line 278:
|
||||
```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 ""
|
||||
```
|
||||
|
||||
### FR2: Fix the 11 pre-existing test mock bugs (G2-G12, G14)
|
||||
|
||||
For each of the 11 tests, change the assertion pattern to handle `Result[str]`:
|
||||
- `assert result == "x"` → `assert result.ok and result.data == "x"`
|
||||
- `assert "text" in result` → `assert result.ok and "text" in result.data`
|
||||
|
||||
For the Grok web_search / x_search tests (G4, G5), the test now goes through the tool loop and the mock is called multiple times. Change `assert captured_kwargs[0]...` to `assert any(kw["extra_body"]... for kw in captured_kwargs)`.
|
||||
|
||||
For `test_headless_service.test_generate_endpoint` (G14): change the mock from `ai_client.send` to `ai_client.send_result` returning `Result(data="AI Response")`.
|
||||
|
||||
### FR3: Fix `test_ai_client_tool_loop_builder` mock shape (G13)
|
||||
|
||||
The mock at `tests/test_ai_client_tool_loop_builder.py:33` uses `patch("src.openai_compatible.send_openai_compatible", side_effect=[tool_response, final])` and returns raw `NormalizedResponse` objects. Since `run_with_tool_loop._default_send` now does `if not res.ok:` expecting a `Result[NormalizedResponse]`, the mock must return `Result(data=tool_response)` and `Result(data=final)`.
|
||||
|
||||
### FR4: Investigate and fix Gemini thinking format (G15)
|
||||
|
||||
Phase 3 task. Empirically investigate:
|
||||
1. Run a Gemini request (real or mocked) that produces thinking content.
|
||||
2. Inspect the raw `resp.text` to see what format it uses.
|
||||
3. If the format is not `<thinking>...</thinking>` or `Thinking:`, decide:
|
||||
- **Option A**: Add a normalization pass in `_send_gemini` and `_send_gemini_cli` to wrap the thinking in `<thinking>` tags before returning.
|
||||
- **Option B**: Extend `parse_thinking_trace` to match the new format.
|
||||
|
||||
The empirical finding determines the approach. Document the result in the commit message.
|
||||
|
||||
### FR5: Add `<think>` half-width marker support (G16)
|
||||
|
||||
Extend the `tag_pattern` regex at `src/thinking_parser.py:9` to also match `<think>...</think>` (half-width). The fix is a single regex addition to the existing pattern. Update the 5+ existing tests in `tests/test_thinking_trace.py` to verify the new pattern works.
|
||||
|
||||
### FR6: Fix `state.toml` duplicate keys (G17)
|
||||
|
||||
Delete lines 23-26 and 46-58 from `conductor/tracks/ai_loop_regressions_20260614/state.toml`. The "completed" entries at lines 18-22 and 29-45 are correct; the "pending" duplicates must be removed.
|
||||
|
||||
### FR7: Update `tracks.md` row 24 (G18)
|
||||
|
||||
Update the status column at `conductor/tracks.md:41` to reflect the track's completion. The user preferred pattern (move to "Recently Completed" or just update status) is a Tier 1 review decision; either is acceptable.
|
||||
|
||||
### FR8: Regression sweep + doc update
|
||||
|
||||
Phase 5 task. Run the full test suite (`uv run pytest tests/`) and verify all G1-G13 + FR1-FR5 fixes are green. Update `docs/guide_ai_client.md` "See Also" section with cross-references to this track (similar to what was done in `ai_loop_regressions_20260614`).
|
||||
|
||||
## 5. Non-Functional Requirements
|
||||
|
||||
- **NFR1 (Atomic per-task commits)**: each plan task is one commit; no batching. Use the project's "1 commit per task" discipline (see `conductor/workflow.md`).
|
||||
- **NFR2 (1-space indentation)**: enforced by the project's AI-Optimized Python style.
|
||||
- **NFR3 (No diagnostic noise in production)**: no `sys.stderr.write("[XYZ_DIAG] ...")` lines in committed code. If instrumentation is needed for the TDD test, it goes to `tests/artifacts/<test_name>.diag.log`.
|
||||
- **NFR4 (Test isolation)**: the 11 test mock fixes must NOT use `unittest.mock.patch` to bypass the new Result API; they must correctly unwrap `result.data` or check `result.ok`. Per the project's "No Mock Patches to Pseudo API" anti-pattern rule.
|
||||
- **NFR5 (No regression in other providers)**: the 5 unaffected providers (Anthropic, Qwen, Grok non-thinking tests, Llama non-mock tests, Llama native non-mock tests) must continue to pass their existing tests.
|
||||
- **NFR6 (Thread safety)**: the FR1 fix in `_api_generate` must use `_disc_entries_lock` (the same lock the original code used) to avoid races with the GUI's discussion updates.
|
||||
|
||||
## 6. Architecture Reference
|
||||
|
||||
For implementation details, consult:
|
||||
|
||||
- **`docs/guide_ai_client.md`**: the canonical guide for `src/ai_client.py`; the new `send_result()` API is documented in the "Data-Oriented Error Handling (Fleury Pattern) > Public API" section. The test mock fixes (FR2, FR3) follow the patterns shown there.
|
||||
- **`docs/guide_app_controller.md`**: the canonical guide for `src/app_controller.py`; the `_api_generate` and `_handle_request_event` flows are described in §"AI Loop Lifecycle". The FR1 fix lives in this subsystem.
|
||||
- **`docs/guide_thinking.md`** (or `docs/guide_discussions.md`): the canonical guide for thinking-mono rendering; the `parse_thinking_trace` markers are documented. FR4 (Gemini format) and FR5 (half-width marker) are in this subsystem.
|
||||
- **`conductor/code_styleguides/error_handling.md`**: the canonical reference for the Result/ErrorInfo pattern; the new FR2 test assertions follow §3.1 "AND over OR (Result struct with side-channel errors)".
|
||||
- **`docs/reports/TRACK_COMPLETION_ai_loop_regressions_20260615.md`**: the parent track's completion report. The G17 state.toml bug and the G18 tracks.md row issue are documented in the Tier 1 review §"Critical Issues" of that track.
|
||||
|
||||
## 7. Out of Scope
|
||||
|
||||
The following items are **explicitly out of scope** and tracked elsewhere:
|
||||
|
||||
- **`public_api_migration_20260606`** (planned, separate track): removes the deprecated `ai_client.send()` and migrates 5 production + 63 test call sites to `send_result()`. This track only fixes the broken `_api_generate` site (G1) and the test mock bugs that the public_api migration would touch (G2-G12). The other 50+ test call sites are deferred to public_api.
|
||||
- **`live_gui_mock_injection_20260615`** (not yet specced): infrastructure for mock injection into the live_gui subprocess. Recommended as a separate track because it requires infrastructure work (subprocess mock protocol, conftest changes) and unblocks future live_gui + AI client tests.
|
||||
- **`test_rag_phase4_final_verify` flakiness**: pre-existing RAG subsystem issue (not caused by the data_oriented_error_handling or ai_loop_regressions tracks). The `'NoneType' object has no attribute 'get'` error is in RAG config lookup code, not AI client code. Recommended as a separate RAG track.
|
||||
- **`test_discussion_truncate_layout.py::test_keep_pairs_input_uses_adequate_width`**: Phase 2 of the UI Polish Five Issues track (`ui_polish_five_issues_20260302`). The track spec is at `docs/superpowers/specs/2026-06-03-ui-polish-design.md`.
|
||||
- **`test_log_management_refresh.py::test_refresh_registry_button_calls_load_registry`**: Phase 3 of the same UI Polish track. Both are out of scope here.
|
||||
- **The deprecated `ai_client.send()` removal**: that's the public_api_migration_20260606 track.
|
||||
|
||||
## 8. Phases (Summary)
|
||||
|
||||
| Phase | Name | Tasks | Verification |
|
||||
|---|---|---|---|
|
||||
| **Phase 1** | **CRITICAL: Fix `_api_generate` NameError (G1)** | 2 tasks: write failing test (`test_generate_endpoint` already exists; verify it fails for the NameError reason), fix the production code | `test_headless_service.test_generate_endpoint` returns 200 |
|
||||
| **Phase 2** | **Fix 10 test mock bugs (G2-G12, G14) + 1 mock shape fix (G13)** | 11 tasks: one per test file (4-5 per file group), TDD-red + green per file | Full suite has 11 fewer failures |
|
||||
| **Phase 3** | **Fix Gemini / Gemini CLI thinking-format (G15)** | 3 tasks: empirical investigation, fix the format mismatch (either normalization pass or parser extension), live_gui verification | Gemini thinking mono renders in Discussion Hub |
|
||||
| **Phase 4** | **Add `<think>` half-width marker (G16)** | 2 tasks: extend regex in `thinking_parser.py:9`, add 1+ new tests in `test_thinking_trace.py` | `parse_thinking_trace` extracts 1 segment from `<think>...</think>` text |
|
||||
| **Phase 5** | **Housekeeping + regression sweep + docs (G17, G18, FR8)** | 4 tasks: fix `state.toml` duplicates, update `tracks.md`, full suite sweep, doc update | Full suite green; state.toml parseable; tracks.md row 24 updated |
|
||||
|
||||
## 9. Risk Analysis
|
||||
|
||||
| Risk | Likelihood | Impact | Mitigation |
|
||||
|---|---|---|---|
|
||||
| **R1**: The FR1 `_api_generate` fix accidentally introduces a regression in the existing FR2/FR3 logic | Low | High | The fix only ADDS lines, doesn't modify any existing logic. After the fix, the function matches the original (pre-`ai_loop_regressions_20260614`) semantics. |
|
||||
| **R2**: The 11 test mock fixes have subtle differences in `result.ok` semantics that cause new test failures | Low | Low | The pattern is mechanical (`assert result.ok` then `assert result.data == "x"`). If a test is `assert result.ok` and `result.ok` is False, the failure message is clear (shows the ErrorInfo). |
|
||||
| **R3**: The Gemini thinking format investigation (Phase 3) requires running a real Gemini request, which the user may not have credentials for | Medium | Medium | If real Gemini credentials are unavailable, use a mock client that returns a realistic Gemini response with thinking content. Document the format assumption. |
|
||||
| **R4**: The `<think>` regex extension accidentally matches too much (e.g., greedy matching across multiple segments) | Low | Low | Use `re.DOTALL` + non-greedy `.*?` (consistent with the existing pattern). The existing 5+ tests in `test_thinking_trace.py` will catch regressions. |
|
||||
| **R5**: The `state.toml` cleanup (Phase 5) accidentally deletes the wrong lines | Very Low | High | Only delete the duplicate "pending" entries; the "completed" entries with commit SHAs must be preserved. The fix is mechanical and verifiable by re-running `tomllib.load()`. |
|
||||
|
||||
## 10. Coordination with Pending Tracks
|
||||
|
||||
This track is **independent** (no `blocked_by`) but interacts with:
|
||||
|
||||
- **`ai_loop_regressions_20260614`** (shipped 2026-06-15): this track fixes the production regression (G1) and housekeeping issues (G17, G18) that the parent track left behind. It also picks up the 2 deferred bugs (G15, G16) from the parent's spec §13. No direct dependency — the parent track is shipped; this track is cleanup.
|
||||
- **`public_api_migration_20260606`** (planned, not yet specced): this track's G2-G12 test mock fixes overlap with the public_api track's test migration scope. After this track ships, the public_api track will have 11 fewer tests to migrate. The public_api track is responsible for the remaining 50+ test call sites and the 5 production call sites.
|
||||
- **`data_oriented_error_handling_20260606`** (shipped 2026-06-12): the root cause of the G2-G14 test mock bugs. This track is the test-cleanup follow-up to the parent refactor. No direct interaction — the parent track is shipped; this track fixes the remaining test fallout.
|
||||
- **UI Polish Five Issues track** (`ui_polish_five_issues_20260302`): the 2 out-of-scope test failures (`test_discussion_truncate_layout`, `test_log_management_refresh`) are Phase 2 and Phase 3 of that track. That track has its own plan and is ready to start; this track does not touch it.
|
||||
|
||||
## 11. Verification Criteria (definition of "done")
|
||||
|
||||
The track is complete when ALL of the following are true:
|
||||
|
||||
- [ ] `test_headless_service::TestHeadlessAPI::test_generate_endpoint` returns 200 (proves the G1 fix).
|
||||
- [ ] All 11 test mock fixes (G2-G12) pass: full batched test suite has 11 fewer failures than before.
|
||||
- [ ] `test_ai_client_tool_loop_builder::test_run_with_tool_loop_calls_request_builder_each_round` passes (G13).
|
||||
- [ ] Phase 3 Gemini investigation produces a finding: either a normalization pass in `_send_gemini*` is added OR the parser is extended, AND a live_gui test or unit test demonstrates Gemini thinking-mono rendering.
|
||||
- [ ] `parse_thinking_trace` correctly extracts 1 ThinkingSegment from `<think>...</think>` text (G16).
|
||||
- [ ] `tests/test_thinking_trace.py` has 1+ new test for the half-width marker; all existing 5+ tests still pass.
|
||||
- [ ] Python's `tomllib.load()` on `conductor/tracks/ai_loop_regressions_20260614/state.toml` succeeds (G17).
|
||||
- [ ] `conductor/tracks.md` row 24 reflects the track's completion (G18).
|
||||
- [ ] Full test suite is green (no new failures beyond the deferred test_rag_phase4_final_verify and UI Polish tests).
|
||||
- [ ] `docs/guide_ai_client.md` "See Also" section has 2 new cross-references: (1) this cleanup track; (2) reference to `public_api_migration_20260606`.
|
||||
- [ ] `metadata.json` `verification_criteria` field is updated to reflect completion.
|
||||
|
||||
## 12. See Also — Follow-up Notes
|
||||
|
||||
### 12.1 `public_api_migration_20260606` (planned, separate track)
|
||||
|
||||
Migrates the remaining 5 production call sites and 63 test call sites to `send_result()`. This track fixes only the broken `_api_generate` site (G1) and the 11 test mock bugs that the public_api track would have touched (G2-G12). The remaining ~50 test call sites and 5 production call sites are deferred.
|
||||
|
||||
### 12.2 `live_gui_mock_injection_20260615` (not yet specced)
|
||||
|
||||
Infrastructure for mock injection into the live_gui subprocess. The `ai_loop_regressions_20260614` Tier 2 review (§9 of the report) recommended this as a follow-up because the live_gui smoke tests only verify the Hook API substrate is reachable — they don't exercise the full request → AI client → discussion pipeline end-to-end. Without this infrastructure, future tracks hitting live_gui + AI client will hit the same wall.
|
||||
|
||||
### 12.3 `test_rag_phase4_final_verify` flakiness (separate RAG concern)
|
||||
|
||||
Pre-existing RAG subsystem issue not caused by the data_oriented_error_handling or ai_loop_regressions tracks. The error `'NoneType' object has no attribute 'get'` is in RAG config lookup code, not AI client code. A partial fix was attempted in commit `16412ad5` (RAG Phase 4 dim-mismatch recovery). Recommended as a separate RAG track.
|
||||
|
||||
### 12.4 UI Polish Five Issues track (separate track)
|
||||
|
||||
The 2 unrelated test failures in the full suite (`test_discussion_truncate_layout` and `test_log_management_refresh`) are Phase 2 and Phase 3 of the UI Polish track (`ui_polish_five_issues_20260302`). That track has its own spec and plan. Not in scope here.
|
||||
Reference in New Issue
Block a user