fix(playbook-optimizer): stop persisting raw exception text in decision_reason - #476
Open
guangyu-reflexio wants to merge 1 commit into
Open
fix(playbook-optimizer): stop persisting raw exception text in decision_reason#476guangyu-reflexio wants to merge 1 commit into
guangyu-reflexio wants to merge 1 commit into
Conversation
…on_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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (3)
Included review availability: 3 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 4 reviews per hour. 📝 WalkthroughWalkthroughThe optimizer now stores a fixed reason for unexpected failures. Tests verify that exception content is excluded from durable job data. An AST-based guard enforces fixed-string ChangesOptimization failure reason
Decision reason vocabulary guard
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to Failed optimization jobs now retain a controlled failure reason rather than storing arbitrary exception text, reducing exposure of customer-derived content while preserving structured diagnostics and traceback logging. The change is covered by regression and vocabulary-guard tests and is ready to merge. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning Your free Security trial is over. An organization admin can upgrade to Advanced for continuous pull request security review or dismiss this notice. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
PlaybookOptimizerwrote a raw exception string intodecision_reason, a durable column:str(exc)on an arbitrary exception can carry customer content. That is not hypothetical — on a sibling code path apydantic.ValidationErrorraised during playbook analysis embedded customer transcript text, because the model's own output is interpolated into the error message.decision_reasonis read back into the domain model and surfaced to operators.Why this shape of fix
Every other writer of that column uses a fixed, controlled string —
"assistant backend aborted one or more evaluations","best candidate did not pass commit thresholds",'retired_by_replay_redesign'— so the column is already a de facto controlled vocabulary and this line was the only outlier. A tenant Postgres trigger also compares it against literals, so the vocabulary is load-bearing rather than cosmetic.The diagnostic is not lost: the two lines immediately above already bind
error_type=type(exc).__name__into structured logging context and calllogger.exception, which records the traceback where it belongs.The exception class name is deliberately NOT added to the column. It buys nothing that
error_tagsdoes not already carry, and it would widen an operator-facing fixed vocabulary into one keyed on third-party exception types (gepa, litellm, pydantic).Scope
A sweep of 253 candidate expressions across both trees found 86 that reach a durable sink. Only this one is fixed here, on the principle that the defect is content-bearing text written into a column not classified to hold content. Every other DB writer targets a field whose declared purpose is an error string —
_agent_runs.last_error,operation_state.error_message,billing_debit_incident.detail. Redacting those is a different design question that reaches into billing and extraction, and was not silently folded in.The tightest sibling, recorded for follow-up:
gepa_adapter.py:257/273writesrationale=f"Assistant failed: {exc}"intoplaybook_optimization_evaluations.rationale.Existing rows
The line has existed since 2026-05-08 (~4 months). The path is opt-in —
PlaybookOptimizerConfig.enabled=Falseby default and it needs an assistant backend configured — so only orgs that deliberately enabled GEPA can hold such a row.On enterprise Postgres these age out:
_retention_gc_retired_optimization_jobshard-deletes non-pending/running jobs past a cutoff, gated onlineage_gc.enabled(default true) with a 90-day window. So contamination is a rolling window that self-heals. Two caveats: OSS SQLite overrides that hook toreturn 0, so nothing purges there; and whether any org actually enabled the optimizer is an environment question that was not answered by querying anything.Tests
102 passedintests/server/services/playbook_optimizer/, verified on top ofmainrather than only on the branch it was written on.The guard asserts a distinctive sentinel from the exception message is absent from the persisted value — a test that merely checks "the message isn't there" passes trivially when the exception has a generic message. Mutation-verified: restoring
decision_reason=str(exc)and the f-string variant each turn it red.Worth noting: the optimizer's exception path had no test coverage at all before this change — the first run died on the shared fake request context lacking
org_id. Four months of an untested failure path is presumably how the leak survived review.Summary by CodeRabbit
Bug Fixes
Tests