docs: document reliability fixes

Author: elkaix

.pythinker/reports/arch-review-2026-05-30.md

@@ -0,0 +1,319 @@
+# Validated Architectural Review Report
+
+**Review scope:** `src/pythinker_code/background/`, `src/pythinker_code/memory/`, and `src/pythinker_code/auth/opencode_go.py`  
+**Validation date:** 2026-05-30  
+**Validated against:** current working tree at `d3bf815627fdcfcf87f1f8abbabbb2d6749419a0`  
+**Status:** findings re-validated against the live tree (tests re-run, usages re-grepped). Each finding now carries a concrete, behavior-preserving fix.
+
+---
+
+## Summary
+
+The original draft mixed valid cleanup opportunities with stale or incorrect findings. This version keeps only findings that still match the current codebase, and pairs each with the most robust fix that preserves existing behavior.
+
+**Current actionable findings:**
+
+1. `memory/retriever.py` still has an unused abstraction seam around `Retriever` / `LexicalRetriever` / `SqliteFts5Retriever`.
+2. `background/manager.py` still duplicates locked task-runtime status mutation logic across six `_mark_task_*` methods.
+3. `background/manager.py` still reaches into private store locking/write helpers where the public `update_runtime()` API can cover most cases.
+4. `memory/recall.py` still passes a `store_path` argument that is immediately discarded.
+5. `memory/consolidation.py` still calls the private `ProjectMemoryStore._ensure_dir()` method.
+
+**Optional cleanups:**
+
+- `Counter(doc)` can replace the manual term-frequency loop in `memory/retriever.py`.
+- OpenCode Go response parsing can be clarified, but any Pydantic refactor must preserve current tolerant parsing behavior.
+
+**Corroborating evidence gathered during re-validation:**
+
+- The retriever seam (Finding 1) has **zero production references** — `SqliteFts5Retriever` and `sqlite_fts5_available` appear only in `retriever_sqlite.py` itself and one test; the only retriever used in the app path is `LexicalRetriever`.
+- The migration target in Finding 3, `BackgroundTaskStore.update_runtime()`, is **already the established pattern** in this module (`manager.py:277`, `manager.py:659`, `worker.py:276`). The six `_mark_task_*` methods and the stale-recovery block are the remaining holdouts.
+- `update_runtime_under_lock()` does **not** exist anywhere in the tree — no new lock-exposing API is needed.
+
+The stale and incorrect draft entries have been removed; the sections below list only current findings, robust fixes, and optional cleanups that still match the codebase.
+
+---
+
+## Recommended fix order
+
+The findings interact. Applying them in this order avoids rework and keeps each diff behavior-preserving:
+
+1. **Finding 4** — drop the dead `store_path` parameter. This removes the `_ensure_dir()` call at `recall.py:252`.
+2. **Finding 5** — add the public `ensure_root()` alias. After step 1, `consolidation.py:36` is the only external `_ensure_dir()` caller, so this isolates the change.
+3. **Finding 2** — extract the status-transition helper. Route it through `update_runtime()`.
+4. **Finding 3** — step 3 already eliminates the private `_runtime_lock` / `_write_runtime_unlocked` usage in the six mark-methods (`manager.py:821-912`). Only the stale-recovery block (`manager.py:598-626`) remains; decide its treatment per Finding 3 below.
+5. **Finding 1** — delete the unused retriever seam.
+6. **Optional A / B** — apply if desired; B is "leave as-is" by default.
+
+---
+
+## Validated Findings
+
+### 1. Unused retriever abstraction and SQLite seam
+
+**Files:**
+
+- `src/pythinker_code/memory/retriever.py:7,49-54`
+- `src/pythinker_code/memory/retriever_sqlite.py` (entire file)
+
+**Severity:** Low  
+**Category:** Overengineering / YAGNI
+
+`Retriever` is an abstract base class with no production polymorphic dispatch. `LexicalRetriever` is the real implementation used by recall (`recall.py:13,94`), while `SqliteFts5Retriever` is a capability seam that delegates directly to `LexicalRetriever`.
+
+Re-validation tightened this: `SqliteFts5Retriever` and `sqlite_fts5_available` have **no production callers at all** — they are referenced only inside `retriever_sqlite.py` and a single test (`tests/core/test_memory_phase_bcd.py:18,158`, `test_sqlite_retriever_falls_back_to_lexical`). There is no `memory/__init__.py` re-exporting these symbols, so nothing outside the package depends on them.
+
+**Robust fix:**
+
+Delete the dead seam outright — it is safe because there is no public export and no production dispatch:
+
+- Delete `src/pythinker_code/memory/retriever_sqlite.py`.
+- In `retriever.py`, remove the `Retriever` ABC (lines 49-51) and make `LexicalRetriever` a plain class (`class LexicalRetriever:`). Remove the now-orphaned `from abc import ABC, abstractmethod` import (line 7).
+- Keep the name `LexicalRetriever` — it is imported at `recall.py:13` and used at `recall.py:94`; renaming is pure churn for no behavioral gain.
+- Delete the test `test_sqlite_retriever_falls_back_to_lexical` and its import at `tests/core/test_memory_phase_bcd.py:18`. (The fallback it asserts no longer exists once the wrapper is gone.)
+
+Do **not** reintroduce an FTS5 abstraction until there is a measured indexed implementation and a real dispatch path. If a future plugin contract genuinely needs polymorphism, add the protocol back at that point with a concrete second implementation — not before.
+
+---
+
+### 2. Duplicated background task status mutation methods
+
+**File:** `src/pythinker_code/background/manager.py:821-922`  
+**Severity:** Medium  
+**Category:** Duplication / maintainability
+
+There are six near-identical status mutation methods, each performing the same locked read-check-mutate-write sequence:
+
+- `_mark_task_running`
+- `_mark_task_awaiting_approval`
+- `_mark_task_completed`
+- `_mark_task_failed`
+- `_mark_task_timed_out`
+- `_mark_task_killed`
+
+**Semantics that must be preserved exactly:**
+
+- terminal states must not be overwritten (early no-op);
+- `updated_at` is set to now on every applied transition;
+- `_mark_task_running()` sets `heartbeat_at = updated_at` and clears `failure_reason`;
+- `completed` clears `failure_reason`; `completed`/`failed`/`timed_out`/`killed` set `finished_at = updated_at`; `running`/`awaiting_approval` do **not** set `finished_at`;
+- `timed_out` sets both `interrupted` and `timed_out`; `killed` sets `interrupted`;
+- telemetry fires **only** when the transition was actually applied (not on the terminal no-op), guarded by `started_at and finished_at`, with reason labels unchanged: `completed` → `success=True` (no reason); `failed` → `reason="error"`; `timed_out` → `reason="timeout"`; `killed` → `reason="killed"`; `running`/`awaiting_approval` → no telemetry.
+
+**Robust fix:**
+
+Extract one private helper that owns the locked, terminal-guarded write via the **public** `update_runtime()` API, and signals whether the transition was applied so telemetry stays in the callers:
+
+```python
+def _transition_status(
+    self,
+    task_id: str,
+    *,
+    mutate: Callable[[TaskRuntime], None],
+) -> TaskRuntime | None:
+    """Locked, terminal-guarded status write via the public store API.
+
+    Stamps ``updated_at`` then applies ``mutate``. Returns the resulting
+    runtime when the transition was applied, or ``None`` when the task was
+    already terminal (no write performed, so callers skip telemetry).
+    """
+    applied = False
+
+    def _apply(runtime: TaskRuntime) -> bool:
+        nonlocal applied
+        if is_terminal_status(runtime.status):
+            return False
+        runtime.updated_at = time.time()
+        mutate(runtime)
+        applied = True
+        return True
+
+    runtime = self._store.update_runtime(task_id, _apply)
+    return runtime if applied else None
+```
+
+Each mark-method becomes a thin mutator plus its own telemetry. Two representative cases:
+
+```python
+def _mark_task_running(self, task_id: str) -> None:
+    def mutate(r: TaskRuntime) -> None:
+        r.status = "running"
+        r.heartbeat_at = r.updated_at
+        r.failure_reason = None
+
+    self._transition_status(task_id, mutate=mutate)
+
+def _mark_task_completed(self, task_id: str) -> None:
+    def mutate(r: TaskRuntime) -> None:
+        r.status = "completed"
+        r.finished_at = r.updated_at
+        r.failure_reason = None
+
+    runtime = self._transition_status(task_id, mutate=mutate)
+    if runtime and runtime.started_at and runtime.finished_at:
+        from pythinker_code.telemetry import track
+
+        track(
+            "background_task_completed",
+            success=True,
+            duration_s=runtime.finished_at - runtime.started_at,
+        )
+```
+
+`_mark_task_failed` / `_mark_task_timed_out` / `_mark_task_killed` follow the same shape, each setting its status, `finished_at`, flags, and `failure_reason` in `mutate`, then emitting telemetry with its own `reason` label guarded by `if runtime and runtime.started_at and runtime.finished_at`. `_mark_task_awaiting_approval` uses only `mutate` with no telemetry.
+
+This collapses six bodies to one shared critical section while keeping each method's distinct mutation and telemetry explicit. Ensure `Callable` is imported (`from collections.abc import Callable`) if it is not already.
+
+---
+
+### 3. Manager still uses private store lock/write internals
+
+**Files:**
+
+- `src/pythinker_code/background/manager.py:598-626` (stale-task recovery)
+- `src/pythinker_code/background/manager.py:821-912` (the six mark-methods)
+- `src/pythinker_code/background/store.py:143-153` (`update_runtime`)
+
+**Severity:** Medium  
+**Category:** Architectural coupling
+
+`BackgroundTaskManager` calls private store internals directly — `self._store._runtime_lock(...)` and `self._store._write_runtime_unlocked(...)` — in both the mark-methods and the stale-recovery block. The store already exposes `update_runtime(task_id, update_fn)`, which performs the locked read-modify-write and returns the resulting runtime, and which the manager **already uses** at `manager.py:277` and `manager.py:659` (and the worker at `worker.py:276`). No `update_runtime_under_lock()` API is needed and none exists.
+
+**Robust fix:**
+
+- **Six mark-methods (`821-912`):** resolved for free by Finding 2 — routing `_transition_status` through `update_runtime()` removes every private `_runtime_lock` / `_write_runtime_unlocked` call in these methods.
+- **Stale-recovery block (`598-626`):** this block reads *both* `read_runtime` and `read_control` under the same lock and branches on `fresh_control.kill_requested_at` (`manager.py:600,615`). `update_runtime`'s callback is only *passed* the runtime, but it is a closure over `self._store`, so it can call `self._store.read_control(view.spec.id)` itself — that read executes inside the same `_runtime_lock` and does not deadlock, because `read_runtime`/`read_control` are lock-free (`store.py` takes `_runtime_lock` only in `write_runtime`/`update_runtime`; the current block already calls `read_control` while holding the lock). So this path *can* migrate with **no new store API**. Two reasonable options:
+  1. **Leave it as-is.** The block is a single, well-commented critical section (`manager.py:595-597` explains *why* the lock spans the read and the write). Holding `_runtime_lock` here is correct and the private access is localized. Most surgical, lowest-risk — the default recommendation.
+  2. **Migrate via a closure that reads control in-callback.** Move the body into a `recover_stale(runtime) -> bool` closure passed to `update_runtime()`; the early-outs (terminal / not-yet-stale) become `return False`, replacing today's `continue`. The surrounding per-view loop is unchanged — each iteration calls `update_runtime(view.spec.id, recover_stale)`:
+
+     ```python
+     def recover_stale(runtime: TaskRuntime) -> bool:
+         if is_terminal_status(runtime.status):
+             return False
+         progress = (
+             runtime.heartbeat_at or runtime.started_at
+             or runtime.updated_at or view.spec.created_at
+         )
+         if now - progress <= stale_after:
+             return False
+         control = self._store.read_control(view.spec.id)  # inside the lock via the closure
+         heartbeat_missing = runtime.heartbeat_at is None
+         runtime.finished_at = now
+         runtime.updated_at = now
+         if control.kill_requested_at is not None:
+             runtime.status = "killed"
+             runtime.interrupted = True
+             runtime.failure_reason = control.kill_reason or "Killed during recovery"
+         else:
+             runtime.status = "lost"
+             runtime.failure_reason = (
+                 "Background worker never heartbeat after startup"
+                 if heartbeat_missing
+                 else "Background worker heartbeat expired"
+             )
+         return True
+
+     self._store.update_runtime(view.spec.id, recover_stale)
+     ```
+
+Neither option needs a new lock-exposing or recovery-specific store method. Prefer (1) for minimal churn, or (2) if you want zero private-internal access from the manager.
+
+---
+
+### 4. `build_recall_block()` accepts an unused `store_path`
+
+**File:** `src/pythinker_code/memory/recall.py:86-97,252-258`  
+**Severity:** Low  
+**Category:** Dead parameter / unnecessary private access
+
+`build_recall_block()` accepts `store_path`, then immediately discards it with `_ = store_path` (`recall.py:97`). The call site computes that value through `self._store._ensure_dir()` (`recall.py:252`) purely to feed the dead parameter. `build_recall_block` is called only at `recall.py:253` and three tests, so the signature is safe to change.
+
+**Robust fix:**
+
+- Remove the `store_path` parameter from `build_recall_block()` (`recall.py:92`) and delete the `_ = store_path` discard line (`recall.py:97`).
+- At the call site, delete `store_root = await self._store._ensure_dir()` (`recall.py:252`) and the `store_path=str(store_root / "memory")` argument (`recall.py:258`). This also removes one private `_ensure_dir()` usage.
+- Update the three call sites that pass the kwarg, dropping `store_path=...`:
+  - `tests/core/test_recall_provider.py:39` (`test_build_recall_block_includes_open_todos_and_facts`)
+  - `tests/core/test_recall_provider.py:52` (`test_build_recall_block_empty_when_nothing`)
+  - `tests/core/test_recall_provider.py:63` (`test_build_recall_block_open_todos_only_does_not_suggest_missing_files`)
+
+The recall output is unchanged — the parameter and its value were never used in the block. The only incidental difference is that recall no longer eagerly creates the store directory via `_ensure_dir()`; the recall read path (candidates are passed in already loaded) does not depend on that side effect.
+
+---
+
+### 5. Memory consolidation calls a private store method
+
+**File:** `src/pythinker_code/memory/consolidation.py:35-37`  
+**Severity:** Low  
+**Category:** Architectural coupling
+
+`inbox_dir()` calls `ProjectMemoryStore._ensure_dir()` from outside `project_memory.py` and suppresses the private-usage warning (`consolidation.py:36`). After Finding 4 removes the `recall.py:252` usage, `consolidation.py:36` is the **only** external `_ensure_dir()` caller.
+
+**Robust fix:**
+
+- Add a public async alias on `ProjectMemoryStore` that delegates to the existing implementation:
+
+  ```python
+  async def ensure_root(self) -> Path:
+      """Public entry point for ``_ensure_dir`` used by collaborators."""
+      return await self._ensure_dir()
+  ```
+
+- Update `consolidation.py:36` to `root = await store.ensure_root()` and drop the `# pyright: ignore[reportPrivateUsage]` suppression.
+
+Keep `_ensure_dir()` as the single source of truth; `ensure_root()` is only a public surface so collaborators don't reach into a private method. (Naming note: the background and notifications stores use a private `_ensure_root`; the public `ensure_root` here is intentional and reads cleanly as the published method.)
+
+---
+
+## Optional Cleanups
+
+### A. Use `collections.Counter` for term frequency
+
+**File:** `src/pythinker_code/memory/retriever.py:76-78`  
+**Severity:** Low
+
+Replace the manual term-frequency loop:
+
+```python
+tf: dict[str, int] = {}
+for term in doc:
+    tf[term] = tf.get(term, 0) + 1
+```
+
+with `tf = Counter(doc)` (`from collections import Counter`). This is a drop-in: the scoring loop guards every access with `if term not in tf: continue` before reading `tf[term]`, so `Counter`'s default-zero behavior changes nothing. Readability cleanup only — not a correctness issue. The adjacent document-frequency loop (`retriever.py:67-70`) could likewise use `df.update(set(doc))`, but leave it unless you are already touching that block.
+
+---
+
+### B. OpenCode Go response parsing can be made clearer, but not stricter by accident
+
+**File:** `src/pythinker_code/auth/opencode_go.py:179-228`  
+**Severity:** Low/Medium
+
+The current implementation manually validates `/models` and `models.dev` payloads with `isinstance()` and casts. The behavior is intentionally tolerant:
+
+- malformed top-level payloads return an empty result (`_extract_model_ids` → `[]`, `_parse_models_dev_metadata` → `{}`);
+- malformed list/dict entries are skipped (`continue`) while valid entries are kept;
+- models.dev enrichment is best-effort and must not break login.
+
+**Robust fix: prefer leaving this as-is.** The current `isinstance`/`cast` code already encodes exactly the tolerance required, and a Pydantic rewrite risks silently regressing it for no functional gain. If clarity is the goal, add a short comment documenting the tolerance contract rather than rewriting the parser.
+
+If a Pydantic refactor is nonetheless mandated, it must preserve those semantics: validate entries **item-by-item** with `model_config = ConfigDict(extra="ignore")`, skipping individual `ValidationError`s, and never reject the whole response because one entry is malformed. Top-level shape mismatch must still yield empty results, and enrichment failure must never propagate into the login path.
+
+---
+
+## Verification Performed
+
+Targeted validation command (re-run during this pass):
+
+```bash
+uv run pytest \
+  tests/background/test_manager.py::test_recover_agent_view_does_not_clobber_terminal_runtime_from_stale_view \
+  tests/background/test_manager.py::test_mark_task_completed_is_lock_protected \
+  tests/background/test_worker.py::test_worker_completes_successfully \
+  tests/core/test_memory_phase_bcd.py::test_sqlite_retriever_falls_back_to_lexical \
+  tests/core/test_recall_provider.py::test_build_recall_block_includes_open_todos_and_facts
+```
+
+Result: **5 passed** (re-confirmed 2026-05-30, `0.08s`).
+
+Note: after applying Finding 1 (delete the retriever seam) and Finding 4 (drop `store_path`), two of these tests change by design — `test_sqlite_retriever_falls_back_to_lexical` is deleted with the seam, and `test_build_recall_block_includes_open_todos_and_facts` drops its `store_path` kwarg. Re-run the full `tests/background` and `tests/core` suites after each fix to confirm no behavioral regressions.