From 3cb0b6e8d2bd459b355815d1753d472eb34d6ba3 Mon Sep 17 00:00:00 2001 From: Guangyu Date: Wed, 2 Sep 2026 18:39:00 +0000 Subject: [PATCH] fix(playbook-optimizer): stop persisting raw exception text in decision_reason The GEPA optimizer's failure path wrote `str(exc)` into `playbook_optimization_jobs.decision_reason`. That column is durable, is `TEXT NOT NULL` in both the SQLite and tenant Postgres schemas, is read straight back into `PlaybookOptimizationJob`, and is shown to operators -- and an arbitrary exception message can carry customer content. A pydantic `ValidationError` raised on a provider response renders the model's own output (itself derived from evidence text) into its message; that was confirmed on a sibling analysis path. Every other writer of this column already uses a fixed phrase, so the column is a de facto controlled vocabulary and this site was the outlier. It now writes one too. Nothing diagnostic is lost: the `error_tags` block immediately above already binds `error_type=type(exc).__name__` and `logger.exception` records the traceback, which is where an unbounded signal belongs. The exception CLASS name is deliberately not added to the column either. It is not customer content, but it would widen an operator-facing fixed vocabulary into a semi-open one keyed on third-party exception types, and it is already captured in the tags. This follows the precedent set by the open-world terminal-failure diagnostic, which records class names under a reserved metadata key while leaving the persisted reason vocabulary fixed. Tests: - a behavioural test raises an exception whose message carries a distinctive sentinel and asserts the sentinel reaches neither `decision_reason` nor `metadata_json`. Restoring `str(exc)`, and an f-string variant of it, both turn it red. - an AST guard asserts every `decision_reason=` passed by a service-layer writer is a fixed string (or a conditional of fixed strings), so a new writer interpolating a value fails the build rather than leaking quietly. It carries a non-vacuity test that fails if the scan stops finding the known writers. It found a real false-positive class on its first run -- the storage layer's row-to-entity hydration -- which is now excluded and documented. The optimizer test harness gained `org_id` on its fake request context: the exception path had never been exercised, so the missing attribute had gone unnoticed. --- .../services/playbook_optimizer/optimizer.py | 11 +- .../test_decision_reason_vocabulary_guard.py | 123 ++++++++++++++++++ .../test_playbook_optimizer.py | 63 +++++++++ 3 files changed, 196 insertions(+), 1 deletion(-) create mode 100644 tests/server/services/playbook_optimizer/test_decision_reason_vocabulary_guard.py diff --git a/reflexio/server/services/playbook_optimizer/optimizer.py b/reflexio/server/services/playbook_optimizer/optimizer.py index 29748adb1..d8c4464b4 100644 --- a/reflexio/server/services/playbook_optimizer/optimizer.py +++ b/reflexio/server/services/playbook_optimizer/optimizer.py @@ -249,8 +249,17 @@ def optimize( error_type=type(exc).__name__, ): logger.exception("Playbook optimization failed") + # The exception message is deliberately not persisted here. It can + # carry customer content -- a pydantic ValidationError raised on a + # provider response renders the model's own output into its message + # -- and ``decision_reason`` is a durable column read back into the + # domain model and shown to operators. The class name, the traceback + # and the tags are already captured by the ``error_tags`` block + # above, which is where an unbounded diagnostic belongs. self.storage.update_playbook_optimization_job( - job.job_id, status="failed", decision_reason=str(exc) + job.job_id, + status="failed", + decision_reason="optimization run raised an unexpected error", ) return "failed" diff --git a/tests/server/services/playbook_optimizer/test_decision_reason_vocabulary_guard.py b/tests/server/services/playbook_optimizer/test_decision_reason_vocabulary_guard.py new file mode 100644 index 000000000..9ac137310 --- /dev/null +++ b/tests/server/services/playbook_optimizer/test_decision_reason_vocabulary_guard.py @@ -0,0 +1,123 @@ +"""Guard: ``decision_reason`` writers must pass a fixed string, never an expression. + +The invariant +------------- +``playbook_optimization_jobs.decision_reason`` is a durable ``TEXT NOT NULL`` +column that is read straight back into ``PlaybookOptimizationJob`` and shown to +operators, and a Postgres trigger compares it against literal values. It is a +controlled vocabulary: every writer names a decision the optimizer made. + +An expression in that keyword is how customer content gets in. ``str(exc)`` on +an arbitrary exception is the concrete case -- a pydantic ``ValidationError`` +raised on a provider response renders the model's own output into its message, +so persisting the message persists evidence text. The class name, the traceback +and the tags belong in the structured-logging / error-reporting path instead, +which every failure site already uses. + +Why a guard rather than review +------------------------------ +The leak is invisible at the call site: ``decision_reason=str(exc)`` reads like +helpful diagnostics, produces no test failure, and only shows its teeth on the +one exception whose message happens to quote a transcript. A behavioural test +pins the one site it exercises; this scan pins the shape of every site. + +Scan approach +------------- +Parse every module under ``reflexio/server/`` with :mod:`ast` and collect each +call to a job writer that passes ``decision_reason=``. The value must be a +string literal, or a conditional expression whose branches are all string +literals (the committed / winner-persisted-only site). Anything else fails. + +Known blind spots (documented, not silently tolerated) +------------------------------------------------------ +* Calls are matched by attribute/function NAME, so a writer reached through an + alias or ``getattr`` is invisible. The non-vacuity test below fails if the + scan ever stops finding the known writers, which is what would happen if the + call shape changed wholesale. +* ``services/storage/`` is skipped. That layer does not DECIDE a reason: it + forwards the one it was handed and hydrates the stored value back into the + entity (``PlaybookOptimizationJob(decision_reason=row["decision_reason"])``), + which is a read and would otherwise be a permanent false positive. The + vocabulary is set by the services that call into it, which is what is scanned. +* Only this package is scanned. ``reflexio_ext`` calls + ``update_playbook_optimization_job`` in exactly one place + (``offline_tuner/open_world/runner.py``) and passes no ``decision_reason``; + raw SQL in either package that writes the column directly is also out of + scope -- those sites all assign literals today. +""" + +from __future__ import annotations + +import ast +from pathlib import Path + +SERVER_ROOT = Path(__file__).resolve().parents[4] / "reflexio" / "server" + +WRITER_NAMES = { + "update_playbook_optimization_job", + "create_playbook_optimization_job", + "PlaybookOptimizationJob", +} + + +def _called_name(func: ast.expr) -> str | None: + if isinstance(func, ast.Attribute): + return func.attr + if isinstance(func, ast.Name): + return func.id + return None + + +def _is_fixed_vocabulary(value: ast.expr) -> bool: + """True when the expression can only ever produce a fixed string.""" + if isinstance(value, ast.Constant): + return isinstance(value.value, str) + if isinstance(value, ast.IfExp): + return _is_fixed_vocabulary(value.body) and _is_fixed_vocabulary(value.orelse) + return False + + +def _decision_reason_writes() -> list[tuple[str, int, bool]]: + """Collect ``(relative_path, lineno, is_fixed)`` for every writer call.""" + found: list[tuple[str, int, bool]] = [] + for path in sorted(SERVER_ROOT.rglob("*.py")): + if "storage" in path.relative_to(SERVER_ROOT).parts: + continue + tree = ast.parse(path.read_text(encoding="utf-8"), filename=str(path)) + for node in ast.walk(tree): + if not isinstance(node, ast.Call): + continue + if _called_name(node.func) not in WRITER_NAMES: + continue + for keyword in node.keywords: + if keyword.arg != "decision_reason": + continue + found.append( + ( + str(path.relative_to(SERVER_ROOT)), + keyword.value.lineno, + _is_fixed_vocabulary(keyword.value), + ) + ) + return found + + +def test_scan_actually_finds_the_known_writers(): + """Non-vacuity: a scan that finds nothing would pass the guard below.""" + writes = _decision_reason_writes() + assert len(writes) >= 3, f"scan found {len(writes)} writers; it has gone blind" + optimizer_writes = [ + w for w in writes if w[0].endswith("playbook_optimizer/optimizer.py") + ] + assert optimizer_writes, f"optimizer.py contributed no writers; scanned {writes}" + + +def test_every_decision_reason_write_is_a_fixed_string(): + dynamic = [ + (path, lineno) for path, lineno, fixed in _decision_reason_writes() if not fixed + ] + assert not dynamic, ( + "decision_reason must be a fixed controlled-vocabulary string, never an " + "expression -- an exception message or other interpolated value can carry " + f"customer content into a durable column. Offending writes: {dynamic}" + ) diff --git a/tests/server/services/playbook_optimizer/test_playbook_optimizer.py b/tests/server/services/playbook_optimizer/test_playbook_optimizer.py index d43fc2a6e..035e9c53e 100644 --- a/tests/server/services/playbook_optimizer/test_playbook_optimizer.py +++ b/tests/server/services/playbook_optimizer/test_playbook_optimizer.py @@ -74,6 +74,7 @@ def _sqlite_storage(tmp_path): def _optimizer_for_test(storage, config) -> PlaybookOptimizer: context = SimpleNamespace( storage=storage, + org_id="opt-test", configurator=SimpleNamespace(get_config=lambda: config), ) llm_client = SimpleNamespace(config=SimpleNamespace(model="fake-model")) @@ -1481,3 +1482,65 @@ def callback() -> PlaybookOptimizationRunStatus: ) assert fired.wait(timeout=5), "scheduled callback never fired" + + +def test_optimizer_failure_does_not_persist_exception_message(tmp_path): + """A failed run records a controlled reason, never the exception message. + + ``decision_reason`` is a durable column read back into the domain model and + shown to operators. An arbitrary exception message can carry customer + content (a pydantic ``ValidationError`` on a provider response renders the + model's own output into its message), so the failure path must not write it. + """ + storage = _sqlite_storage(tmp_path) + config = Config( + storage_config=StorageConfigSQLite(db_path=str(tmp_path / "reflexio.db")), + playbook_optimizer_config=PlaybookOptimizerConfig( + enabled=True, + optimize_agent_playbooks=True, + webhook_url="https://assistant.example.test/rollout", + auto_update_pending_agent_playbooks=True, + min_commit_windows=1, + min_commit_score=0.1, + min_commit_likert=1, + ), + ) + optimizer = _optimizer_for_test(storage, config) + incumbent = AgentPlaybook( + agent_playbook_id=1, + playbook_name="support", + agent_version="v1", + content="incumbent", + playbook_status=PlaybookStatus.PENDING, + ) + optimizer._load_incumbent = Mock(return_value=incumbent) # type: ignore[method-assign] + optimizer._resolve_windows = Mock( # type: ignore[method-assign] + return_value=[_scenario_window(idx) for idx in range(1, 6)] + ) + + # Stands in for customer content that a real exception message would carry. + sentinel = "SENTINEL-c7e41f-my-bank-account-is-frozen" + message = f"1 validation error for Output\n input_value='{sentinel}'" + assert sentinel in message, "test is vacuous unless the message carries it" + + def raising_run_gepa(*args, **kwargs): # noqa: ARG001 + raise ValueError(message) + + optimizer._run_gepa = raising_run_gepa # type: ignore[method-assign] + + status = optimizer.optimize( + PlaybookOptimizationTarget(kind="agent_playbook", target_id=1) + ) + + assert status == "failed" + rows = storage.conn.execute( + "SELECT status, decision_reason, metadata_json FROM playbook_optimization_jobs" + ).fetchall() + assert len(rows) == 1 + assert rows[0]["status"] == "failed" + persisted_reason = rows[0]["decision_reason"] + assert sentinel not in persisted_reason + assert sentinel not in rows[0]["metadata_json"] + # Pinned literally, not against the source constant: an assertion that + # imported the constant would follow it if someone changed it back. + assert persisted_reason == "optimization run raised an unexpected error"