diff --git a/conductor/tracks/fix_test_failures_20260624/metadata.json b/conductor/tracks/fix_test_failures_20260624/metadata.json new file mode 100644 index 00000000..ffba932f --- /dev/null +++ b/conductor/tracks/fix_test_failures_20260624/metadata.json @@ -0,0 +1,59 @@ +{ + "track_id": "fix_test_failures_20260624", + "name": "Fix 14 Test Failures (post-polish merge)", + "created_date": "2026-06-24", + "branch": "master", + "depends_on": ["code_path_audit_polish_20260622"], + "blocks": [], + "scope": { + "new_files": [], + "modified_files": [ + "src/openai_schemas.py", + "tests/test_auto_whitelist.py", + "tests/test_command_palette_sim.py" + ], + "deleted_files": [] + }, + "estimated_effort": { + "method": "scope (per workflow.md §Tier 1 Track Initialization Rules). NO day estimates.", + "phase_1": "1 task: add custom __init__ to NormalizedResponse (fixes 12 tests)", + "phase_2": "1 task: update test_auto_whitelist_keywords to use dataclasses.replace", + "phase_3": "1 task: update test_palette_starts_hidden to use deterministic close", + "phase_4": "1 task: 6 verification criteria + TRACK_COMPLETION + state + tracks.md" + }, + "verification_criteria": [ + "VC1: 12 NormalizedResponse tests pass", + "VC2: test_auto_whitelist_keywords passes", + "VC3: test_palette_starts_hidden passes", + "VC4: full batched test suite is green (all 11 tiers PASS)", + "VC5: 4 audit gates remain clean", + "VC6: no new test failures introduced" + ], + "known_issues": [], + "deferred_to_followup_tracks": [], + "regressions_and_pre_existing_failures": [], + "pre_existing_failures_remaining": [], + "risk_register": [ + { + "id": "risk-1", + "description": "Custom __init__ with init=False breaks other NormalizedResponse callers passing positional args", + "likelihood": "low", + "impact": "Phase 1 Task 1.1 may break production code at src/ai_client.py:908 or the 12 tests", + "mitigation": "All 12 tests + 1 production site use kwargs; verify with ast.parse + 12-test run" + }, + { + "id": "risk-2", + "description": "dataclasses.replace for Session may not work if Session is slotted or has __slots__", + "likelihood": "low", + "impact": "Phase 2 Task 2.1 fails", + "mitigation": "Session is a regular @dataclass(frozen=True); replace() works on regular dataclasses" + }, + { + "id": "risk-3", + "description": "A deterministic close callback does not exist in _predefined_callbacks", + "likelihood": "medium", + "impact": "Phase 3 Task 3.1 needs fallback (push toggle twice)", + "mitigation": "Plan includes the toggle-twice fallback in the Task 3.1 WHAT description" + } + ] +} \ No newline at end of file diff --git a/conductor/tracks/fix_test_failures_20260624/plan.md b/conductor/tracks/fix_test_failures_20260624/plan.md new file mode 100644 index 00000000..d76e5cc5 --- /dev/null +++ b/conductor/tracks/fix_test_failures_20260624/plan.md @@ -0,0 +1,104 @@ +# Plan: fix_test_failures_20260624 + +3 phases, 3 tasks, 3 atomic commits. TDD: write the failing test invocation first (the existing 14 tests are the red), fix the code (green), commit. + +## Phase 1: NormalizedResponse dual-signature (1 task) + +Focus: Add custom `__init__` to `NormalizedResponse` that accepts both new nested `usage: UsageStats` and legacy flat kwargs. This fixes 12 of the 14 failing tests in one place. + +- [ ] Task 1.1: Add custom `__init__` to `NormalizedResponse` in `src/openai_schemas.py`. + - WHERE: `src/openai_schemas.py:75-80` (the `@dataclass(frozen=True) class NormalizedResponse:` block) + - WHAT: + - Change `@dataclass(frozen=True)` to `@dataclass(frozen=True, init=False)` on line 75 + - Add a custom `__init__` method (after the class fields) that: + - Signature: `(self, text: str, tool_calls: tuple[ToolCall, ...] = (), usage: Optional[UsageStats] = None, raw_response: Any = None, usage_input_tokens: Optional[int] = None, usage_output_tokens: Optional[int] = None, usage_cache_read_tokens: Optional[int] = None, usage_cache_creation_tokens: Optional[int] = None) -> None` + - If `usage is None` and any legacy flat kwarg is non-None: build `UsageStats(input_tokens=usage_input_tokens or 0, output_tokens=usage_output_tokens or 0, cache_read_tokens=usage_cache_read_tokens or 0, cache_creation_tokens=usage_cache_creation_tokens or 0)` + - If `usage is None` and all legacy flat kwargs are None: build `UsageStats(0, 0)` + - Otherwise: use the provided `usage` + - Use `object.__setattr__(self, "text", text)` for all 4 field assignments (required because `frozen=True` locks `__setattr__`) + - HOW: Use `manual-slop_py_update_definition` to replace the entire class block. The new content includes the `init=False` decorator and the custom `__init__` method. + - SAFETY: + - Verify with `ast.parse(open("src/openai_schemas.py").read())` (no syntax errors) + - Run `uv run python -c "from src.openai_schemas import NormalizedResponse, UsageStats; r = NormalizedResponse(text='hi', tool_calls=(), usage_input_tokens=10, usage_output_tokens=5, raw_response=None); print(r.usage.input_tokens, r.usage.output_tokens)"` + - Run the 12 affected tests + - COMMIT: `fix(schemas): add legacy-kwarg backward compat to NormalizedResponse.__init__` + - GIT NOTE: 12 tests fixed by single backward-compat __init__; supports both new nested usage: UsageStats and legacy flat usage_input_tokens=... kwargs + - VERIFY: `uv run pytest tests/test_ai_client_cli.py tests/test_ai_client_tool_loop.py tests/test_ai_client_tool_loop_builder.py tests/test_ai_client_tool_loop_send_func.py tests/test_gemini_cli_integration.py tests/test_gemini_cli_parity_regression.py tests/test_gemini_cli_edge_cases.py -v` shows 12/12 pass + +## Phase 2: Session frozen fix (1 task) + +Focus: Update the test to use `dataclasses.replace()` instead of mutating the frozen `Session`. + +- [ ] Task 2.1: Update `test_auto_whitelist_keywords` to use `dataclasses.replace`. + - WHERE: `tests/test_auto_whitelist.py:20` + - WHAT: Change `reg.data[session_id]["whitelisted"] = True` to `reg.data[session_id] = dataclasses.replace(reg.data[session_id], whitelisted=True)` + - HOW: Use `manual-slop_edit_file` with the exact old/new text. Add `import dataclasses` to the top of the file if not already imported. + - SAFETY: Verify with `uv run pytest tests/test_auto_whitelist.py -v` + - COMMIT: `test(auto-whitelist): use dataclasses.replace for frozen Session mutation` + - GIT NOTE: 1 test fixed; the old `["whitelisted"] = True` was invalid on a frozen dataclass + - VERIFY: `uv run pytest tests/test_auto_whitelist.py -v` shows 0 failures + +## Phase 3: Toggle race fix (1 task) + +Focus: Replace the non-deterministic toggle with a deterministic close path. + +- [ ] Task 3.1: Update `test_palette_starts_hidden` to use a deterministic close callback. + - WHERE: `tests/test_command_palette_sim.py:38` (the `client.push_event("custom_callback", {"callback": "_toggle_command_palette", "args": []})` call) + - WHAT: + - Check `_predefined_callbacks` in `src/api_hooks.py` for a close callback (e.g., `_close_command_palette` or similar). If found, use it. + - If no close callback exists, push `_toggle_command_palette` once, poll for state, then push again to guarantee the palette is closed. + - HOW: Use `manual-slop_edit_file` with the exact old/new text. + - SAFETY: The test must end with `show_command_palette is False` regardless of starting state. Verify with `uv run pytest tests/test_command_palette_sim.py -v` in isolation AND in the full batch (per the `Isolated-Pass Verification Fallacy` rule in `conductor/workflow.md`). + - COMMIT: `test(palette): use deterministic close instead of toggle for palette_starts_hidden test` + - GIT NOTE: 1 test fixed; toggle is non-deterministic (close-if-open, open-if-closed); new code forces close regardless of starting state + - VERIFY: `uv run pytest tests/test_command_palette_sim.py -v` AND `uv run python scripts/run_tests_batched.py` (tier-3-live_gui) + +## Phase 4: Verification (1 task) + +Focus: Confirm all 10 verification criteria pass. + +- [ ] Task 4.1: Run all 6 VCs; write the track's end-of-track report. + - WHERE: Full test suite + audit gates + - WHAT: + - Run VC1-VC6 (the 6 verification criteria from the spec) + - Write `docs/reports/TRACK_COMPLETION_fix_test_failures_20260624.md` with the verification results + - Update this track's `state.toml` to `status = "completed"`, `current_phase = "complete"`, all 4 phases `completed` + - Update `conductor/tracks.md` to add a row for this track + - HOW: Run each VC command, capture output, write the report. + - SAFETY: The 2 pre-existing-violation audit gates (NG1, NG2 from the polish track) are still out of scope. Do not regress them. + - COMMIT: 3 commits: `conductor(state): fix_test_failures_20260624 SHIPPED`, `docs(reports): TRACK_COMPLETION for fix_test_failures_20260624`, `conductor(tracks): add fix_test_failures_20260624 row` + - GIT NOTE: 1 per commit per workflow.md + - VERIFY: All 6 VCs pass; the 14 previously-failing tests are now green; no new failures introduced + +## Commit Log (Expected) + +1. `fix(schemas): add legacy-kwarg backward compat to NormalizedResponse.__init__` (Task 1.1) +2. `test(auto-whitelist): use dataclasses.replace for frozen Session mutation` (Task 2.1) +3. `test(palette): use deterministic close instead of toggle for palette_starts_hidden test` (Task 3.1) +4. `conductor(state): fix_test_failures_20260624 SHIPPED` (Task 4.1) +5. `docs(reports): TRACK_COMPLETION for fix_test_failures_20260624` (Task 4.1) +6. `conductor(tracks): add fix_test_failures_20260624 row` (Task 4.1) + +## Verification Commands (run at end of Phase 4) + +```bash +# VC1: the 12 NormalizedResponse tests pass +uv run pytest tests/test_ai_client_cli.py tests/test_ai_client_tool_loop.py tests/test_ai_client_tool_loop_builder.py tests/test_ai_client_tool_loop_send_func.py tests/test_gemini_cli_integration.py tests/test_gemini_cli_parity_regression.py tests/test_gemini_cli_edge_cases.py -v + +# VC2: test_auto_whitelist_keywords passes +uv run pytest tests/test_auto_whitelist.py -v + +# VC3: test_palette_starts_hidden passes +uv run pytest tests/test_command_palette_sim.py -v + +# VC4: full batched suite is green +uv run python scripts/run_tests_batched.py + +# VC5: 4 audit gates +uv run python scripts/audit_exception_handling.py +uv run python scripts/audit_weak_types.py +uv run python scripts/audit_main_thread_imports.py +uv run python scripts/audit_no_models_config_io.py + +# VC6: confirm no new failures (diff against pre-fix summary) +``` \ No newline at end of file diff --git a/conductor/tracks/fix_test_failures_20260624/spec.md b/conductor/tracks/fix_test_failures_20260624/spec.md new file mode 100644 index 00000000..9eea35e1 --- /dev/null +++ b/conductor/tracks/fix_test_failures_20260624/spec.md @@ -0,0 +1,133 @@ +# Track Specification: fix_test_failures_20260624 + +## Overview + +3 surgical fixes to make the full test suite green after the polish merge. The 14 failing tests cluster into 3 root causes; each is a single-file change. + +## Current State Audit (commit 4c2bb3c9, master) + +3 tier failures from `uv run python scripts/run_tests_batched.py`: + +### Root cause 1: NormalizedResponse API mismatch (12 tests) + +`src/openai_schemas.py:75-80` defines `NormalizedResponse` with 4 fields: `text`, `tool_calls`, `usage: UsageStats`, `raw_response`. The `@dataclass` auto-generates `__init__` requiring `usage: UsageStats`. + +But 12 tests + 1 production site at `src/ai_client.py:908` call it with the OLD flat-kwarg API: +```python +NormalizedResponse( + text=..., tool_calls=..., + usage_input_tokens=10, usage_output_tokens=5, + usage_cache_read_tokens=0, usage_cache_creation_tokens=0, + raw_response=None, +) +``` + +**Failing tests:** +- `tests/test_ai_client_cli.py::test_ai_client_send_gemini_cli` +- `tests/test_ai_client_tool_loop.py` (5 tests) +- `tests/test_ai_client_tool_loop_builder.py::test_run_with_tool_loop_calls_request_builder_each_round` +- `tests/test_ai_client_tool_loop_send_func.py` (2 tests) +- `tests/test_gemini_cli_integration.py::test_gemini_cli_full_integration` +- `tests/test_gemini_cli_parity_regression.py::test_send_invokes_adapter_send` +- `tests/test_gemini_cli_edge_cases.py::test_gemini_cli_loop_termination` + +**Fix:** add a custom `__init__` to `NormalizedResponse` (using `@dataclass(frozen=True, init=False)`) that accepts BOTH signatures. The legacy flat kwargs are auto-converted to a `UsageStats` instance. + +### Root cause 2: Session dataclass frozen (1 test) + +`tests/test_auto_whitelist.py:20` does `reg.data[session_id]["whitelisted"] = True`. `Session` is `@dataclass(frozen=True)` so attribute assignment fails. + +**Failing test:** `tests/test_auto_whitelist.py::test_auto_whitelist_keywords` + +**Fix:** update the test to use `dataclasses.replace(reg.data[session_id], whitelisted=True)` (or equivalent setter on the registry). + +### Root cause 3: Toggle race (1 test) + +`tests/test_command_palette_sim.py::test_palette_starts_hidden` calls `_toggle_command_palette` (non-deterministic) and asserts `show_command_palette is False`. The toggle is open-if-closed, close-if-open, so the assertion depends on prior state. + +**Failing test:** `tests/test_command_palette_sim.py::test_palette_starts_hidden` + +**Fix:** call a deterministic close callback (e.g., `_close_command_palette`) instead of toggle. If no such callback exists, push the toggle twice (open then close). + +## Goals + +| ID | Goal | Acceptance | +|---|---|---| +| G1 | `NormalizedResponse` accepts both the new `usage: UsageStats` and the legacy flat `usage_input_tokens=...` kwargs | All 12 affected tests pass; existing usage in `src/ai_client.py:908` continues to work | +| G2 | `test_auto_whitelist_keywords` no longer attempts to mutate a frozen `Session` | The test passes; no production code change | +| G3 | `test_palette_starts_hidden` uses a deterministic close path | The test passes; the test no longer depends on prior fixture state | +| G4 | Full test suite (`scripts/run_tests_batched.py`) is green | 0 failed, 0 new failures vs baseline | + +## Non-Goals + +- Refactoring `NormalizedResponse` to remove the legacy support (the fix is additive; legacy kwargs continue to work) +- Migrating `src/ai_client.py:908` from the legacy API to the new API (out of scope; the fix makes both work) +- Adding new tests beyond fixing the 14 failing ones +- Touching any other files + +## Functional Requirements + +### FR1: NormalizedResponse dual-signature __init__ + +`src/openai_schemas.py` line 75-80: change `@dataclass(frozen=True)` to `@dataclass(frozen=True, init=False)` and add a custom `__init__` that: +- Accepts all 4 canonical kwargs: `text`, `tool_calls`, `usage`, `raw_response` (with defaults) +- Accepts all 4 legacy flat kwargs: `usage_input_tokens`, `usage_output_tokens`, `usage_cache_read_tokens`, `usage_cache_creation_tokens` (all `Optional[int]`, default `None`) +- If `usage` is None AND any legacy flat kwarg is non-None, builds a `UsageStats` from the legacy kwargs +- If both `usage` and legacy kwargs are None, builds `UsageStats(0, 0)` +- Uses `object.__setattr__` for all 4 field assignments (required because `frozen=True`) + +### FR2: test_auto_whitelist_keywords uses replace + +`tests/test_auto_whitelist.py:20`: change `reg.data[session_id]["whitelisted"] = True` to use `dataclasses.replace(reg.data[session_id], whitelisted=True)` and assign back to `reg.data[session_id]`. + +### FR3: test_palette_starts_hidden uses deterministic close + +`tests/test_command_palette_sim.py:38`: change the `_toggle_command_palette` callback to a deterministic close. Options (in preference order): +- (a) Call `_close_command_palette` directly if it exists in `_predefined_callbacks` +- (b) If no close callback exists, push `_toggle_command_palette` once, wait for state to settle, then push it again to guarantee close +- The test must end with `show_command_palette is False` regardless of starting state + +## Non-Functional Requirements + +- NFR1: 1-space indentation +- NFR2: CRLF line endings (Windows) +- NFR3: No comments in source code +- NFR4: Per-task atomic commits +- NFR5: No new pip dependencies +- NFR6: 0 new test failures introduced + +## Architecture Reference + +- `conductor/code_styleguides/error_handling.md` — `Result[T]` convention (no changes needed; no Result changes in this fix) +- `conductor/code_styleguides/data_oriented_design.md` — DOD reference (the NormalizedResponse fix is purely additive) +- `src/openai_schemas.py:75-93` — the file being modified (FR1) +- `tests/test_auto_whitelist.py:20` — the file being modified (FR2) +- `tests/test_command_palette_sim.py:38` — the file being modified (FR3) + +## Out of Scope + +- Migrating `src/ai_client.py:908` from the legacy API to `usage: UsageStats(...)` (the additive fix makes both work; explicit migration is a separate concern) +- Adding regression tests for the new NormalizedResponse dual-signature behavior (the 12 existing tests ARE the regression test) +- Refactoring Session to not be frozen (the test fix is simpler) +- Fixing the root cause of `_toggle_command_palette` being non-deterministic (the test fix is simpler) +- Any changes to test infrastructure or conftest + +## Verification Criteria (Definition of Done) + +| # | Criterion | Verification command | +|---|---|---| +| VC1 | The 12 NormalizedResponse tests pass | `uv run pytest tests/test_ai_client_cli.py tests/test_ai_client_tool_loop.py tests/test_ai_client_tool_loop_builder.py tests/test_ai_client_tool_loop_send_func.py tests/test_gemini_cli_integration.py tests/test_gemini_cli_parity_regression.py tests/test_gemini_cli_edge_cases.py -v` | +| VC2 | `test_auto_whitelist_keywords` passes | `uv run pytest tests/test_auto_whitelist.py -v` | +| VC3 | `test_palette_starts_hidden` passes | `uv run pytest tests/test_command_palette_sim.py -v` | +| VC4 | Full batched test suite is green | `uv run python scripts/run_tests_batched.py` (all 11 tiers PASS) | +| VC5 | The 4 mandatory audit gates remain clean | `uv run python scripts/audit_exception_handling.py` (informational); `uv run python scripts/audit_weak_types.py`; `uv run python scripts/audit_main_thread_imports.py`; `uv run python scripts/audit_no_models_config_io.py` | +| VC6 | No new test failures introduced | diff `uv run python scripts/run_tests_batched.py` summary table against pre-fix baseline (the 14 known failures are now green; no new failures added) | + +## Risks + +| # | Risk | Likelihood | Mitigation | +|---|---|---|---| +| R1 | The custom `__init__` with `init=False` breaks other NormalizedResponse callers that pass positional args | low | The class is only constructed in 1 production site (`src/ai_client.py:908`) and the 12 test sites, all of which use kwargs. Verify by running the 12 tests. | +| R2 | The `dataclasses.replace()` for Session assignment to `reg.data[session_id]` mutates the registry's internal state in a way that breaks other tests | low | The registry stores sessions in a dict. `reg.data[session_id] = replace(...)` is a normal dict assignment, equivalent to the old `reg.data[session_id]["whitelisted"] = True` semantics. | +| R3 | A deterministic close callback doesn't exist in `_predefined_callbacks` | medium | Fallback option: push toggle twice. Verify by checking the available callbacks in the API hooks registry. | +| R4 | `UsageStats(0, 0)` syntax (when no kwargs and no legacy) is wrong if UsageStats requires explicit args | low | Verified: `UsageStats` has `input_tokens: int` and `output_tokens: int` as required, with `cache_*_tokens: int = 0` defaults. `UsageStats(0, 0)` is valid. | \ No newline at end of file diff --git a/conductor/tracks/fix_test_failures_20260624/state.toml b/conductor/tracks/fix_test_failures_20260624/state.toml new file mode 100644 index 00000000..15ca7ee5 --- /dev/null +++ b/conductor/tracks/fix_test_failures_20260624/state.toml @@ -0,0 +1,39 @@ +# Track state for fix_test_failures_20260624 +# 3 surgical fixes for 14 post-polish-merge test failures. +# 4 phases, 4 tasks. Tier 2 to execute per conductor/workflow.md. + +[meta] +track_id = "fix_test_failures_20260624" +name = "Fix 14 Test Failures (post-polish merge)" +status = "active" +current_phase = 0 +last_updated = "2026-06-24" + +[parent] +# Follow-up to code_path_audit_polish_20260622 (merged) + +[blocked_by] +code_path_audit_polish_20260622 = "merged" + +[blocks] +# This track blocks nothing. It is a test-fix task. + +[phases] +phase_1 = { status = "pending", checkpointsha = "", name = "NormalizedResponse dual-signature __init__" } +phase_2 = { status = "pending", checkpointsha = "", name = "Session frozen fix in test_auto_whitelist" } +phase_3 = { status = "pending", checkpointsha = "", name = "Toggle race fix in test_palette_starts_hidden" } +phase_4 = { status = "pending", checkpointsha = "", name = "Verification + End-of-Track Report" } + +[tasks] +t1_1 = { status = "pending", commit_sha = "", description = "Add custom __init__ to NormalizedResponse accepting both nested usage: UsageStats and legacy flat usage_input_tokens=... kwargs" } +t2_1 = { status = "pending", commit_sha = "", description = "Update test_auto_whitelist_keywords to use dataclasses.replace for frozen Session mutation" } +t3_1 = { status = "pending", commit_sha = "", description = "Update test_palette_starts_hidden to use deterministic close callback instead of non-deterministic toggle" } +t4_1 = { status = "pending", commit_sha = "", description = "Run all 6 VCs; write TRACK_COMPLETION report; update this state.toml + conductor/tracks.md" } + +[verification] +vc1_normalized_response_tests_pass = false +vc2_auto_whitelist_test_passes = false +vc3_palette_starts_hidden_test_passes = false +vc4_full_batched_suite_green = false +vc5_audit_gates_clean = false +vc6_no_new_test_failures = false \ No newline at end of file