From 2ef22e69e70086d2ef23ad66fc613253c9e3b895 Mon Sep 17 00:00:00 2001 From: Andrew Chen <48723787+chuenchen309@users.noreply.github.com> Date: Sun, 12 Jul 2026 00:35:12 +0800 Subject: [PATCH 1/2] Remove crashing extra_params.append in NeMoGuardrailsServer.__init__ __init__ ran: if self.extra_params and not self.extra_params.get("extra_body"): self.extra_params.append("extra_body") but extra_params is a dict (OpenAICompatible default {}, consumed via .items()), so .append raises AttributeError. Any user who sets a non-empty extra_params without an 'extra_body' key crashes generator construction. The block is also vestigial: guardrails config is delivered through the self.extra_body attribute (set just below and passed to the client by the base class). And a naive 'fix' of setting extra_params["extra_body"] = {} would be worse -- OpenAICompatible._call_model spreads extra_params into the request after applying self.extra_body, so it would overwrite the real guardrails config with an empty dict. Since the block only ever crashes or is skipped (it never completed successfully), removing it changes no working behaviour. Existing rail-selection tests still pass. Co-Authored-By: Claude Opus 4.8 Signed-off-by: Andrew Chen <48723787+chuenchen309@users.noreply.github.com> --- garak/generators/guardrails.py | 2 -- tests/generators/test_guardrails.py | 21 +++++++++++++++++++++ 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/garak/generators/guardrails.py b/garak/generators/guardrails.py index 30a5177da..ba7a280aa 100644 --- a/garak/generators/guardrails.py +++ b/garak/generators/guardrails.py @@ -72,8 +72,6 @@ class NeMoGuardrailsServer(OpenAICompatible): def __init__(self, name="", config_root=_config): self.api_key = "not-used" # suppress any api_key from being sent as the server does not utilize one super().__init__(name, config_root) - if self.extra_params and not self.extra_params.get("extra_body"): - self.extra_params.append("extra_body") guardrails = None if self.config_ids: diff --git a/tests/generators/test_guardrails.py b/tests/generators/test_guardrails.py index 06f77fa87..34da1ebf8 100644 --- a/tests/generators/test_guardrails.py +++ b/tests/generators/test_guardrails.py @@ -51,3 +51,24 @@ def test_guardrail_selection(selected_rails, respx_mock, openai_compat_mocks): assert "guardrails" in content assert "config_ids" in content assert rail in content + + +def test_nonempty_extra_params_does_not_crash_init(): + """Construction with a non-empty extra_params lacking 'extra_body' must not crash. + + The removed block ran ``self.extra_params.append("extra_body")`` — .append + on a dict — which raised AttributeError during __init__ for any user who + set extra_params without an 'extra_body' key. + """ + config_root = { + "generators": { + "guardrails": { + "NeMoGuardrailsServer": { + "name": "UnknownModel", + "extra_params": {"foo": 1}, + } + } + } + } + g = NeMoGuardrailsServer(config_root=config_root) + assert g.extra_params == {"foo": 1} From 814f5cc50f3d088c1e4d65561065214499fdd99c Mon Sep 17 00:00:00 2001 From: Andrew Chen <48723787+chuenchen309@users.noreply.github.com> Date: Tue, 14 Jul 2026 08:53:59 +0800 Subject: [PATCH 2/2] fix: properly merge user extra_body instead of dropping it Addresses review feedback from @jmartin-tech: the prior commit removed the crashing `self.extra_params.append("extra_body")` guard outright, but that left a separate, pre-existing bug in place -- a user-provided `extra_body` arriving nested inside `extra_params` was never merged into `self.extra_body`. Since `OpenAICompatible._call_model()` applies `self.extra_params` *after* the instance's own attributes, a lingering `extra_params["extra_body"]` would silently overwrite the guardrails config injected into `self.extra_body`, with the user's own extra_body also never actually reaching the request in combination with it. Pop `extra_body` out of `extra_params` (if present) and fold it into `self.extra_body` before the existing guardrails-merge logic runs, so the two sources combine instead of one shadowing the other. Also raise a clear ValueError if the popped value isn't a dict, per review suggestion, rather than failing confusingly later. Added two tests: one confirms a user extra_body and the guardrails config both end up in the actual outgoing request body (verified via the existing respx-mocked OpenAI client, per review suggestion to check how these map into the client call); the other confirms a non-dict extra_body raises ValueError at construction time. Confirmed both fail against the pre-fix code and pass after. Ran the full generators/test_guardrails.py + test_openai.py suites (no regressions) and black --check clean. Co-Authored-By: Claude Opus 4.8 Signed-off-by: Andrew Chen <48723787+chuenchen309@users.noreply.github.com> --- garak/generators/guardrails.py | 16 +++++++ tests/generators/test_guardrails.py | 65 +++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+) diff --git a/garak/generators/guardrails.py b/garak/generators/guardrails.py index ba7a280aa..d56213ac1 100644 --- a/garak/generators/guardrails.py +++ b/garak/generators/guardrails.py @@ -73,6 +73,22 @@ def __init__(self, name="", config_root=_config): self.api_key = "not-used" # suppress any api_key from being sent as the server does not utilize one super().__init__(name, config_root) + # A user-provided `extra_body` may arrive nested inside `extra_params` (the generic + # passthrough dict for OpenAICompatible). Left there, it would independently overwrite + # `self.extra_body` in `_call_model()` (extra_params is applied after the instance's own + # attributes), silently dropping the `guardrails` config merged in below. Pop it out and + # fold it into `self.extra_body` instead so the two sources combine rather than one + # shadowing the other. + if self.extra_params and "extra_body" in self.extra_params: + user_extra_body = self.extra_params.pop("extra_body", None) + if user_extra_body: + if not isinstance(user_extra_body, dict): + raise ValueError( + "NeMoGuardrailsServer: `extra_params['extra_body']` must be a dict, got " + f"{type(user_extra_body).__name__}" + ) + self.extra_body = user_extra_body + guardrails = None if self.config_ids: guardrails = {"config_ids": self.config_ids} diff --git a/tests/generators/test_guardrails.py b/tests/generators/test_guardrails.py index 34da1ebf8..69cee77d7 100644 --- a/tests/generators/test_guardrails.py +++ b/tests/generators/test_guardrails.py @@ -1,6 +1,8 @@ # SPDX-FileCopyrightText: Copyright (c) 2024 NVIDIA CORPORATION & AFFILIATES. All rights reserved. # SPDX-License-Identifier: Apache-2.0 +import json + import httpx import pytest @@ -72,3 +74,66 @@ def test_nonempty_extra_params_does_not_crash_init(): } g = NeMoGuardrailsServer(config_root=config_root) assert g.extra_params == {"foo": 1} + + +@pytest.mark.respx(base_url=NeMoGuardrailsServer.DEFAULT_PARAMS["uri"]) +def test_user_extra_body_is_merged_not_shadowed_by_guardrails_config( + respx_mock, openai_compat_mocks +): + """A user-provided extra_body (via extra_params) must combine with, not be + overwritten by, the guardrails config injected into self.extra_body — and + must not remain in extra_params where it would independently overwrite + self.extra_body again in _call_model(). + """ + mock_response = openai_compat_mocks["chat"] + mock_request = respx_mock.post("chat/completions") + mock_request.mock( + return_value=httpx.Response( + mock_response["code"], + json=mock_response["json"], + ) + ) + config_root = { + "generators": { + "guardrails": { + "NeMoGuardrailsServer": { + "name": "UnknownModel", + "config_ids": ["rail1"], + "extra_params": {"extra_body": {"custom_key": "custom_value"}}, + } + } + } + } + g = NeMoGuardrailsServer(config_root=config_root) + + assert "extra_body" not in g.extra_params + assert g.extra_body == { + "custom_key": "custom_value", + "guardrails": {"config_ids": ["rail1"]}, + } + + conv = Conversation(turns=[Turn(role="user", content=Message("Testing text"))]) + g.generate(conv) + assert mock_request.called + sent_body = json.loads(mock_request.calls.last.request.content) + assert sent_body["custom_key"] == "custom_value" + assert sent_body["guardrails"] == {"config_ids": ["rail1"]} + + +def test_non_dict_user_extra_body_raises_value_error(): + """A non-dict extra_params['extra_body'] must raise a clear ValueError at + construction time rather than silently corrupting self.extra_body or + failing later with an unrelated error. + """ + config_root = { + "generators": { + "guardrails": { + "NeMoGuardrailsServer": { + "name": "UnknownModel", + "extra_params": {"extra_body": "not-a-dict"}, + } + } + } + } + with pytest.raises(ValueError, match="must be a dict"): + NeMoGuardrailsServer(config_root=config_root)