From 26fe0b726470693159026dca80eeb15f10107766 Mon Sep 17 00:00:00 2001 From: wolverinaton Date: Sun, 30 Aug 2026 14:47:47 -0300 Subject: [PATCH] feat(guard): un except que escribe un codigo constante debe guardar la causa MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tercera aparicion del mismo defecto en este repo, y las tres costaron dias: 1. `profile/engine.py` registraba `AntigravityError` —un nombre de proveedor— cuando el fallo real era `ProfileValidationError: profile candidates must appear exactly once`. Diez dias apuntando a una caida de proveedor que no existia; se vio recien al agregar un campo `detail`. 2. `graph_observation_engine.py` tenia `except Exception:` sin capturar y escribia `error_code="synthesis_failed"` fijo. Cinco intentos dejaron cinco veces la misma palabra y cero informacion. 3. La migracion 0022 documenta lo mismo para `outcome`: "el unico rastro de por que era un sha256 de los codigos de diagnostico — la razon se destruia al escribir". Una correccion que recurre tres veces tiene que volverse algo que falle solo, no otra linea de prosa. `scripts/check_swallowed_cause.py` camina el AST: si dentro de un handler se pasa un STRING LITERAL a un parametro que nombra la causa (error_code, error, reason, outcome), el handler tiene que ligar la excepcion y usarla. Escape declarado: `# swallow-ok: `. Calibracion, que es lo que decide si un guard sobrevive: 4 hallazgos en todo el repo, los 4 genuinos — incluida `discovery_failed`, hermana de la que ya conociamos y que nadie habia visto. Un guard que inunda entrena a apaciguarlo. Arreglado de raiz: `fail_job` no tenia DONDE poner el detalle. Ahora acepta `detail` y lo escribe en `diagnostic_codes`, la columna de texto legible que la migracion 0022 agrego exactamente para esto, truncada a 500 chars. Los dos handlers de PPR-7 pasan `f"{type(exc).__name__}: {exc}"`. Los dos de los hooks usan el escape con motivo: son quiet-by-contract y `capture_usage.outcome` es vocabulario de contabilidad ('ok'/'error'), no un campo de diagnostico — guardar la causa ahi pide una columna en otro subsistema. Deuda anotada, no escondida. BARRA ROJA CONTRA HISTORIA REAL, no casos sinteticos: el guard corrido sobre `graph_observation_engine.py` en main detecta las 2 violaciones; sobre la version arreglada, 0. 11 tests del propio guard — 3 de alarma y 7 de SILENCIO, porque lo que lo hace sostenible es no ladrar ante codigo que ya cumple — mas uno que lo corre contra el repo real. 185/185 en observaciones y dreaming, ruff limpio. --- docs/generated/release-truth.json | 2 +- docs/generated/release-truth.md | 2 +- .../hooks/memorymaster-auto-ingest.py | 5 +- .../hooks/memorymaster-session-end.py | 5 +- .../knowledge/graph_observation_engine.py | 14 +- .../knowledge/graph_observation_repository.py | 20 ++- scripts/check_swallowed_cause.py | 136 ++++++++++++++++++ tests/test_swallowed_cause_check.py | 125 ++++++++++++++++ 8 files changed, 298 insertions(+), 11 deletions(-) create mode 100644 scripts/check_swallowed_cause.py create mode 100644 tests/test_swallowed_cause_check.py diff --git a/docs/generated/release-truth.json b/docs/generated/release-truth.json index 4c532f96..4c892b73 100644 --- a/docs/generated/release-truth.json +++ b/docs/generated/release-truth.json @@ -135,7 +135,7 @@ "console_entrypoints": 8, "mcp_tools": 51, "ops_cli_commands": 5, - "pytest_test_functions": 4126 + "pytest_test_functions": 4137 }, "feature_profile_matrix": { "capture_hook": [ diff --git a/docs/generated/release-truth.md b/docs/generated/release-truth.md index 15efdf85..a1a2e1d0 100644 --- a/docs/generated/release-truth.md +++ b/docs/generated/release-truth.md @@ -13,7 +13,7 @@ Do not edit this file by hand. Run `python scripts/generate_release_truth.py`. - Main CLI commands: **119** - Operations CLI commands: **5** - Console entrypoints: **8** -- Pytest source test functions: **4126** +- Pytest source test functions: **4137** ## MCP tools diff --git a/memorymaster/config_templates/hooks/memorymaster-auto-ingest.py b/memorymaster/config_templates/hooks/memorymaster-auto-ingest.py index 791c54b0..69741c11 100644 --- a/memorymaster/config_templates/hooks/memorymaster-auto-ingest.py +++ b/memorymaster/config_templates/hooks/memorymaster-auto-ingest.py @@ -292,7 +292,10 @@ def _run_incremental_llm(ledger, transcript_path, session_id, cwd, provider, ope outcome="ok", ) ledger.commit_cursor(chunk) - except Exception: + except Exception: # swallow-ok: hook quiet-by-contract; `capture_usage.outcome` + # es vocabulario de contabilidad ('ok'/'error'), no un campo de diagnostico. + # Guardar la causa pide una columna nueva en capture_usage, que es otro + # subsistema. Deuda anotada, no escondida. ledger.finish_llm(reservation, input_bytes=0, output_bytes=0, outcome="error") finally: if temp_path: diff --git a/memorymaster/config_templates/hooks/memorymaster-session-end.py b/memorymaster/config_templates/hooks/memorymaster-session-end.py index 7b0ae9dc..049431f9 100644 --- a/memorymaster/config_templates/hooks/memorymaster-session-end.py +++ b/memorymaster/config_templates/hooks/memorymaster-session-end.py @@ -48,7 +48,10 @@ def main(): run(DB_PATH, temp_path, source_agent="session-end-hook", cwd=cwd) ledger.finish_llm(reservation, input_bytes=len(chunk.text.encode("utf-8")), output_bytes=0, outcome="ok") ledger.commit_cursor(chunk) - except Exception: + except Exception: # swallow-ok: hook quiet-by-contract; `capture_usage.outcome` + # es vocabulario de contabilidad ('ok'/'error'), no un campo de diagnostico. + # Guardar la causa pide una columna nueva en capture_usage, que es otro + # subsistema. Deuda anotada, no escondida. ledger.finish_llm(reservation, input_bytes=len(chunk.text.encode("utf-8")), output_bytes=0, outcome="error") finally: if temp_path: diff --git a/memorymaster/knowledge/graph_observation_engine.py b/memorymaster/knowledge/graph_observation_engine.py index 378f249e..58c70532 100644 --- a/memorymaster/knowledge/graph_observation_engine.py +++ b/memorymaster/knowledge/graph_observation_engine.py @@ -217,8 +217,11 @@ def process_discovery(self, *, owner: str, scope: str, limit: int = 10) -> Obser job.id, owner=owner, outcome=outcome, diagnostic_codes=codes ) completed += 1 - except Exception: # noqa: BLE001 - typed retry boundary persisted below - self.repo.fail_job(job.id, owner=owner, error_code="discovery_failed") + except Exception as exc: # noqa: BLE001 - typed retry boundary persisted below + self.repo.fail_job( + job.id, owner=owner, error_code="discovery_failed", + detail=f"{type(exc).__name__}: {exc}", + ) failed += 1 return ObservationCycleResult( discovery_completed=completed, @@ -274,8 +277,11 @@ def process_synthesis(self, *, owner: str, scope: str) -> ObservationCycleResult outcome = "no_signal" self.repo.complete_job(job.id, owner=owner, outcome=outcome) completed += 1 - except Exception: # noqa: BLE001 - fail closed and retry from IDs - self.repo.fail_job(job.id, owner=owner, error_code="synthesis_failed") + except Exception as exc: # noqa: BLE001 - fail closed and retry from IDs + self.repo.fail_job( + job.id, owner=owner, error_code="synthesis_failed", + detail=f"{type(exc).__name__}: {exc}", + ) failed += 1 return ObservationCycleResult( synthesis_completed=completed, diff --git a/memorymaster/knowledge/graph_observation_repository.py b/memorymaster/knowledge/graph_observation_repository.py index 4e533054..573810b5 100644 --- a/memorymaster/knowledge/graph_observation_repository.py +++ b/memorymaster/knowledge/graph_observation_repository.py @@ -300,7 +300,20 @@ def complete_job( conn.commit() return cur.rowcount > 0 - def fail_job(self, job_id: int, *, owner: str, error_code: str) -> bool: + def fail_job( + self, job_id: int, *, owner: str, error_code: str, detail: str | None = None + ) -> bool: + """Marca el job fallido y GUARDA la causa, no solo la etiqueta. + + `error_code` clasifica ("synthesis_failed"); `detail` dice por que. Sin + el segundo, cinco intentos dejaban cinco veces la misma palabra y cero + informacion: la razon real —un proveedor dado de baja— se destruia al + escribir. Se apoya en `diagnostic_codes`, la columna de texto legible que + la migracion 0022 agrego exactamente para esto. + + Se trunca a 500 chars: un traceback entero no aporta sobre la primera + linea y esta tabla no es un log. + """ stamp_dt = _now() with self._connection() as conn: row = conn.execute( @@ -315,12 +328,13 @@ def fail_job(self, job_id: int, *, owner: str, error_code: str) -> bool: next_attempt = None if blocked else _iso(stamp_dt + timedelta(seconds=delay)) cur = conn.execute( """UPDATE graph_observation_jobs - SET status=?, error_code=?, next_attempt_at=?, updated_at=?, - lease_owner=NULL, lease_expires_at=NULL + SET status=?, error_code=?, diagnostic_codes=?, next_attempt_at=?, + updated_at=?, lease_owner=NULL, lease_expires_at=NULL WHERE id=? AND status='leased' AND lease_owner=?""", ( "blocked" if blocked else "retryable", error_code, + (detail or "")[:500] or None, next_attempt, _iso(stamp_dt), job_id, diff --git a/scripts/check_swallowed_cause.py b/scripts/check_swallowed_cause.py new file mode 100644 index 00000000..b44f9d8a --- /dev/null +++ b/scripts/check_swallowed_cause.py @@ -0,0 +1,136 @@ +"""Un `except` que escribe un codigo de error constante tiene que guardar la causa. + +POR QUE EXISTE. El mismo defecto aparecio TRES veces en este repo, y las tres +costo dias: + +1. `profile/engine.py` registraba `AntigravityError` —un nombre de proveedor— + cuando el fallo real era `ProfileValidationError: profile candidates must + appear exactly once`. Diez dias de diagnostico apuntando a una caida de + proveedor que no existia. Se descubrio recien al agregar un campo `detail`. +2. `graph_observation_engine.py` tenia `except Exception:` sin capturar y + escribia `error_code="synthesis_failed"` fijo. La razon real —un proveedor + dado de baja— quedaba destruida en cada uno de los 5 intentos. +3. La migracion 0022 documenta lo mismo para `outcome`: "el unico rastro de + *por que* era un sha256 de los codigos de diagnostico — la razon se destruia + al escribir". + +La regla: si dentro de un handler de excepcion se pasa un STRING LITERAL a un +parametro que nombra la causa (`error_code`, `error`, `reason`, `outcome`), el +handler tiene que ligar la excepcion con `as` Y usarla —loguearla, guardarla en +un campo `detail`, lo que sea—. Escribir una etiqueta constante y tirar la +excepcion convierte un diagnostico de un minuto en uno de diez dias. + +QUE **NO** MARCA, a proposito. Un handler que liga y usa la excepcion pasa, +aunque escriba una etiqueta constante: la etiqueta es para clasificar y el +detalle para diagnosticar, y los dos juntos estan bien. Un `raise` tambien pasa: +propagar preserva la causa por definicion. Una regla que dispara sobre codigo +que ya cumple entrena a apaciguarla, que es peor que no tenerla. + +Escape: `# swallow-ok: ` en la linea del `except`. + +Uso: python scripts/check_swallowed_cause.py [paths...] +Sale 1 y lista las violaciones; sale 0 y calla si no hay. +""" +from __future__ import annotations + +import ast +import sys +from dataclasses import dataclass +from pathlib import Path + +# Parametros que NOMBRAN la causa. Deliberadamente corto: `status` y `code` +# quedan afuera porque se usan para mil cosas que no son diagnostico, y marcarlos +# haria que la regla dispare sobre codigo sano. +CAUSE_KEYWORDS = frozenset({"error_code", "error", "reason", "outcome"}) + +ESCAPE = "swallow-ok" + + +@dataclass(frozen=True) +class Violation: + path: str + line: int + keyword: str + value: str + + def __str__(self) -> str: + return ( + f"{self.path}:{self.line}: escribe {self.keyword}={self.value!r} dentro de un" + f" except que descarta la excepcion" + ) + + +def _uses_name(node: ast.AST, name: str) -> bool: + return any( + isinstance(sub, ast.Name) and sub.id == name for sub in ast.walk(node) + ) + + +def _constant_cause_writes(handler: ast.ExceptHandler) -> list[tuple[int, str, str]]: + found: list[tuple[int, str, str]] = [] + for node in ast.walk(handler): + if not isinstance(node, ast.Call): + continue + for kw in node.keywords: + if kw.arg in CAUSE_KEYWORDS and isinstance(kw.value, ast.Constant): + if isinstance(kw.value.value, str): + found.append((node.lineno, kw.arg, kw.value.value)) + return found + + +def check_source(source: str, path: str) -> list[Violation]: + try: + tree = ast.parse(source) + except SyntaxError: + return [] + lines = source.splitlines() + violations: list[Violation] = [] + + for handler in (n for n in ast.walk(tree) if isinstance(n, ast.ExceptHandler)): + raw = lines[handler.lineno - 1] if handler.lineno <= len(lines) else "" + if ESCAPE in raw: + continue + # Re-lanzar preserva la causa por definicion. + if any(isinstance(n, ast.Raise) for n in ast.walk(handler)): + continue + # Liga la excepcion Y la usa -> cumple, aunque escriba una etiqueta fija. + if handler.name and any( + _uses_name(stmt, handler.name) for stmt in handler.body + ): + continue + for line, keyword, value in _constant_cause_writes(handler): + violations.append(Violation(path, line, keyword, value)) + return violations + + +def check_paths(paths: list[Path]) -> list[Violation]: + violations: list[Violation] = [] + for root in paths: + files = [root] if root.is_file() else sorted(root.rglob("*.py")) + for file in files: + if "test" in file.name or "/tests/" in file.as_posix(): + continue + violations.extend( + check_source(file.read_text(encoding="utf-8", errors="replace"), + file.as_posix()) + ) + return violations + + +def main(argv: list[str]) -> int: + roots = [Path(a) for a in argv[1:]] or [Path("memorymaster")] + violations = check_paths(roots) + if not violations: + return 0 + print(f"{len(violations)} handler(es) escriben una causa constante y tiran la real:") + for violation in violations: + print(f" {violation}") + print( + "\nLigar la excepcion (`except X as exc`) y usarla: loguearla o guardarla" + f"\nen un campo de detalle. Escape justificado: `# {ESCAPE}: `." + ) + return 1 + + +if __name__ == "__main__": + sys.exit(main(sys.argv)) diff --git a/tests/test_swallowed_cause_check.py b/tests/test_swallowed_cause_check.py new file mode 100644 index 00000000..eee60f1c --- /dev/null +++ b/tests/test_swallowed_cause_check.py @@ -0,0 +1,125 @@ +"""Tests del guard que exige guardar la causa, no solo la etiqueta. + +Un guard sin tests es una superstición: no se sabe si atrapa lo que dice ni, +peor, si dispara sobre codigo sano. Lo segundo importa mas — un check que ladra +ante codigo que ya cumple entrena a apaciguarlo, y termina desactivado. + +Por eso hay tantos casos de SILENCIO como de alarma, y el ultimo test corre el +guard contra el repo real: la unica prueba de que es sostenible es que main pase. +""" +from __future__ import annotations + +from pathlib import Path + +from scripts.check_swallowed_cause import check_paths, check_source + +REPO = Path(__file__).resolve().parents[1] + + +def _n(src: str) -> int: + return len(check_source(src, "x.py")) + + +# --- lo que DEBE marcar ----------------------------------------------------- + +def test_marca_except_sin_ligar_que_escribe_codigo_constante(): + """El caso exacto que costo diez dias en graph_observation_engine.""" + assert _n( + "try:\n f()\n" + "except Exception:\n" + " repo.fail_job(1, error_code='synthesis_failed')\n" + ) == 1 + + +def test_marca_aunque_ligue_si_nunca_usa_la_excepcion(): + """Poner `as exc` y no usarlo es la misma perdida con mejor apariencia.""" + assert _n( + "try:\n f()\n" + "except Exception as exc:\n" + " repo.fail_job(1, error_code='synthesis_failed')\n" + ) == 1 + + +def test_marca_cada_palabra_de_causa(): + for palabra in ("error_code", "error", "reason", "outcome"): + src = ( + "try:\n f()\n" + "except Exception:\n" + f" ledger.finish(1, {palabra}='error')\n" + ) + assert _n(src) == 1, f"no marco {palabra}" + + +# --- lo que debe CALLAR (lo que hace sostenible al guard) ------------------- + +def test_calla_si_liga_y_usa_la_excepcion(): + """Etiqueta + detalle es correcto: la etiqueta clasifica, el detalle explica.""" + assert _n( + "try:\n f()\n" + "except Exception as exc:\n" + " repo.fail_job(1, error_code='synthesis_failed'," + " detail=f'{type(exc).__name__}: {exc}')\n" + ) == 0 + + +def test_calla_si_solo_loguea_la_excepcion(): + assert _n( + "try:\n f()\n" + "except Exception as exc:\n" + " logger.warning('fallo: %s', exc)\n" + " repo.fail_job(1, error_code='synthesis_failed')\n" + ) == 0 + + +def test_calla_si_re_lanza(): + """Propagar preserva la causa por definicion.""" + assert _n( + "try:\n f()\n" + "except Exception:\n" + " repo.fail_job(1, error_code='x')\n" + " raise\n" + ) == 0 + + +def test_calla_ante_el_escape_declarado(): + assert _n( + "try:\n f()\n" + "except Exception: # swallow-ok: hook quiet-by-contract\n" + " ledger.finish(1, outcome='error')\n" + ) == 0 + + +def test_calla_si_el_codigo_no_es_constante(): + """Un valor derivado de la excepcion ya lleva la causa adentro.""" + assert _n( + "try:\n f()\n" + "except Exception as exc:\n" + " repo.fail_job(1, error_code=f'failed:{type(exc).__name__}')\n" + ) == 0 + + +def test_calla_ante_un_except_que_no_escribe_causa(): + """La mayoria de los handlers del repo: no tocan un campo de causa.""" + assert _n( + "try:\n f()\n" + "except Exception:\n" + " data = {}\n" + ) == 0 + + +def test_calla_ante_palabras_parecidas_pero_no_de_causa(): + """`status` y `code` se usan para mil cosas; marcarlos inundaria de falsos.""" + assert _n( + "try:\n f()\n" + "except Exception:\n" + " resp(status='error', code='500')\n" + ) == 0 + + +# --- el repo real ----------------------------------------------------------- + +def test_el_repo_pasa_el_guard(): + """Si esto se pone rojo, o hay un handler nuevo que tira la causa o el guard + se volvio demasiado ancho. Las dos merecen mirarse, ninguna silenciarse.""" + violations = check_paths([REPO / "memorymaster"]) + assert not violations, "\n".join(str(v) for v in violations)