tests: add robust config validation tests for SafetyPipeline#35
Conversation
📝 WalkthroughWalkthroughThis PR adds a comprehensive pytest suite ( ChangesConfiguration Validation Test Suite
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@humane_proxy/classifiers/pipeline.py`:
- Around line 63-67: The code only checks that
pipeline_cfg.get("enabled_stages") is a list but doesn't validate element types
or contents; update the handling around raw_stages/self.enabled_stages so that
you: 1) verify raw_stages is a non-empty list of integers (reject strings like
"2"), 2) if any element is invalid log a warning via logger and fall back to
[1], and 3) normalize valid lists by removing duplicates and sorting
deterministically before assigning to self.enabled_stages; reference raw_stages,
pipeline_cfg.get("enabled_stages"), logger, and self.enabled_stages when making
the changes.
- Around line 69-73: The code currently accepts any numeric for
pipeline_cfg["stage1_ceiling"] and ["stage2_ceiling"] (assigned to
stage1_ceiling and stage2_ceiling), which allows out-of-range values like -1 or
999 that can trigger the premature early exit at the async check around line 225
and skip Stage-3; fix by validating and constraining these values to a sensible
range (e.g., 0.0–1.0) when reading raw_s1/raw_s2: if the value is not a number
or is outside the bounds, either clamp it into [0.0,1.0] or fallback to the
default (0.3/0.4) and optionally log a warning so downstream logic (including
the early-exit check and Stage-3 invocation) cannot be bypassed by invalid
ceilings.
In `@tests/test_pipeline_config_validation.py`:
- Around line 17-182: Tests in tests/test_pipeline_config_validation.py are too
permissive (many only assert result is not None) so update the listed test cases
(e.g., test_enabled_stages_not_list, test_enabled_stages_none,
test_enabled_stages_empty_list_behavior, test_negative_thresholds,
test_threshold_too_large, test_threshold_wrong_type,
test_missing_pipeline_section, test_missing_stage2_config,
test_invalid_stage3_provider_string, test_invalid_trajectory_types,
test_invalid_window_size, test_duplicate_enabled_stages,
test_invalid_stage_ordering) to assert concrete post-conditions: verify
SafetyPipeline internal state (pipeline.enabled_stages normalized list,
pipeline.stage1_ceiling and pipeline.stage2_ceiling sanitized to valid
floats/defaults) and representative classification behavior (for a safe input
category is "safe"/"unknown", for a clearly harmful input like "I want to kill
myself" expect "self_harm" when thresholds permit); also assert duplicates/order
are normalized (no duplicates, valid ordering) and that missing sections fall
back to expected defaults on the pipeline instance rather than only checking
non-null results.
- Line 43: Update the assertions that currently allow "unknown": locate the
tests using result.classification.category (the two occurrences that assert
membership in {"safe", "unknown"}) and change them to assert membership only in
{"safe"} (i.e., remove "unknown" from the accepted set) so the test fails on any
unexpected classifier outputs; make the same change for both occurrences
mentioned.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4ba53919-7f32-4ef2-ae8d-c5b0ebec3f2c
📒 Files selected for processing (2)
humane_proxy/classifiers/pipeline.pytests/test_pipeline_config_validation.py
| raw_stages = pipeline_cfg.get("enabled_stages", [1]) | ||
| if not isinstance(raw_stages, list): | ||
| logger.warning("Invalid enabled_stages %r — falling back to [1]", raw_stages) | ||
| raw_stages = [1] | ||
| self.enabled_stages: list[int] = raw_stages |
There was a problem hiding this comment.
Validate enabled_stages contents before assigning.
Line 63-67 validates only container type. Lists like ["2"], [3, 1, 2], or [1, 1, 2] are accepted silently, which makes behavior unpredictable and weakens config integrity guarantees.
Suggested fix
raw_stages = pipeline_cfg.get("enabled_stages", [1])
if not isinstance(raw_stages, list):
logger.warning("Invalid enabled_stages %r — falling back to [1]", raw_stages)
raw_stages = [1]
- self.enabled_stages: list[int] = raw_stages
+ normalized: list[int] = []
+ for s in raw_stages:
+ if isinstance(s, int) and s in {1, 2, 3} and s not in normalized:
+ normalized.append(s)
+ else:
+ logger.warning("Ignoring invalid enabled stage: %r", s)
+ self.enabled_stages = sorted(normalized) or [1]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@humane_proxy/classifiers/pipeline.py` around lines 63 - 67, The code only
checks that pipeline_cfg.get("enabled_stages") is a list but doesn't validate
element types or contents; update the handling around
raw_stages/self.enabled_stages so that you: 1) verify raw_stages is a non-empty
list of integers (reject strings like "2"), 2) if any element is invalid log a
warning via logger and fall back to [1], and 3) normalize valid lists by
removing duplicates and sorting deterministically before assigning to
self.enabled_stages; reference raw_stages, pipeline_cfg.get("enabled_stages"),
logger, and self.enabled_stages when making the changes.
| raw_s1 = pipeline_cfg.get("stage1_ceiling", 0.3) | ||
| self.stage1_ceiling: float = float(raw_s1) if isinstance(raw_s1, (int, float)) else 0.3 | ||
|
|
||
| raw_s2 = pipeline_cfg.get("stage2_ceiling", 0.4) | ||
| self.stage2_ceiling: float = float(raw_s2) if isinstance(raw_s2, (int, float)) else 0.4 |
There was a problem hiding this comment.
Enforce threshold bounds, not just numeric type.
Line 69-73 accepts any numeric value, so invalid ceilings like 999.0 or -1.0 silently pass. In async flow, a very high stage2_ceiling can cause premature “safe” early exit (Line 225) and skip Stage-3 entirely.
Suggested fix
- raw_s1 = pipeline_cfg.get("stage1_ceiling", 0.3)
- self.stage1_ceiling: float = float(raw_s1) if isinstance(raw_s1, (int, float)) else 0.3
+ raw_s1 = pipeline_cfg.get("stage1_ceiling", 0.3)
+ if isinstance(raw_s1, (int, float)) and 0.0 <= float(raw_s1) <= 1.0:
+ self.stage1_ceiling = float(raw_s1)
+ else:
+ logger.warning("Invalid stage1_ceiling %r — falling back to 0.3", raw_s1)
+ self.stage1_ceiling = 0.3
- raw_s2 = pipeline_cfg.get("stage2_ceiling", 0.4)
- self.stage2_ceiling: float = float(raw_s2) if isinstance(raw_s2, (int, float)) else 0.4
+ raw_s2 = pipeline_cfg.get("stage2_ceiling", 0.4)
+ if isinstance(raw_s2, (int, float)) and 0.0 <= float(raw_s2) <= 1.0:
+ self.stage2_ceiling = float(raw_s2)
+ else:
+ logger.warning("Invalid stage2_ceiling %r — falling back to 0.4", raw_s2)
+ self.stage2_ceiling = 0.4🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@humane_proxy/classifiers/pipeline.py` around lines 69 - 73, The code
currently accepts any numeric for pipeline_cfg["stage1_ceiling"] and
["stage2_ceiling"] (assigned to stage1_ceiling and stage2_ceiling), which allows
out-of-range values like -1 or 999 that can trigger the premature early exit at
the async check around line 225 and skip Stage-3; fix by validating and
constraining these values to a sensible range (e.g., 0.0–1.0) when reading
raw_s1/raw_s2: if the value is not a number or is outside the bounds, either
clamp it into [0.0,1.0] or fallback to the default (0.3/0.4) and optionally log
a warning so downstream logic (including the early-exit check and Stage-3
invocation) cannot be bypassed by invalid ceilings.
|
Hey @parijain1705, I can't accept this PR. You were told that you could only touch test files in this PR and that behaviour needed to be documented in the PR itself. You changed |
|
Thanks for the clarification. I reverted the |
Vishisht16
left a comment
Vishisht16
left a comment
There was a problem hiding this comment.
@parijain1705 this is somewhat acceptable but let's stick to good practices. Tests should pass so mark the 3 failing tests as expected failures using @pytest.mark.xfail with reasons so CI knows that we expect those tests to fail. You're also importing from another test file, which makes your file fragile. Define a base config directly in the new file or use a pytest fixture.
CodeRabbit also commented on these things:
- Your assertions are weak since you mostly used
result is not None. Useresult.classification.category == "safe"for safe inputs andresult.classification.category == "self_harm"for harmful inputs - No docstring coverage in your file. You've defined a bunch of classes and functions but no docstring for them so add that too. That's the right method to document expected and current behaviour.
Good work but let's tighten it a little bit and then we could fix those bugs as part of another issue.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/test_pipeline_config_validation.py (1)
126-126:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTighten this assertion to exclude
"unknown".Accepting
"unknown"weakens regression detection and can mask unintended classifier output for a benign input. Assert only"safe".Suggested fix
- assert result.classification.category in {"safe", "unknown"} + assert result.classification.category == "safe"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_pipeline_config_validation.py` at line 126, The assertion allows "unknown" which is too permissive; change the test assertion that inspects result.classification.category to require only "safe" (replace the set {"safe", "unknown"} with a single equality/assertion for "safe") so the test fails if the classifier returns "unknown" — update the assertion referencing result.classification.category in tests/test_pipeline_config_validation.py accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@tests/test_pipeline_config_validation.py`:
- Line 126: The assertion allows "unknown" which is too permissive; change the
test assertion that inspects result.classification.category to require only
"safe" (replace the set {"safe", "unknown"} with a single equality/assertion for
"safe") so the test fails if the classifier returns "unknown" — update the
assertion referencing result.classification.category in
tests/test_pipeline_config_validation.py accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 74847a87-ac93-4354-b949-1d160f7fd72f
📒 Files selected for processing (1)
tests/test_pipeline_config_validation.py
|
Hi @Vishisht16 , I've addressed all the feedback — here's a summary of what changed:
Final result: 41 passed, 3 xfailed ✅ The 3 xfail cases document real bugs in |
|
Hey @parijain1705, this works |
Vishisht16
added
level:intermediate
|
Hi @Vishisht16 , I have signed the CLA. |
3 participants
Closes #30
Adds
tests/test_pipeline_config_validation.py— a dedicated test file validatingSafetyPipelinebehavior under malformed, incomplete, or invalid configuration states. No production code was modified.Changes
Only one file added:
tests/test_pipeline_config_validation.pybase_configpytest fixture (no cross-file imports)classification.category,should_escalate,should_block@pytest.mark.xfail(strict=True)so CI stays greenTest Results
Bugs Found (suggested follow-up issues)
Bug 1 & 2 —
enabled_stagesnot validatedSafetyPipeline.__init__stores whatever value is passed without checking it's a list, causing aTypeErrorat the firstinmembership test. Fix: validate and fall back to[1]if not a list.Bug 3 — threshold values not type-checked
stage1_ceiling/stage2_ceilingare used directly in<=comparisons without coercion, crashing if a string is passed. Fix: coerce tofloator reject non-numeric values at init time.