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"