fix(playbook-optimizer): stop persisting raw exception text in decision_reason

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"
 

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}"
+    )

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"