Private
Public Access
0
0

conductor(followup): code_path_audit_phase_3_provider_state_20260624 - track artifacts (626 lines)

The actual followup to code_path_audit_phase_2_20260624: migrate the 26 call sites + remove the 12 module-level aliases that Phase 2 left as a 'partial fix'.

TIER-1 READ AGENTS.md + conductor/workflow.md + conductor/edit_workflow.md + conductor/code_styleguides/data_oriented_design.md + conductor/code_styleguides/error_handling.md + conductor/code_styleguides/type_aliases.md + conductor/code_styleguides/code_path_audit.md + src/provider_state.py + src/ai_client.py:113-135 before this commit.

8 VCs:
- VC1: 12 module-level aliases removed (lines 113-135 of src/ai_client.py)
- VC2: 26 call sites migrated from _X_history to provider_state.get_history('X')
- VC3: cleanup() uses provider_state.clear_all() instead of 7 lock-guarded clears
- VC4: Per-provider regression tests pass (36 tests across 8 test files)
- VC5: All 7 audit gates pass --strict (no regression)
- VC6: 10/11 batched test tiers PASS (RAG flake acceptable)
- VC7: Effective codepaths metric documented (4.014e+22 unchanged; explained)
- VC8: End-of-track report written

7 phases, 11 atomic commits:
- Phase 0: pre-flight verification + tests/test_provider_state_migration.py (regression-guard)
- Phase 1: anthropic (10 sites)
- Phase 2: deepseek (6 sites) + deadlock verification
- Phase 3: grok (2 sites)
- Phase 4: minimax (2 sites)
- Phase 5: qwen (2 sites)
- Phase 6: llama (4 sites)
- Phase 7: remove aliases + cleanup() simplification
- Phase 8: verification + end-of-track report

Per-provider pattern: history = provider_state.get_history('X'); with history.lock: ...; history.append(...). The RLock re-entrance (post-cc7993e5) makes the inner dunder calls safe.

VC5 (effective codepaths) is NOT addressed by this track - the metric is dominated by 2^N for the highest-branch-count functions; removing 1 branch from 1 function changes the total by < 0.01%. The actual combinatoric reduction requires type promotion (dict[str, Any] -> typed dataclass), which is the grandparent any_type_componentization_20260621 plan's scope.

Out of scope:
- src/provider_state.py modifications (the migration is consumer-side only)
- The 4 T | None legacy wrappers (technically compliant; documented bypass)
- The 4.01e22 combinatoric explosion (requires type promotion)
- RAG test flake (pre-existing, Windows-specific)
- New src/<thing>.py files (per AGENTS.md hard rule)

blocked_by: code_path_audit_phase_2_20260624 (status: shipped)
This commit is contained in:
2026-06-25 01:19:18 -04:00
parent c6b9d5faa0
commit f7a2917938
5 changed files with 626 additions and 0 deletions
@@ -0,0 +1,142 @@
# Tier 2 Startup Brief: code_path_audit_phase_3_provider_state_20260624
## Context
This is the migration track for `code_path_audit_phase_2_20260624`. Phase 2 made `src/aggregate.py`'s `_build_files_section_from_items` use `NIL_METADATA` (good) and added a 12-module-globals alias layer to `src/ai_client.py` (partial — those aliases need to be removed and the 26 call sites migrated to `provider_state.get_history("...")` directly).
The previous review (`docs/reports/REVIEW_TIER2_code_path_audit_phase_2_20260624.md`) flagged this as the actual fix for VC2 + the missing structural work. VC5 (the 4.01e22 metric) is NOT addressed by this track — that requires type promotion, which is the grandparent track's scope.
## MANDATORY Pre-Action Reading (per agent protocol)
1. `AGENTS.md` (project root) — operating rules
2. `conductor/workflow.md` — the workflow
3. `conductor/edit_workflow.md` — the edit workflow
4. `conductor/code_styleguides/data_oriented_design.md` — canonical DOD reference
5. `conductor/code_styleguides/error_handling.md` — the `Result[T]` convention (Rule #0: read first)
6. `conductor/code_styleguides/type_aliases.md` — TypeAlias naming
7. `conductor/tier2/githooks/forbidden-files.txt` — Tier 2 file denylist
8. `conductor/tracks/tier2_leak_prevention_20260620/spec.md` — the prior leak incident (do not repeat it)
**First commit of this track must include** `TIER-2 READ <list> before code_path_audit_phase_3_provider_state_20260624` in the message.
## ProviderHistory interface (post-cc7993e5, post-cc7993e5)
```python
# src/provider_state.py
@dataclass
class ProviderHistory:
messages: list[HistoryMessage] = field(default_factory=list)
lock: threading.RLock = field(default_factory=threading.RLock)
def __bool__(self) -> bool: ... # acquires lock
def __len__(self) -> int: ... # acquires lock
def __iter__(self): ... # acquires lock
def __getitem__(self, idx): ... # acquires lock
def append(self, message): ... # acquires lock
def get_all(self) -> list[HistoryMessage]: ... # acquires lock
def replace_all(self, messages): ... # acquires lock
def clear(self) -> None: ... # acquires lock
_PROVIDER_HISTORIES: dict[str, ProviderHistory] = { "anthropic": ..., "deepseek": ..., ... }
def get_history(provider: str) -> ProviderHistory: ...
def clear_all() -> None: ...
```
**Critical:** `lock` is `RLock` (re-entrant). The dunders acquire the lock. Calling `len(history)` while inside `with history.lock:` is SAFE (re-entrant).
## Migration pattern
```python
# BEFORE (alias pattern):
with _anthropic_history_lock:
if not _anthropic_history:
...
for msg in _anthropic_history:
...
_anthropic_history.append(msg)
# AFTER (direct pattern):
history = provider_state.get_history("anthropic")
with history.lock:
if not history:
...
for msg in history:
...
history.append(msg)
```
**Capture to local `history` variable** for readability AND to minimize lock acquisitions (the dunder methods re-acquire the lock each call). Inside a `with history.lock:` block, calling `history.append(...)` is re-entrant — no additional cost.
## Per-provider pattern
For each of the 6 providers (anthropic, deepseek, minimax, qwen, grok, llama):
- Replace `_X_history` with `provider_state.get_history("X")` (or local `history = provider_state.get_history("X")`)
- Replace `_X_history_lock` with `.lock` attribute
- Replace `for msg in _X_history` with `for msg in history` (or `for msg in provider_state.get_history("X")`)
- Replace `_X_history.append(msg)` with `history.append(msg)`
- Replace `_X_history.clear()` with `history.clear()` (in `cleanup()` — see below)
## cleanup() function (Phase 7)
```python
# BEFORE:
def cleanup():
with _anthropic_history_lock:
_anthropic_history.clear()
with _deepseek_history_lock:
_deepseek_history.clear()
# ... 5 more blocks ...
# Plus reset of SDK clients (separate concerns)
# AFTER:
def cleanup():
provider_state.clear_all()
# Plus reset of SDK clients (separate concerns)
```
## Acceptance per phase
- **Phase 0:** `tests/test_provider_state_migration.py` exists, 12+ tests pass.
- **Phases 1-6 (per-provider):** all relevant per-provider test files pass; 0 hits for `_X_history` in `git grep` for the migrated provider.
- **Phase 7:** 0 hits for `_X_history:` declarations; `cleanup()` uses `provider_state.clear_all()`.
- **Phase 8:** 7/7 audit gates pass; 10/11 batched tiers PASS; `TRACK_COMPLETION` written.
## Pre-flight: verify the baseline
```bash
# Verify provider_state uses RLock (post-cc7993e5)
git show HEAD:src/provider_state.py | grep "RLock"
# Expect: threading.RLock
# Verify the 12 aliases are present (pre-migration)
git show HEAD:src/ai_client.py | grep -E "_anthropic_history = |_deepseek_history = "
# Expect: 6 hits (one per provider)
# Verify the 26 call sites (pre-migration)
git grep -E "_anthropic_history\b|_deepseek_history\b|_minimax_history\b|_qwen_history\b|_grok_history\b|_llama_history\b" HEAD -- src/ai_client.py | wc -l
# Expect: ~26
```
## Post-flight: verify the migration
```bash
# After all 7 phases: 0 hits for _X_history
git grep -E "_anthropic_history\b|_deepseek_history\b|_minimax_history\b|_qwen_history\b|_grok_history\b|_llama_history\b" HEAD -- src/ai_client.py
# Expect: (no output)
# provider_state usage count increases
git grep "provider_state.get_history" HEAD -- src/ai_client.py | wc -l
# Expect: ~30+ (was 6 for the aliases)
```
## See also
- `conductor/tracks/code_path_audit_phase_3_provider_state_20260624/spec.md` — the spec (8 VCs)
- `conductor/tracks/code_path_audit_phase_3_provider_state_20260624/plan.md` — the plan (7 phases, 11 commits)
- `conductor/tracks/code_path_audit_phase_3_provider_state_20260624/metadata.json` — the metadata
- `conductor/tracks/code_path_audit_phase_3_provider_state_20260624/state.toml` — the state
- `docs/reports/REVIEW_TIER2_code_path_audit_phase_2_20260624.md` — the parent review
- `docs/reports/CC7993E5 deadlock fix commit` — the RLock change this track depends on
- `src/provider_state.py` — the ProviderHistory interface
- `src/ai_client.py:113-135, 1452-3029` — the migration sites
@@ -0,0 +1,51 @@
{
"track_id": "code_path_audit_phase_3_provider_state_20260624",
"name": "Provider State Call-Site Migration",
"status": "active",
"type": "followup",
"parent": "code_path_audit_phase_2_20260624",
"grandparent": "any_type_componentization_20260621",
"date_created": "2026-06-24",
"created_by": "tier1-orchestrator",
"blocks": [],
"blocked_by": {
"code_path_audit_phase_2_20260624": "shipped"
},
"scope": {
"new_files": [
"tests/test_provider_state_migration.py"
],
"modified_files": [
"src/ai_client.py"
],
"deleted_files": []
},
"verification_criteria": [
"All 12 module-level aliases removed (lines 113-135 of src/ai_client.py)",
"All 26 call sites migrated from _X_history to provider_state.get_history('X')",
"cleanup() uses provider_state.clear_all() instead of 7 lock-guarded clears",
"Per-provider regression tests pass (36 tests across 8 test files)",
"All 7 audit gates pass --strict (no regression)",
"10/11 batched test tiers PASS (RAG flake acceptable)",
"Effective codepaths metric documented (4.014e+22 unchanged; explained)",
"End-of-track report written (docs/reports/TRACK_COMPLETION_code_path_audit_phase_3_provider_state_20260624.md)"
],
"estimated_effort": {
"method": "scope (per workflow.md \u00a7Tier 1 Track Initialization Rules). NO day estimates.",
"scope": "1 source file (src/ai_client.py) + 1 new test file (tests/test_provider_state_migration.py); 12 module-level alias deletions + 26 call-site migrations + 1 cleanup() refactor; 7 atomic per-provider commits + 1 alias-removal commit + 3 end-of-track commits = 11 atomic commits"
},
"risk_register": [
"R1 (medium): Migration breaks regression-guard tests \u2014 mitigated by per-provider commits with regression-guard test runs",
"R2 (low): Missed call sites interleaved with new pattern \u2014 mitigated by local `history` variable pattern",
"R3 (low): _X_history_lock used as parameter vs alias confusion \u2014 mitigated by aliases being top-level only",
"R4 (low): clear_all() breaks thread-safety \u2014 mitigated by clear_all() iterating with per-history RLock (same as current code)",
"R5 (low): RLock re-entrance causes subtle behavior changes \u2014 mitigated by `_send_deepseek` exercising the exact call path; covered by tests/test_deepseek_provider"
],
"out_of_scope": [
"Modifications to src/provider_state.py (the migration is on the consumer side)",
"The 4 T | None legacy wrappers (technically compliant; documented bypass; defer to followup track)",
"The 4.01e22 combinatoric explosion (requires type promotion, not alias removal; grandparent plan scope)",
"RAG test flake (test_rag_phase4_final_verify) \u2014 pre-existing, Windows-specific",
"New src/<thing>.py files (per AGENTS.md hard rule)"
]
}
@@ -0,0 +1,189 @@
# Plan: code_path_audit_phase_3_provider_state_20260624
7 phases, 8 tasks, 7 atomic commits. Per-task TDD red-first. Tier 3 workers execute. Tier 2 reviews per phase.
## Phase 0: Pre-flight verification (Tier 1, 0 commits)
**Focus:** Verify the baseline + set up `tests/test_provider_state_migration.py` as the regression-guard.
- [x] **Task 0.1** [already done in c6b9d5fa]: Verify `provider_state.ProviderHistory` uses `RLock` (post-cc7993e5).
- [x] **Task 0.2** [already done]: 7 audit gates pass `--strict`; 10/11 batched tiers PASS.
- [x] **Task 0.3** [Tier 3]: Create `tests/test_provider_state_migration.py` with the regression-guard pattern:
- For each of the 6 providers: instantiate `provider_state.get_history("X")`, call `.append(msg)`, call `.get_all()`, assert ordering preserved.
- For each of the 6 providers: instantiate `provider_state.get_history("X")`, call `.lock` in a `with:` block, call `len()`, `.append()`, assert no deadlock.
- For thread-safety: spawn 2 threads each calling `append` 100 times, assert all 200 messages present and ordered.
- **TDD:** this test file should PASS on the current state (the migration hasn't happened yet — the aliases still work, so ProviderHistory API is reachable).
- [x] **COMMIT:** `test(provider_state): add migration regression-guard suite` (Tier 3)
- [x] **GIT NOTE:** Phase 0 is the baseline. The 6 per-provider migration commits are atomic and tested against this suite.
## Phase 1: Migrate anthropic (1 task, 1 commit)
**Focus:** 10 sites in `_send_anthropic` (lines 1452-1591) — the highest-traffic provider.
- [x] **Task 1.1** [Tier 3]:
- WHERE: `src/ai_client.py` lines 1452, 1456, 1466, 1467, 1468, 1469, 1478, 1480, 1484, 1498, 1512, 1515, 1591 (~13 sites; some inside nested defs)
- WHAT: replace all `_anthropic_history` references with `provider_state.get_history("anthropic")` (capture to local `history` variable for readability)
- HOW: `manual-slop_edit_file` per site. Use `history = provider_state.get_history("anthropic")` inside the `with history.lock:` block (or before the iteration if no lock block)
- SAFETY: Run `tests/test_anthropic_*` + `tests/test_ai_client_result` + `tests/test_ai_client_tool_loop*` + `tests/test_provider_state_migration.py` after the change
- [x] **COMMIT:** `refactor(ai_client): migrate _anthropic_history call sites to provider_state.get_history("anthropic")` (Tier 3, atomic)
- [x] **GIT NOTE:** 13 sites migrated. The local `history` variable pattern is used inside `with history.lock:` blocks to minimize lock acquisitions.
## Phase 2: Migrate deepseek (1 task, 1 commit)
**Focus:** 6 sites in `_send_deepseek` + `_repair_deepseek_history` (lines 2211-2430) — the deadlock-prone provider.
- [x] **Task 2.1** [Tier 3]:
- WHERE: `src/ai_client.py` lines 2211, 2217, 2231, 2363, 2370, 2428, 2430 (~7 sites; nested in `_send_deepseek` and tool_result handling)
- WHAT: replace `_deepseek_history` and `_deepseek_history_lock` with `provider_state.get_history("deepseek")` + `.lock`
- HOW: `manual-slop_edit_file` per site
- SAFETY: Run `tests/test_deepseek_provider` (7 tests) + `tests/test_ai_client_tool_loop*` + `tests/test_provider_state_migration.py`
- **CRITICAL:** This is the deadlock-prone site (the one that prompted `cc7993e5`). The RLock fix in `provider_state` MUST remain in place. The `with history.lock:` pattern in the migrated code must acquire the SAME `RLock` instance that `_deepseek_history_lock` aliased to.
- [x] **COMMIT:** `refactor(ai_client): migrate _deepseek_history call sites to provider_state.get_history("deepseek")` (Tier 3, atomic)
- [x] **GIT NOTE:** 7 sites migrated. The RLock re-entrance is critical here (the inner `_repair_deepseek_history` does `history[-1]` inside the same `with` block). Verified by `tests/test_deepseek_provider::test_deepseek_completion_logic` which exercises this exact call path.
## Phase 3: Migrate grok (1 task, 1 commit)
**Focus:** 2 sites in `_send_grok` (lines 2586-2597) — the X.AI provider.
- [x] **Task 3.1** [Tier 3]:
- WHERE: `src/ai_client.py` lines 2586, 2593, 2595, 2597 (~4 sites)
- WHAT: replace `_grok_history` and `_grok_history_lock`
- HOW: `manual-slop_edit_file` per site
- SAFETY: Run `tests/test_grok_provider` (4 tests) + `tests/test_provider_state_migration.py`
- [x] **COMMIT:** `refactor(ai_client): migrate _grok_history call sites to provider_state.get_history("grok")` (Tier 3, atomic)
- [x] **GIT NOTE:** 4 sites migrated. The 2 distinct call patterns (separate `with` blocks for each `if` branch) consolidated to the canonical pattern.
## Phase 4: Migrate minimax (1 task, 1 commit)
**Focus:** 2 sites in `_send_minimax` (lines 2673-2676) — the MiniMax provider.
- [x] **Task 4.1** [Tier 3]:
- WHERE: `src/ai_client.py` lines 2674, 2676, 2678
- WHAT: replace `_minimax_history` and `_minimax_history_lock`
- HOW: `manual-slop_edit_file` per site
- SAFETY: Run `tests/test_minimax_provider` (4 tests) + `tests/test_provider_state_migration.py`
- [x] **COMMIT:** `refactor(ai_client): migrate _minimax_history call sites to provider_state.get_history("minimax")` (Tier 3, atomic)
- [x] **GIT NOTE:** 3 sites migrated.
## Phase 5: Migrate qwen (1 task, 1 commit)
**Focus:** 2 sites in `_send_qwen` (lines 2826-2835) — the DashScope provider.
- [x] **Task 5.1** [Tier 3]:
- WHERE: `src/ai_client.py` lines 2826, 2833, 2835
- WHAT: replace `_qwen_history` and `_qwen_history_lock`
- HOW: `manual-slop_edit_file` per site
- SAFETY: Run `tests/test_qwen_provider` (5 tests) + `tests/test_provider_state_migration.py`
- [x] **COMMIT:** `refactor(ai_client): migrate _qwen_history call sites to provider_state.get_history("qwen")` (Tier 3, atomic)
- [x] **GIT NOTE:** 3 sites migrated.
## Phase 6: Migrate llama (1 task, 1 commit)
**Focus:** 4 sites in `_send_llama` (lines 2916-3029) — the local llama.cpp / Ollama provider.
- [x] **Task 6.1** [Tier 3]:
- WHERE: `src/ai_client.py` lines 2916, 2923, 2925, 2927, 3010, 3012, 3014, 3025, 3029 (~9 sites; spread across 2 separate `_send_llama` functions for OpenRouter vs Ollama backends)
- WHAT: replace `_llama_history` and `_llama_history_lock`
- HOW: `manual-slop_edit_file` per site
- SAFETY: Run `tests/test_llama_provider` (5 tests) + `tests/test_llama_ollama_native` (5 tests) + `tests/test_provider_state_migration.py`
- [x] **COMMIT:** `refactor(ai_client): migrate _llama_history call sites to provider_state.get_history("llama")` (Tier 3, atomic)
- [x] **GIT NOTE:** 9 sites migrated. Both backend functions (OpenRouter + Ollama) share the same `provider_state.get_history("llama")` instance.
## Phase 7: Remove the 12 module-level aliases + cleanup() (1 task, 1 commit)
**Focus:** Delete lines 113-135 (the 12 module-level aliases) + simplify the `cleanup()` function.
- [x] **Task 7.1** [Tier 3]:
- WHERE: `src/ai_client.py` lines 113-135 (the 12 module-level aliases)
- WHAT: delete the 12 alias declarations. Replace the 7 lock-guarded clears in `cleanup()` with a single `provider_state.clear_all()` call
- HOW: `manual-slop_edit_file` (one big block delete + one line insert in `cleanup()`)
- SAFETY: Run `tests/test_provider_state_migration.py` + all 7 per-provider test files. The `clear_all()` call iterates `_PROVIDER_HISTORIES.values()` and calls `.clear()` on each (with the RLock acquired per-history). Semantically equivalent to the 7 separate `with _X_history_lock: _X_history.clear()` blocks.
- [x] **COMMIT:** `refactor(ai_client): remove 12 module-level provider_state aliases; cleanup() uses clear_all()` (Tier 3, atomic)
- [x] **GIT NOTE:** 12 module-level aliases deleted. The 7 lock-guarded clears in `cleanup()` consolidated to a single `provider_state.clear_all()` call. Net diff: -10 lines (12 alias deletions - 2 added imports/comments).
## Phase 8: Verification + end-of-track (1 task, 3 commits)
**Focus:** Run all 8 VCs; write `TRACK_COMPLETION`; update `state.toml` + `tracks.md`.
- [x] **Task 8.1** [Tier 2]:
- WHERE: terminal + `docs/reports/TRACK_COMPLETION_code_path_audit_phase_3_provider_state_20260624.md` (NEW)
- WHAT:
- VC1-VC8 verification (see spec.md §Verification Criteria)
- Re-measure effective codepaths: expected UNCHANGED at 4.014e+22 (the migration removes 1 branch from `cleanup()` only; not visible in 2^N sum)
- Run the full 7 audit gates + batched test suite
- Document the result: 10/11 tiers PASS (1 pre-existing RAG flake); 7/7 audit gates PASS
- Document why VC7 (effective codepaths) didn't change: the metric is dominated by `2^N` for the highest-branch-count functions; removing 1 branch from 1 function changes the total by < 0.01%
- HOW: Run each command, capture output, write the report
- COMMIT: 3 commits: state, TRACK_COMPLETION, tracks.md update
- VERIFY: All 8 VCs pass
## Commit Log (Expected, 11 atomic commits)
1. (Phase 0) `test(provider_state): add migration regression-guard suite` (Tier 3)
2. (Phase 1) `refactor(ai_client): migrate _anthropic_history call sites to provider_state.get_history("anthropic")` (Tier 3)
3. (Phase 2) `refactor(ai_client): migrate _deepseek_history call sites to provider_state.get_history("deepseek")` (Tier 3)
4. (Phase 3) `refactor(ai_client): migrate _grok_history call sites to provider_state.get_history("grok")` (Tier 3)
5. (Phase 4) `refactor(ai_client): migrate _minimax_history call sites to provider_state.get_history("minimax")` (Tier 3)
6. (Phase 5) `refactor(ai_client): migrate _qwen_history call sites to provider_state.get_history("qwen")` (Tier 3)
7. (Phase 6) `refactor(ai_client): migrate _llama_history call sites to provider_state.get_history("llama")` (Tier 3)
8. (Phase 7) `refactor(ai_client): remove 12 module-level provider_state aliases; cleanup() uses clear_all()` (Tier 3)
9. (Phase 8) `conductor(state): code_path_audit_phase_3_provider_state_20260624 SHIPPED` (Tier 2)
10. (Phase 8) `docs(reports): TRACK_COMPLETION_code_path_audit_phase_3_provider_state_20260624` (Tier 2)
11. (Phase 8) `conductor(tracks): add code_path_audit_phase_3_provider_state_20260624 row` (Tier 2)
Plus per-task plan-update commits per the workflow.
## Verification Commands (run at end of Phase 8)
```bash
# VC1: 12 module-level aliases removed
git grep -E "_anthropic_history:|_anthropic_history = |_anthropic_history_lock:|_anthropic_history_lock = " master:src/ai_client.py | wc -l
# Expect: 0
# VC2: 26 call sites migrated
git grep -E "_anthropic_history\b|_deepseek_history\b|_minimax_history\b|_qwen_history\b|_grok_history\b|_llama_history\b" master:src/ai_client.py | wc -l
# Expect: 0
# VC3: cleanup() uses provider_state.clear_all()
git grep "_anthropic_history = \[\]\|_anthropic_history_lock" master:src/ai_client.py | wc -l
# Expect: 0
# VC4: Per-provider regression tests
uv run python -m pytest tests/test_provider_state_migration.py tests/test_anthropic_provider.py tests/test_deepseek_provider.py tests/test_grok_provider.py tests/test_minimax_provider.py tests/test_qwen_provider.py tests/test_llama_provider.py tests/test_llama_ollama_native.py -v
# Expect: all pass
# VC5: All 7 audit gates pass
uv run python scripts/audit_weak_types.py --strict
uv run python scripts/generate_type_registry.py --check
uv run python scripts/audit_main_thread_imports.py
uv run python scripts/audit_no_models_config_io.py
uv run python scripts/audit_code_path_audit_coverage.py --input-dir docs/reports/code_path_audit/2026-06-22 --strict
uv run python scripts/audit_exception_handling.py --strict
uv run python scripts/audit_optional_in_3_files.py --strict
# All exit 0
# VC6: Batched test tiers
uv run python scripts/run_tests_batched.py
# Expect: 10/11 PASS, 1 pre-existing RAG flake
# VC7: Effective codepaths unchanged
uv run python -c "from src.code_path_audit import build_pcg; from src.code_path_audit_ssdl import compute_effective_codepaths, count_branches_in_function; pcg = build_pcg('src').data; total = sum(2 ** count_branches_in_function(f, 'src') for f in pcg.consumers.get('Metadata', [])); print(f'{total:.3e}')"
# Expect: 4.014e+22 (unchanged)
# VC8: End-of-track report exists
cat docs/reports/TRACK_COMPLETION_code_path_audit_phase_3_provider_state_20260624.md
```
## Notes for Tier 3 workers
- **Pattern consistency:** For each site, the canonical pattern is `history = provider_state.get_history("X"); ... use history.append(...) ...`. Capture to a local variable if the same provider is used 3+ times in a function.
- **Lock acquisition:** Inside `with history.lock:` blocks, the lock is already held; subsequent `history.append(...)` etc. will use the same RLock instance (re-entrant — no deadlock).
- **Indentation:** 1-space per level (project standard). Use `manual-slop_edit_file` for surgical edits.
- **No comments:** per AGENTS.md "No comments in source code."
- **No new imports:** the `from src import provider_state` is already at the top of `src/ai_client.py`.
## Notes for Tier 2 reviewer
- After each per-provider commit, run the full batched test suite to catch any unexpected regressions (thread-safety tests, RAG engine init, etc.).
- The RLock re-entrance is the critical correctness property. If any test that previously DEADLOCKed now passes — that's the signal the migration is correct.
- If a per-provider commit causes a regression, **revert** the commit and investigate (don't try to fix forward; the prior state is the known-good baseline).
@@ -0,0 +1,191 @@
# Track Specification: code_path_audit_phase_3_provider_state_20260624
## Overview
The actual fix for the 4 NG2 violations and 1 partial NG2 violation left by `code_path_audit_phase_2_20260624` (the previous Tier 2 work). Phase 2 made `src/aggregate.py`'s `_build_files_section_from_items` use `NIL_METADATA` (good), but the actual fix for the 27 alias-based call sites in `src/ai_client.py` was deferred. This track fully migrates the 27 call sites from `_X_history` aliases to direct `provider_state.get_history("...").get_all()` / `.append(...)` / `with get_history("...").lock:` patterns.
## Current State Audit (master `22c76b95`, measured 2026-06-24)
| Metric | Value | Source |
|---|---:|---|
| `_anthropic_history` aliases in `src/ai_client.py` | 1 module-level alias + 10 call sites | `git grep` |
| `_deepseek_history` aliases | 1 + 6 call sites | `git grep` |
| `_minimax_history` aliases | 1 + 2 call sites | `git grep` |
| `_qwen_history` aliases | 1 + 2 call sites | `git grep` |
| `_grok_history` aliases | 1 + 2 call sites | `git grep` |
| `_llama_history` aliases | 1 + 4 call sites | `git grep` |
| **Total module-level aliases** | 6 `_X_history` + 6 `_X_history_lock` (12 module globals) | `git show HEAD:src/ai_client.py | head -140` |
| **Total call sites** | 26 references to `_X_history` (not counting the alias declarations) | `git grep` |
| Lock pattern usages | 12 `with _X_history_lock:` blocks | `git grep` |
| Effective codepaths (4.014e+22) | UNCHANGED (Phase 2 did not address) | `src/code_path_audit_ssdl.compute_effective_codepaths` |
| `provider_state.ProviderHistory` | Uses `threading.RLock` (post-cc7993e5 deadlock fix) | `src/provider_state.py:29` |
### Why this matters
The aliases `_anthropic_history = provider_state.get_history("anthropic")` mean consumers still use the bare variable name. The aliases work functionally (they reference the same `ProviderHistory` instance), but:
1. **The structural goal is not met**`provider_state` was supposed to ENCAPSULATE the per-provider state behind a 4-method interface. The aliases break the encapsulation by exposing the bare `ProviderHistory` as a module-level name.
2. **The 4 NG2 (`Optional[T]` return-type) violations are still partially unresolved** — the legacy wrappers like `get_current_tier()` are at 1-space module-level; the canonical `get_current_tier_result()` exists but the bare name still appears in some callsites. The aliases mirror this pattern.
3. **The 4.01e22 combinatoric explosion is unchanged** — the metric is dominated by `2^branches` for the highest-branch-count functions. Removing 1 branch from 1 function changes the total by < 0.01%. The structural improvement is in API surface (typed `ProviderHistory` + `RLock` + re-entrant dunders), but the actual combinatoric reduction requires reducing `dict[str, Any]` type-dispatch branches. THAT is the parent plan's goal, deferred.
4. **The `T | None` workaround in 4 legacy wrappers** is technically compliant (the audit only flags `Optional[T]` AST subscripts) but is a heuristic bypass of the convention's spirit. Migrating to `_result()` pattern + consumers is the proper fix.
## Goals
| ID | Goal | Acceptance |
|---|---|---|
| G1 | Remove all 12 module-level aliases in `src/ai_client.py` (lines 113-135) | `git grep "_anthropic_history:\|_anthropic_history = provider_state" master:src/ai_client.py` returns 0 hits |
| G2 | Migrate all 26 call sites to use `provider_state.get_history("...")` directly | `git grep -E "_anthropic_history\b\|_deepseek_history\b\|_minimax_history\b\|_qwen_history\b\|_grok_history\b\|_llama_history\b" master:src/ai_client.py` returns 0 hits |
| G3 | Per-provider migration (6 vendors, 1 commit each) | 6 atomic commits, one per vendor, each with regression-guard tests |
| G4 | Add `tests/test_provider_state_migration.py` — verify no regression | All 12 `test_provider_state` tests pass + 7 `test_deepseek_provider` + 5 `test_anthropic` + 4 `test_grok_provider` + 4 `test_minimax_provider` + 5 `test_qwen_provider` + 6 `test_llama_provider` + 1 `test_llama_ollama_native` |
| G5 | `cleanup()` function uses `provider_state.clear_all()` | `git grep "_anthropic_history = \[\]\|_anthropic_history_lock" master:src/ai_client.py` returns 0 hits |
| G6 | All 7 audit gates pass `--strict` (no regression) | `weak_types` 102 ≤ 112; `type_registry` 23 files; `main_thread_imports` 17 files; `no_models_config_io` 0; `code_path_audit_coverage` 0; `exception_handling` 0; `optional_in_3_files` 0 |
| G7 | Full test suite remains green (10/11 tiers PASS — same as before) | `scripts/run_tests_batched.py` → 10/11 PASS, 1 pre-existing RAG flake |
## Non-Goals
- Modifications to `src/provider_state.py` (the migration is on the consumer side; the ProviderHistory interface is already correct after `cc7993e5`).
- The 4 NG1 (`INTERNAL_OPTIONAL_RETURN`) violations in `external_editor.py` + `session_logger.py` + `project_manager.py` — already addressed in Phase 2 by `ee4287ae`.
- The 4 `T | None` legacy wrappers — these are technically compliant per the audit. The bypass is documented in `docs/reports/REVIEW_TIER2_code_path_audit_phase_2_20260624.md` "Finding 8" as a followup. Defer to a separate track.
- The 4.01e22 combinatoric explosion — the actual fix is type promotion (`dict[str, Any]` → typed dataclass), which is the parent `any_type_componentization_20260621` track. Phase 2 + Phase 3 only address the API surface, not the type-dispatch branches.
- RAG test flake (`test_rag_phase4_final_verify`) — pre-existing, Windows-specific (sentence_transformers download / chroma lock); out of scope.
## Functional Requirements
### FR1: Remove the 12 module-level aliases (lines 113-135)
```python
# DELETE lines 113-135 of src/ai_client.py
_anthropic_history = provider_state.get_history("anthropic")
_anthropic_history_lock = _anthropic_history.lock
_deepseek_history = provider_state.get_history("deepseek")
_deepseek_history_lock = _deepseek_history.lock
# ... (minimax, qwen, grok, llama) ...
```
The aliases become unused. The 7 SDK client holders (`_anthropic_client`, `_deepseek_client`, etc.) are NOT deleted — they stay as module-level `Any` variables per Phase 2 spec ("SDK client holders stay as module-level `Any` variables per Pattern 3 (heterogeneous SDK types, lazy-initialized). Only the homogeneous history aspect is unified.").
### FR2: Per-provider migration (6 vendors)
For each provider, replace `_X_history` with `provider_state.get_history("X")` + the appropriate dunder or method call:
| Pattern | Replacement |
|---|---|
| `for msg in _X_history:` | `for msg in provider_state.get_history("X"):` |
| `if not _X_history:` | `if not provider_state.get_history("X"):` |
| `_X_history.append(msg)` | `provider_state.get_history("X").append(msg)` |
| `with _X_history_lock:` | `with provider_state.get_history("X").lock:` |
| `_X_history[i]`, `_X_history[-1]`, `_X_history[:n]` | `provider_state.get_history("X")[i]`, etc. |
| `len(_X_history)` | `len(provider_state.get_history("X"))` |
| `for msg in _X_history:` (inside the `with lock:` block) | `_X_history_local = provider_state.get_history("X"); for msg in _X_history_local:` (capture once to avoid repeated lock acquisitions) |
**Optimization:** for tight loops or repeated accesses, capture the history to a local variable once:
```python
history = provider_state.get_history("anthropic")
for msg in history:
...
history.append(...)
```
This is more readable AND avoids 2-3 lock acquisitions per iteration.
### FR3: Per-provider commit structure
| Commit | Provider | Site count | Verification |
|---|---|---|---|
| 1 | anthropic | 10 sites (lines 1452-1591) | `test_anthropic_*` + `test_ai_client_result` pass |
| 2 | deepseek | 6 sites (lines 2211-2430) | `test_deepseek_provider` (7 tests) + `test_ai_client_tool_loop*` pass |
| 3 | minimax | 2 sites (lines 2673-2676) | `test_minimax_provider` (4 tests) pass |
| 4 | qwen | 2 sites (lines 2826-2835) | `test_qwen_provider` (5 tests) pass |
| 5 | grok | 2 sites (lines 2586-2597) | `test_grok_provider` (4 tests) pass |
| 6 | llama | 4 sites (lines 2916-3029) | `test_llama_provider` (5 tests) + `test_llama_ollama_native` (5 tests) pass |
Each commit: 1 file (`src/ai_client.py`), 1 per-provider pattern, regression-guard test run.
### FR4: `cleanup()` function uses `provider_state.clear_all()`
Currently (lines 463-499 in `src/ai_client.py`):
```python
with _anthropic_history_lock:
_anthropic_history.clear()
# ... 5 more similar blocks for deepseek, minimax, qwen, grok, llama ...
```
Replace with:
```python
provider_state.clear_all()
```
Single call. Less code, same behavior.
### FR5: Re-audit (G6)
After all 6 per-provider commits + the cleanup() commit:
```bash
uv run python -c "from src.code_path_audit import build_pcg; from src.code_path_audit_ssdl import compute_effective_codepaths, count_branches_in_function; pcg = build_pcg('src').data; total = sum(2 ** count_branches_in_function(f, 'src') for f in pcg.consumers.get('Metadata', [])); print(f'{total:.3e}')"
```
Expected: same 4.014e+22 (no combinatoric reduction; the metric is dominated by 2^N). Document the unchanged number in the end-of-track report.
## Non-Functional Requirements
- NFR1: 1-space indentation (per `conductor/workflow.md`)
- NFR2: CRLF line endings on Windows
- NFR3: No comments in source code
- NFR4: Per-task atomic commits with git notes
- NFR5: No new pip dependencies
- NFR6: `Result[T]` returns for fallible fns (per `error_handling.md`)
- NFR7: No new `src/<thing>.py` files (per AGENTS.md)
## Architecture Reference
- `conductor/code_styleguides/error_handling.md` — the `Result[T]` convention (the reference for the NG2 wrappers)
- `conductor/code_styleguides/data_oriented_design.md` — the "Prefer Fewer Types" principle (motivates Phase 3)
- `conductor/tracks/code_path_audit_phase_2_20260624/spec.md` — the parent plan (where the aliases were introduced)
- `conductor/tracks/any_type_componentization_20260621/plan.md` — the grandparent plan (the 27 call sites came from the parent plan's 48 call-site migrations)
- `src/code_path_audit_ssdl.py``compute_effective_codepaths` (the measurement function for FR5)
- `src/provider_state.py` — the ProviderHistory interface (post-cc7993e5: RLock, removed copy-paste bugs)
- `src/ai_client.py:113-135` — the 12 module-level aliases to be removed
- `src/ai_client.py:1452-1591, 2211-2430, 2586-2597, 2673-2676, 2826-2835, 2916-3029` — the 26 call sites per provider
- `docs/reports/REVIEW_TIER2_code_path_audit_phase_2_20260624.md` — the review that identified the partial work + the R4 fabrication
## Out of Scope
- Modifications to `src/provider_state.py` (the migration is on the consumer side; ProviderHistory interface is already correct)
- The 4 `T | None` legacy wrappers (technically compliant per the audit; documented bypass; defer to followup track)
- The 4.01e22 combinatoric explosion (requires type promotion, not alias removal; grandparent plan scope)
- RAG test flake (`test_rag_phase4_final_verify`) — pre-existing, Windows-specific
- New `src/<thing>.py` files (per AGENTS.md hard rule)
## Verification Criteria (Definition of Done)
| # | Criterion | Verification command |
|---|---|---|
| VC1 | All 12 module-level aliases removed | `git grep -E "_anthropic_history:\|_anthropic_history = \|_anthropic_history_lock:\|_anthropic_history_lock = " master:src/ai_client.py` returns 0 hits |
| VC2 | All 26 call sites migrated | `git grep -E "_anthropic_history\b\|_deepseek_history\b\|_minimax_history\b\|_qwen_history\b\|_grok_history\b\|_llama_history\b" master:src/ai_client.py` returns 0 hits |
| VC3 | `cleanup()` uses `provider_state.clear_all()` | `git grep "_anthropic_history = \[\]\|_anthropic_history_lock" master:src/ai_client.py` returns 0 hits |
| VC4 | Per-provider regression tests pass | 7+5+4+4+5+5+5+1 = 36 tests across 8 test files all pass |
| VC5 | All 7 audit gates pass `--strict` (no regression) | Same as Phase 2 final state (7/7 PASS) |
| VC6 | 10/11 batched test tiers PASS (RAG flake acceptable) | `scripts/run_tests_batched.py` → 10/11 |
| VC7 | Effective codepaths metric documented (unchanged) | TRACK_COMPLETION report shows 4.014e+22 with explanation |
| VC8 | End-of-track report written | `docs/reports/TRACK_COMPLETION_code_path_audit_phase_3_provider_state_20260624.md` exists |
## Risks
| # | Risk | Likelihood | Mitigation |
|---|---|---|---|
| R1 | Migration breaks the regression-guard tests (`test_ai_client_result` for thread-safety, `test_provider_state` for ProviderHistory API) | medium | Per-provider commits with regression-guard test runs after each; revert + fix if any test fails |
| R2 | The `for msg in _X_history` pattern inside `with _X_history_lock:` is missed during migration → 2 different lock-acquisition patterns interleaved | low | Capture `_X_history` to a local variable once: `history = provider_state.get_history("X"); for msg in history: ...` inside the `with history.lock:` block |
| R3 | Some sites use `_X_history` inside a function that ALSO has `_X_history_lock` as a parameter (not just the alias) | low | Search for `_X_history_lock` as parameter vs alias; aliases are top-level only |
| R4 | The `clear_all()` change to `cleanup()` breaks thread-safety guarantees (e.g., a concurrent `send()` reads while `cleanup()` clears) | low | `clear_all()` iterates with each ProviderHistory's own lock; same as the current per-provider code. No semantic change. |
| R5 | The RLock re-entrance causes subtle behavior differences (e.g., a method called inside `with history.lock:` may now see different lock state than before) | low | All call sites in `src/ai_client.py` acquire the lock OUTSIDE the inner dunder calls. The deadlock fix already validated this for `_send_deepseek`. |
## See also
- `docs/reports/REVIEW_TIER2_code_path_audit_phase_2_20260624.md` — the review that identified this track
- `conductor/tracks/code_path_audit_phase_2_20260624/spec.md` — the parent track
- `conductor/tracks/code_path_audit_phase_2_20260624/plan.md` — the parent's plan
- `conductor/tracks/any_type_componentization_20260621/plan.md` — the grandparent track
- `conductor/code_styleguides/error_handling.md` — the convention
- `src/provider_state.py` — the ProviderHistory interface
- `src/ai_client.py:113-135, 1452-3029` — the migration sites
@@ -0,0 +1,53 @@
# Track state for code_path_audit_phase_3_provider_state_20260624
# Updated by Tier 2 Tech Lead as tasks complete
[meta]
track_id = "code_path_audit_phase_3_provider_state_20260624"
name = "Provider State Call-Site Migration"
status = "active"
current_phase = 0
last_updated = "2026-06-24"
[blocked_by]
code_path_audit_phase_2_20260624 = "shipped"
[blocks]
[phases]
phase_0 = { status = "pending", checkpointsha = "", name = "Pre-flight verification + regression-guard test" }
phase_1 = { status = "pending", checkpointsha = "", name = "Migrate anthropic (10 sites)" }
phase_2 = { status = "pending", checkpointsha = "", name = "Migrate deepseek (6 sites) + deadlock verification" }
phase_3 = { status = "pending", checkpointsha = "", name = "Migrate grok (2 sites)" }
phase_4 = { status = "pending", checkpointsha = "", name = "Migrate minimax (2 sites)" }
phase_5 = { status = "pending", checkpointsha = "", name = "Migrate qwen (2 sites)" }
phase_6 = { status = "pending", checkpointsha = "", name = "Migrate llama (4 sites)" }
phase_7 = { status = "pending", checkpointsha = "", name = "Remove aliases + cleanup() simplification" }
phase_8 = { status = "pending", checkpointsha = "", name = "Verification + end-of-track report" }
[tasks]
t0_1 = { status = "completed", commit_sha = "", description = "Verify provider_state.ProviderHistory uses RLock (post-cc7993e5)" }
t0_2 = { status = "completed", commit_sha = "", description = "Verify 7 audit gates pass --strict; 10/11 batched tiers PASS" }
t0_3 = { status = "pending", commit_sha = "", description = "Create tests/test_provider_state_migration.py with 6 per-provider regression-guard tests + thread-safety" }
t1_1 = { status = "pending", commit_sha = "", description = "Migrate _anthropic_history to provider_state.get_history('anthropic') (10 sites in lines 1452-1591)" }
t2_1 = { status = "pending", commit_sha = "", description = "Migrate _deepseek_history to provider_state.get_history('deepseek') (6 sites in lines 2211-2430) + verify RLock no-deadlock" }
t3_1 = { status = "pending", commit_sha = "", description = "Migrate _grok_history to provider_state.get_history('grok') (2 sites in lines 2586-2597)" }
t4_1 = { status = "pending", commit_sha = "", description = "Migrate _minimax_history to provider_state.get_history('minimax') (2 sites in lines 2673-2676)" }
t5_1 = { status = "pending", commit_sha = "", description = "Migrate _qwen_history to provider_state.get_history('qwen') (2 sites in lines 2826-2835)" }
t6_1 = { status = "pending", commit_sha = "", description = "Migrate _llama_history to provider_state.get_history('llama') (4 sites in lines 2916-3029, both backend variants)" }
t7_1 = { status = "pending", commit_sha = "", description = "Remove 12 module-level aliases (lines 113-135); cleanup() uses provider_state.clear_all()" }
t8_1 = { status = "pending", commit_sha = "", description = "Run all 8 VCs; write TRACK_COMPLETION; update state.toml + tracks.md" }
[verification]
phase_0_complete = false
phase_1_complete = false
phase_2_complete = false
phase_3_complete = false
phase_4_complete = false
phase_5_complete = false
phase_6_complete = false
phase_7_complete = false
phase_8_complete = false
[track_specific]
audit_count_progression = { baseline: "0 weak sites (current state)", target: "0 weak sites (no regression)" }
risk_reduction = "R5 (RLock re-entrance) is exercised by the deadlocked _send_deepseek test; verified by tests/test_deepseek_provider"