6.6 KiB
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_atsays 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:
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):
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ů.