Skip to content

Commit da8e734

Browse files
titaiwangmsCopilot
andcommitted
polish: address reviewer feedback on PR #28083
- Add missing silently-ignore comment in backend.py prepare() - Improve exclusion comment style to per-attribute in backend.py - Fix circular self-reference in run_model() docstring - Rewrite SKILL.md rule 3 for cold readers - Add error message assertion to test_run_model_with_blocked_run_option_raises - Add tests for unknown kwargs through rep.run() and run_model() Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Agent-signed-off: Developer (1003864e) [claude-opus-4.6] Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent bcd48a1 commit da8e734

3 files changed

Lines changed: 26 additions & 5 deletions

File tree

.agents/skills/python-kwargs-setattr-security/SKILL.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ for k, v in kwargs.items():
4242

4343
1. **Use the existing object** in `hasattr(options, k)` — never `hasattr(ClassName(), k)` (creates throwaway C++ objects per iteration)
4444
2. **RuntimeError** is the ORT convention for API misuse errors (not ValueError)
45-
3. **Silent ignore for RunOptions** when the same kwargs are forwarded from a SessionOptions path (e.g. `run_model()` sends kwargs to both `prepare` and `rep.run`)
45+
3. **Silent ignore for one path is OK when kwargs are forwarded to both paths**: `run_model()` passes the same kwargs dict to both `prepare()` (validates SessionOptions) and `rep.run()` (validates RunOptions). A RunOptions kwarg unknown to SessionOptions is silently ignored by `prepare()` — this is correct because `rep.run()` will validate it. Only raise RuntimeError when the attr exists on the target object but is blocked.
4646
4. **Frozenset constant naming**: `_ALLOWED_<CLASSNAME>` — ALL_CAPS, Google Style
4747
5. **No type annotations** on module-level constants (ORT Python convention)
4848

onnxruntime/python/backend/backend.py

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,8 +18,10 @@
1818
from onnxruntime.backend.backend_rep import OnnxRuntimeBackendRep
1919

2020
# Allowlist of SessionOptions attributes that are safe to set via the backend API.
21-
# Dangerous attributes (e.g. optimized_model_filepath, profile_file_prefix, enable_profiling)
22-
# are intentionally excluded to prevent arbitrary file writes through user-controlled kwargs.
21+
# Dangerous attributes intentionally excluded:
22+
# optimized_model_filepath — triggers Model::Save(), overwrites arbitrary files
23+
# profile_file_prefix — writes profiling JSON to arbitrary path
24+
# enable_profiling — causes uncontrolled file writes to cwd
2325
_ALLOWED_SESSION_OPTIONS = frozenset(
2426
{
2527
"enable_cpu_mem_arena",
@@ -140,6 +142,7 @@ def prepare(cls, model, device=None, **kwargs):
140142
f"SessionOptions attribute '{k}' is not permitted via the backend API. "
141143
f"Allowed attributes: {', '.join(sorted(_ALLOWED_SESSION_OPTIONS))}"
142144
)
145+
# else: silently ignore unknown keys
143146

144147
excluded_providers = os.getenv("ORT_ONNX_BACKEND_EXCLUDE_PROVIDERS", default="").split(",")
145148
providers = [x for x in get_available_providers() if (x not in excluded_providers)]
@@ -182,7 +185,7 @@ def run_model(cls, model, inputs, device=None, **kwargs):
182185
None means the default one which depends on
183186
the compilation settings
184187
:param kwargs: accepted keys are the union of ``_ALLOWED_SESSION_OPTIONS``
185-
(see ``backend.py``) and ``_ALLOWED_RUN_OPTIONS`` (see ``backend_rep.py``).
188+
(defined in this module) and ``_ALLOWED_RUN_OPTIONS`` (see ``backend_rep.py``).
186189
Kwargs are passed directly to both ``prepare()`` and ``rep.run()``; each
187190
validates against its own allowlist and raises ``RuntimeError`` for blocked
188191
attributes. Logging-related kwargs (``log_severity_level``,

onnxruntime/test/python/onnxruntime_test_python_backend.py

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -158,8 +158,26 @@ def test_run_model_with_blocked_run_option_raises(self):
158158
"""run_model() must raise RuntimeError when given a blocked RunOptions attribute."""
159159
name = get_name("mul_1.onnx")
160160
x = np.array([[1.0, 2.0], [3.0, 4.0], [5.0, 6.0]], dtype=np.float32)
161-
with self.assertRaises(RuntimeError):
161+
with self.assertRaises(RuntimeError) as ctx:
162162
backend.run(name, [x], terminate=True)
163+
self.assertIn("not permitted", str(ctx.exception))
164+
165+
def test_unknown_kwarg_is_silently_ignored_in_run(self):
166+
"""A kwarg unknown to RunOptions must be silently ignored by rep.run()."""
167+
name = get_name("mul_1.onnx")
168+
rep = backend.prepare(name)
169+
x = np.array([[1.0, 2.0], [3.0, 4.0], [5.0, 6.0]], dtype=np.float32)
170+
res = rep.run(x, completely_unknown_key="bar")
171+
output_expected = np.array([[1.0, 4.0], [9.0, 16.0], [25.0, 36.0]], dtype=np.float32)
172+
np.testing.assert_allclose(res[0], output_expected, rtol=1e-05, atol=1e-08)
173+
174+
def test_unknown_kwarg_is_silently_ignored_in_run_model(self):
175+
"""An unknown kwarg must be silently ignored by both prepare() and rep.run() in run_model()."""
176+
name = get_name("mul_1.onnx")
177+
x = np.array([[1.0, 2.0], [3.0, 4.0], [5.0, 6.0]], dtype=np.float32)
178+
res = backend.run(name, [x], completely_unknown_key="baz")
179+
output_expected = np.array([[1.0, 4.0], [9.0, 16.0], [25.0, 36.0]], dtype=np.float32)
180+
np.testing.assert_allclose(res[0], output_expected, rtol=1e-05, atol=1e-08)
163181

164182
def test_run_model_with_blocked_session_option_raises(self):
165183
"""run_model() must raise RuntimeError when given a blocked SessionOptions attribute."""

0 commit comments

Comments
 (0)