runtime

plans/2026-09-02_reflect-review-findings.md

@@ -0,0 +1,133 @@
+# Reflect skill — review 2026-09-02
+
+Stav: nálezy review, opravy navrženy, čekají na schválení implementace
+Datum: 2026-09-02
+
+Review prošlo celý skill: SKILL.md, README.md, `reflect_apply.py`, `reflect_auto.py`,
+`reflect_distill.py` a všech 168 testů (prošly, 1.1 s). Celkový verdikt: nadprůměrně
+dobře napsaný — „záruky v kódu, ne v promptu" je proveden důsledně, testy kódují
+racionalu u každého assertu. Níže jsou problémy v pořadí závažnosti + návrh opravy
+u každého.
+
+## 1. Fold přeloženého open nálezu zahodí drafted patch a skip count (bug)
+
+`merge_findings` (reflect_auto.py ~522) dědí `regression_of` a `history`, ale ne
+`patch`, `patch_drafted_at` ani `skipped`. Jakmile noční běh znovu spatří stejný
+`open` pattern, `supersede` starý záznam zahodí a nový vzniká bez těchto polí.
+
+Následky:
+
+- patch složený při review přes `--set-patch` (agent ho pracně ověřil proti
+  souboru) zmizí; SKILL.md krok 2 tvrdí „`patch_drafted_at` says an earlier
+  review drafted it" — po jedné noci to neplatí
+- „deferred 2× already" z kroku 3 se vynuluje — nález se předkládá donebezedne
+  bez viditelné historie odkladů, i když ho uživatel už několikrát odložil
+- audit log (`DRAFTED`, `SKIPPED`) ukazuje práci, na kterou store už neodkazuje
+
+**Fix:** v `merge_findings` při foldu open/watch předchozího záznamu přenést:
+
+```python
+if previous and previous["status"] == STATUS_OPEN:
+    if previous.get("patch") and not item.get("patch"):
+        # drafted during a review, verified against the file — do not throw it away
+        patch = previous["patch"]          # do Finding(...)
+        patch_drafted_at = previous.get("patch_drafted_at")
+    skipped = previous.get("skipped")      # deferral history survives the fold
+```
+
+Pole `patch_drafted_at` a `skipped` je potřeba přidat do `Finding` dataclass a
+`to_json()` (podmíněně jako ostatní volitelná pole). Nový patch z analýzy má
+přednost před starým draftem; jinak se drží draft z review. Testy: fold open
+nálezu s `skipped={count:2}` a drafted patchem → nový záznam obojí nese;
+nový patch z modelu draft nepřepisuje, ale nahrazuje.
+
+## 2. `reflect_apply.py` nechrání vlastní store jako cíl patche (designová mezera)
+
+`_resolve_target` odmítne cestu mimo workspace, ale klidně aplikuje patch na
+`reflect/findings.jsonl`, `reflect/state.json` nebo `log/reflect.log`. Celá
+filozofie skillu je „do audit trail píše jen reflect_apply" — ale reflect_apply
+sám může patchem přepsat audit trail (typicky změnit `rejected` záznam zpět na
+`open`, což oživí zamítnutý vzor). Stane se to „se schválením uživatele", které
+v diffu snadno přehlédne, že jde o store.
+
+**Fix:** explicitní blocklist v `check_patch` / `_resolve_target`
+(reflect_apply.py):
+
+```python
+PROTECTED = ("reflect/", "log/reflect.log")
+
+def _resolve_target(workspace, relative):
+    target = (workspace / relative).resolve()
+    if not target.is_relative_to(workspace.resolve()):
+        raise ApplyError(f"{relative} resolves outside the workspace")
+    if target == (workspace / FINDINGS_REL).resolve() or \
+       any(target.is_relative_to(workspace / prefix) for prefix in PROTECTED):
+        raise ApplyError(f"{relative} is part of the reflect store — not a patch target")
+    ...
+```
+
+Důvod zdůvodnit v chybové hlášce („the audit trail is never a patch target").
+Pozn.: `set_patch` tím kryje i draft, nejen apply. Test: patch s
+`file: reflect/findings.jsonl` → exit 2, store netčen.
+
+## 3. Vakuózní assert v testu (test_reflect_auto.py)
+
+`test_first_run_has_no_trend_to_show` tvrdí `"(minule" not in report`, ale
+`_rate_line` generuje anglické „(previous run …)". Assert nikdy nemůže selhat,
+test tedy nic nehlídá.
+
+**Fix:** `assert "(previous" not in report`. Jednořádková změna.
+
+## 4. Noise prefix `"cli"` chytá i `client*` (případné falešné vyřazení)
+
+`key.startswith(NOISE_PREFIXES)` — session `client_xyz` (nebo cokoli začínající
+„cli") se tiše vyřadí z analýzy. Prefix-match na krátkých prefixech je
+přístřelen. Podobně base64 session jména obsahující `-`/`_` (jsou v urlsafe
+abecedě) se nedekódují kvůli heuristice `"_" in stem or "-" in stem` —
+legitimní stará session se pak chytne prefixem, nebo naopak neprojde.
+
+**Fix:** noise match na hranici klíče: session klíče mají tvar
+`<prefix>_<rest>` resp. base64 bez `_`, takže matchovat
+`key == prefix or key.startswith(prefix + "_")`. Případně (jednodušeji)
+přejmenovat prefix na `cli_` v NOISE_PREFIXES, protože reálné machinery session
+jsou `cli_<…>`. Test: `client_abc` prochází, `cli_kimi-ollama-test` ne.
+
+## Menší
+
+### 5. Git fingerprint se nekontroluje při LLM error cestě
+
+`_resolve_findings`: fingerprint se porovná jen po úspěšném tahu. Když tah
+skončí LLM errorem a agent v tu chvíli něco zapsal, guard se neprojeví a
+run pokračuje. Riziko je teoretické (error odpověď znamená, že k zápisu
+s nejvyšší pravděpodobností nedošlo), ale guard je zadarmadlo dokončit.
+
+**Fix:** porovnat fingerprint i na začátku error větve
+(`if result.stop_reason == "error" or result.error:`) — stejná kontrola,
+stejná hláška.
+
+### 6. `MIN_MESSAGES=5` počítá i tool výsledky
+
+`distill_session` inkrementuje `message_count` pro user, assistant i tool
+role. Session s 1 user zprávou a 2 tool cally (celkem 5 záznamů) projde
+prahem, přestože je to jeden dotaz — nižší signál, ne rovnou chyba.
+
+**Fix (volitelné):** počítat jen user + assistant zprávy
+(`message_count += 1` jen v těch dvou větvích). Tool results počítat jako
+součást tahu, ne jako zprávu. Existující testy `message_count == 6` adaptovat.
+
+### 7. Neatomičnost mezi `git commit` patche a `_save(findings)`
+
+Crash mezi commitem patche a zápisem store zanechá soubor patched, ale store
+`open` (bez audit linky). Re-apply se správně odmítne na chybějícím `old_text`,
+ale audit stopa pro ten commit chybí. U single-user workspace akceptovatelné;
+zmiňuju pro úplnost. Plná atomicita (např. save-first-then-commit s rollbackem
+store) nedoporučuju — přidá složitost pro okrajový scénář. Spíš uvážit
+pořadí: nejdřív `_save` + audit, pak commit; pak crash zanechá `applied`
+záznam bez commitu, což reflektuje hlášku `git revert` — ale zase commit
+refusu zanechá store applied bez commitu. Trade-off, nechává rozhodnutí
+na implementaci.
+
+## Pořadí implementace
+
+1 → 2 → 3 → 5 → 4 → (6, 7 volitelné). Položky 1–3 jsou přímé rozpory se
+zárukami deklarovanými v README („Záruky"), 5 a 4 jsou levné tvrdnutí guardů.