fix(library): unblock reasoning models in self-check and content-safety actions - #1816
Merged
Merged
Conversation
…ty actions
Reasoning models (OpenAI o-series, gpt-5, DeepSeek-R1, Gemini 2.5,
Qwen QwQ, etc.) spend output tokens on internal reasoning before
producing visible text. The library's safety/self-check actions
defaulted max_tokens to 3 (or 10 for topic safety), which reasoning
models consume entirely on thinking, returning empty content with
finish_reason='length'. Callers saw a silent "" result and the checks
produced nonsense.
- Bump the fallback _MAX_TOKENS from 3 to 1024 in:
self_check/input_check, self_check/output_check, self_check/facts,
content_safety (both input and output checks).
Classical models still stop early via stop tokens / natural completion,
so cost impact is negligible. User-configured max_tokens (via prompts
config) still wins.
- Add warn_if_truncated() in actions/llm/utils.py: emits a WARNING when
response.content is empty and finish_reason == 'length'. Called from
each patched action so any user who tunes max_tokens too low for a
reasoning model gets an actionable log line instead of silent failure.
- Unit-test warn_if_truncated for empty-with-length, non-empty, and
non-length finish-reason cases.
topic_safety is not affected (its max_tokens is computed but never
passed to llm_call in the current code). Not touched here.
Contributor
Greptile SummaryThis PR fixes silent failures when reasoning models exhaust the small default token budget on internal thinking, by bumping the fallback
|
| Filename | Overview |
|---|---|
| nemoguardrails/actions/llm/utils.py | Adds warn_if_truncated helper that detects empty content + finish_reason='length' and emits a warning; well-implemented with clear docstring and bool return value. |
| nemoguardrails/library/self_check/facts/actions.py | Bumps _MAX_TOKENS to 1024 and adds an early return 0.0 fail-safe on truncation, correctly guarding against the is_content_safe inversion bug on empty input. |
| nemoguardrails/library/self_check/input_check/actions.py | Bumps _MAX_TOKENS to 1024 and wires warn_if_truncated; empty-response case is incidentally fail-safe because is_content_safe('') returns [False] (not safe), causing the action to block. |
| nemoguardrails/library/self_check/output_check/actions.py | Same pattern as input_check; empty-response case is incidentally fail-safe via is_content_safe('') not safe, output blocked by the output_mapping decorator. |
| nemoguardrails/library/content_safety/actions.py | Bumps _MAX_TOKENS to 1024 and wires warn_if_truncated, but the return value is discarded — the fail-safe branch from the docstring is not implemented here, leaving behavior on empty content dependent on the content-safety parser's defaults. |
| tests/integrations/langchain/test_actions_llm_utils.py | Adds four tests covering warn_if_truncated; tests are correct and cover key paths, but placed in the LangChain integration directory despite having no LangChain dependency. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[LLM call via llm_call] --> B{warn_if_truncated}
B -- content empty AND finish_reason=length --> C[Emit WARNING log]
B -- otherwise --> D[Return False, continue normally]
C --> E{Which action?}
E -- self_check_facts --> F["return 0.0 (fail-safe: block output)"]
E -- self_check_input --> G["parse_task_output with empty string"]
E -- self_check_output --> G
E -- content_safety_check_input/output --> G
G --> H{"is_content_safe('') → [False]"}
H -- self_check_input --> I["Block input (fail-safe ✓)"]
H -- self_check_output --> J["is_safe=False → decorator inverts → block (fail-safe ✓)"]
H -- content_safety --> K["Depends on parser default (behavior unchanged from pre-PR)"]
Prompt To Fix All With AI
This is a comment left during a code review.
Path: tests/integrations/langchain/test_actions_llm_utils.py
Line: 641-680
Comment:
**Test placement mismatch with utility scope**
`warn_if_truncated` is a general LLM utility with no LangChain dependency, but its tests are placed in `tests/integrations/langchain/`. The tests import only `nemoguardrails.types.LLMResponse` and `nemoguardrails.actions.llm.utils.warn_if_truncated` — no LangChain fixtures are used. Grouping them here will make them harder to find and could cause them to be skipped if LangChain-specific test prerequisites are added to the suite in the future.
How can I resolve this? If you propose a fix, please make it concise.Reviews (2): Last reviewed commit: "apply review suggestions" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
📝 WalkthroughWalkthroughThis PR introduces a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
nemoguardrails/library/content_safety/actions.py (1)
50-50: Optional: centralize the fallback max-token constant.Both actions use the same fallback value; moving it to a module-level constant will reduce drift risk.
Proposed refactor
log = logging.getLogger(__name__) +DEFAULT_FALLBACK_MAX_TOKENS = 1024 @@ - _MAX_TOKENS = 1024 @@ - max_tokens = max_tokens or _MAX_TOKENS + max_tokens = max_tokens or DEFAULT_FALLBACK_MAX_TOKENS @@ - _MAX_TOKENS = 1024 @@ - max_tokens = max_tokens or _MAX_TOKENS + max_tokens = max_tokens or DEFAULT_FALLBACK_MAX_TOKENSAlso applies to: 151-151
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoguardrails/library/content_safety/actions.py` at line 50, There are duplicate fallback token values (the literal 1024) used in two actions; define a single module-level constant (e.g., FALLBACK_MAX_TOKENS or MAX_TOKENS) at the top of nemoguardrails/library/content_safety/actions.py and replace the duplicated occurrences (including the existing _MAX_TOKENS and the other fallback at line ~151) to reference that constant from functions like the content-safety action handlers so the value is centralized and avoids drift.tests/integrations/langchain/test_actions_llm_utils.py (1)
646-669: Add a whitespace-only truncation test for parity with runtime behavior.Given this suite is the contract for
warn_if_truncated, it should also cover" \n\t"content withfinish_reason="length".Proposed test addition
class TestWarnIfTruncated: @@ def test_silent_on_non_length_finish_reason(self, caplog): from nemoguardrails.types import LLMResponse response = LLMResponse(content="", finish_reason="stop") with caplog.at_level("WARNING"): warn_if_truncated(response, "self_check_input") assert not caplog.records + + def test_warns_on_whitespace_only_content_with_length_finish(self, caplog): + from nemoguardrails.types import LLMResponse + + response = LLMResponse(content=" \n\t", finish_reason="length") + with caplog.at_level("WARNING"): + warn_if_truncated(response, "self_check_input") + assert any("self_check_input" in rec.message and "length" in rec.message for rec in caplog.records)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integrations/langchain/test_actions_llm_utils.py` around lines 646 - 669, Add a new test to TestWarnIfTruncated that asserts warn_if_truncated emits a warning when LLMResponse.content is only whitespace (e.g., " \n\t") and finish_reason=="length"; locate the test class in tests/integrations/langchain/test_actions_llm_utils.py and add a method (e.g., test_warns_on_whitespace_only_content_with_length_finish) that constructs LLMResponse(content=" \n\t", finish_reason="length"), invokes warn_if_truncated(response, "self_check_input") under caplog.at_level("WARNING"), and asserts the warning record contains "self_check_input" and "length".nemoguardrails/actions/llm/utils.py (1)
399-415: Treat whitespace-only content as truncated as well.Right now,
" \n\t"won’t trigger the warning even though it behaves like empty output for parsing and safety checks.Proposed patch
def warn_if_truncated(response: LLMResponse, task: str) -> None: @@ - if not response.content and response.finish_reason == "length": + content = response.content or "" + if not content.strip() and response.finish_reason == "length": logger.warning(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoguardrails/actions/llm/utils.py` around lines 399 - 415, The current warn_if_truncated function only triggers when response.content is falsy, missing cases where content is whitespace-only (e.g., " \n\t"); update the condition in warn_if_truncated to treat whitespace-only content as empty by checking response.content.strip() (while handling None safely) together with response.finish_reason == "length" so the logger.warning branch fires for whitespace-only outputs; reference symbols: warn_if_truncated, response.content, response.finish_reason, and logger.warning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@nemoguardrails/actions/llm/utils.py`:
- Around line 399-415: The current warn_if_truncated function only triggers when
response.content is falsy, missing cases where content is whitespace-only (e.g.,
" \n\t"); update the condition in warn_if_truncated to treat whitespace-only
content as empty by checking response.content.strip() (while handling None
safely) together with response.finish_reason == "length" so the logger.warning
branch fires for whitespace-only outputs; reference symbols: warn_if_truncated,
response.content, response.finish_reason, and logger.warning.
In `@nemoguardrails/library/content_safety/actions.py`:
- Line 50: There are duplicate fallback token values (the literal 1024) used in
two actions; define a single module-level constant (e.g., FALLBACK_MAX_TOKENS or
MAX_TOKENS) at the top of nemoguardrails/library/content_safety/actions.py and
replace the duplicated occurrences (including the existing _MAX_TOKENS and the
other fallback at line ~151) to reference that constant from functions like the
content-safety action handlers so the value is centralized and avoids drift.
In `@tests/integrations/langchain/test_actions_llm_utils.py`:
- Around line 646-669: Add a new test to TestWarnIfTruncated that asserts
warn_if_truncated emits a warning when LLMResponse.content is only whitespace
(e.g., " \n\t") and finish_reason=="length"; locate the test class in
tests/integrations/langchain/test_actions_llm_utils.py and add a method (e.g.,
test_warns_on_whitespace_only_content_with_length_finish) that constructs
LLMResponse(content=" \n\t", finish_reason="length"), invokes
warn_if_truncated(response, "self_check_input") under
caplog.at_level("WARNING"), and asserts the warning record contains
"self_check_input" and "length".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1b51d7bf-f521-4599-a4cc-25b28d3d2582
📒 Files selected for processing (6)
nemoguardrails/actions/llm/utils.pynemoguardrails/library/content_safety/actions.pynemoguardrails/library/self_check/facts/actions.pynemoguardrails/library/self_check/input_check/actions.pynemoguardrails/library/self_check/output_check/actions.pytests/integrations/langchain/test_actions_llm_utils.py
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
tgasser-nv
approved these changes
Apr 24, 2026
tgasser-nv
left a comment
tgasser-nv
left a comment
Collaborator
There was a problem hiding this comment.
Please add tests to cover self_check_facts as well before merging and make sure all lines are covered. Not sure how we can set the max tokens without it being brittle, but 3 -> 1024 definitely helps
trebedea
approved these changes
Apr 24, 2026
trebedea
left a comment
trebedea
left a comment
Member
There was a problem hiding this comment.
This looks good, we need to handle reasoning models as guards.
In many use-cases that have complex custom policies, reasoning guards are the standard and there are plenty of such models: Nemotron Content Safety Reasoning, Granite Guardian (latest version released at the end of 2025 is a reasoning guard), gpt-oss-safeguard.
We need to also mention something about this in the documentation, e.g. if you are using a reasoning guard model please set the corresponding MAX_TOKENS in the task and we should also document that we have a default limit of 1024 if no limit is specified.
Otherwise, I agree with using a warning instead of throwing an exception for incomplete reasoning - I guess this might be useful for the main LLM as well, no?
Already talked with @Pouyanpi about this.
Collaborator
Author
|
docs at #1833 |
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
3 participants
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.
Description
Reasoning models (OpenAI o-series, gpt-5, DeepSeek-R1, etc.) spend output tokens on internal reasoning before producing visible text. The library's safety/self-check actions defaulted max_tokens to 3 (or 10 for topic safety), which reasoning models consume entirely on thinking, returning empty content with finish_reason='length'. Callers saw a silent "" result and the checks produced nonsense.
bump the fallback
_MAX_TOKENSfrom 3 to 1024 in: self_check/input_check, self_check/output_check, self_check/facts, content_safety (both input and output checks). Classical models still stop early via stop tokens / natural completion, so cost impact is negligible. configured max_tokens (via prompts config) still wins.add
warn_if_truncated(): emits a warning whenresponse.contentis empty andfinish_reason == 'length'. Called from each patched action so any user who tunes max_tokens too low for a reasoning model gets an actionable log line instead of silent failure.is it better to raise an error instead?
Summary by CodeRabbit
New Features
Improvements
Tests