You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Deprecate omitting observability_level on every public API that defaults it, so a caller that misses the argument gets a signal instead of silently keeping the old behavior.
Follow-up to a review comment on #141, filed at Nina Chikanov (@nina-msft)'s request and rescoped to the feedback below.
Motivation
#141 added observability_level to evaluate_turn_async with a compatibility default:
That default keeps every existing caller working, which is the right call for that PR. The cost is that it fails open. A caller that omits the argument treats every adapter as fully observable, which is the bug #140 describes: an evaluator that needs an evidence channel the adapter does not report returns NOT_DETECTED, and resolve_as_attack turns that into SAFE.
The same default is on EvalContext and on EvalContext.from_response. Result defaults the same field the other way, to RESPONSE_ONLY, where it is reporting metadata that every built-in path overwrites with the adapter's actual profile. evaluate_turn_async is exported from rampart.core and docs/contributing/extending-rampart.md presents it as the seam for a custom execution strategy, so out-of-repo callers are the ones most likely to miss it.
No shipped code relies on any of these defaults: every Result and EvalContext built in rampart/ passes observability_level explicitly. The repo's own tests are another matter. Counting the construction sites by AST, 83 calls under tests/ omit the argument: 64 Result, 13 EvalContext, 3 EvalContext.from_response and 3 evaluate_turn_async, including test_observability_level_defaults_to_no_declared_limit, which asserts the very default the warning would be about. Those would need the argument or an explicit filter before the warning lands.
Proposed solution
Making observability_level required is out of scope. The deprecation is:
When the argument is omitted, emit an actionable DeprecationWarning that names the affected API, explains the compatibility fallback being applied, directs the caller to pass the adapter's declared level, normally adapter.observability_profile, and names the removal release.
Warn on omission only. Passing any ObservabilityLevel explicitly, including the value that is the current default, must not warn.
Emit exactly one warning per omitted public call, including wrapper paths such as EvalContext.from_response.
Preserve the current fallback behavior for the whole deprecation window.
Name the removal release under the existing two-minor-version policy in docs/contributing/release-process.md: a warning shipped in 0.x.0 targets 0.(x+2).0.
Update the docs and the generated reference: docs/contributing/extending-rampart.md, the direct Result(...) example in docs/usage/pytest-integration.md, the Result(...) helper in docs/contributing/testing.md, the EvalContext, Result and evaluate_turn_async docstrings, and add evaluate_turn_async to the rampart.core.execution members on docs/api/core-protocols.md, where it is public but absent.
Add tests proving an omitted argument warns once and an explicit value does not warn at all.
Alternatives considered
Leave the defaults. Cheapest, but the failure is silent and points the wrong way for a safety framework.
Make the argument required now. A breaking API change with no migration window, and out of scope per the review discussion.
Default EvalContext to RESPONSE_ONLY to match Result. Fails safe, but it changes behavior for every caller that omits the argument, with no migration window, and makes a fully observable adapter look unobservable, which is its own false signal.
Infer the level inside evaluate_turn_async. It receives the evaluator and the turn data, not the adapter, so there is nothing to infer from.
rampart/common/deprecation.py already provides emit_deprecation_warning(old_item=, new_item=, removed_in=), used for the rampart_sinks fixture with removed_in="0.3.0". Its message maps an old item to a replacement, so an omitted argument needs different wording, and the helper may need a second entry point.
Telling omission apart from an explicit value that happens to equal the default means the parameter and the dataclass fields need a sentinel default rather than an ObservabilityLevel member, with the real fallback applied after the check. That is the part worth agreeing on before implementation.
Happy to take this one if you would like it picked up.
Summary
Deprecate omitting
observability_levelon every public API that defaults it, so a caller that misses the argument gets a signal instead of silently keeping the old behavior.Follow-up to a review comment on #141, filed at Nina Chikanov (@nina-msft)'s request and rescoped to the feedback below.
Motivation
#141 added
observability_leveltoevaluate_turn_asyncwith a compatibility default:That default keeps every existing caller working, which is the right call for that PR. The cost is that it fails open. A caller that omits the argument treats every adapter as fully observable, which is the bug #140 describes: an evaluator that needs an evidence channel the adapter does not report returns
NOT_DETECTED, andresolve_as_attackturns that intoSAFE.The same default is on
EvalContextand onEvalContext.from_response.Resultdefaults the same field the other way, toRESPONSE_ONLY, where it is reporting metadata that every built-in path overwrites with the adapter's actual profile.evaluate_turn_asyncis exported fromrampart.coreanddocs/contributing/extending-rampart.mdpresents it as the seam for a custom execution strategy, so out-of-repo callers are the ones most likely to miss it.No shipped code relies on any of these defaults: every
ResultandEvalContextbuilt inrampart/passesobservability_levelexplicitly. The repo's own tests are another matter. Counting the construction sites by AST, 83 calls undertests/omit the argument: 64Result, 13EvalContext, 3EvalContext.from_responseand 3evaluate_turn_async, includingtest_observability_level_defaults_to_no_declared_limit, which asserts the very default the warning would be about. Those would need the argument or an explicit filter before the warning lands.Proposed solution
Making
observability_levelrequired is out of scope. The deprecation is:observability_level:evaluate_turn_async, directEvalContextconstruction,EvalContext.from_response, and directResultconstruction.DeprecationWarningthat names the affected API, explains the compatibility fallback being applied, directs the caller to pass the adapter's declared level, normallyadapter.observability_profile, and names the removal release.ObservabilityLevelexplicitly, including the value that is the current default, must not warn.EvalContext.from_response.docs/contributing/release-process.md: a warning shipped in0.x.0targets0.(x+2).0.docs/contributing/extending-rampart.md, the directResult(...)example indocs/usage/pytest-integration.md, theResult(...)helper indocs/contributing/testing.md, theEvalContext,Resultandevaluate_turn_asyncdocstrings, and addevaluate_turn_asyncto therampart.core.executionmembers ondocs/api/core-protocols.md, where it is public but absent.Alternatives considered
EvalContexttoRESPONSE_ONLYto matchResult. Fails safe, but it changes behavior for every caller that omits the argument, with no migration window, and makes a fully observable adapter look unobservable, which is its own false signal.evaluate_turn_async. It receives the evaluator and the turn data, not the adapter, so there is nothing to infer from.Additional context
rampart/common/deprecation.pyalready providesemit_deprecation_warning(old_item=, new_item=, removed_in=), used for therampart_sinksfixture withremoved_in="0.3.0". Its message maps an old item to a replacement, so an omitted argument needs different wording, and the helper may need a second entry point.ObservabilityLevelmember, with the real fallback applied after the check. That is the part worth agreeing on before implementation.