fix: normalize multi-skill risk_score before threshold check - #368
fix: normalize multi-skill risk_score before threshold check#368andrewwhitecdw wants to merge 1 commit into
Conversation
fb7980f to
0970996
Compare
rng1995
left a comment
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Requesting changes because this head cannot be assessed confidently as the claimed focused fix. It contains 18,513 additions and 1,868 deletions across 143 files, has no completed checks on the current head, and conflicts with main. Please rebase/split to a reviewable patch and obtain green CI; manual review is required before approval.
0970996 to
11b2cb4
Compare
| score = result.get("risk_score") or 0 | ||
| if isinstance(score, int) and score > max_score: | ||
| try: | ||
| score = int(score) |
There was a problem hiding this comment.
[P1] Add a regression for the string-score exit-code path
This is the only behavior change, but no test exercises it. Add a multi-skill regression where risk_score is a numeric string and assert the aggregate score/exit status honors it; also cover the malformed-value fallback. Otherwise this correctness fix can silently regress.
rng1995
left a comment
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Re-review: still requesting changes. The prior scope/conflict blocker is resolved—the current diff is focused and CI is green—but the new string-to-integer risk-score path has no regression test. Add coverage proving a numeric string affects the aggregate score/exit status and malformed input falls back safely; see the inline blocker.
|
@rng1995 added the regression test you requested in
Both tests pass locally: |
03bfed7 to
2749a9c
Compare
`_scan_multi_skill` checked `isinstance(score, int)` before comparing a skill's `risk_score` against the running maximum. When the score is stored as a string (e.g., deserialized from JSON), the check failed and the value was ignored, so the aggregate exit code did not reflect a high-risk skill. Convert the score to `int` safely with a fallback to `0`, then compare it unconditionally. Add regression tests covering numeric-string scores and the malformed-value fallback. Signed-off-by: andrewwhitecdw <andrewwhitecdw@users.noreply.github.com>
f564fc6 to
401c20e
Compare
2 participants
No issue filed — this is a correctness fix for aggregate risk scoring in multi-skill scans.
Description
_scan_multi_skillinsrc/skillspector/cli.pycheckedisinstance(score, int)before comparing a skill'srisk_scoreagainst the running maximum. When the score is stored as a string (e.g., deserialized from JSON), the check failed and the value was ignored, so the aggregate exit code did not reflect a high-risk skill. This change converts the score tointsafely with a fallback to0, then compares it unconditionally.Changes
src/skillspector/cli.py: safely coercerisk_scoretointbefore the threshold comparison in_scan_multi_skill.tests/unit/test_cli.py: add regression tests for numeric-string scores and malformed-value fallback.Testing
Added two regression tests:
test_recursive_scan_string_risk_score_counts_toward_exit_code— mocks two skills with string risk scores ("25"and"75") and asserts the aggregate exits with code 1 because the max coerced score (75) crosses the threshold.test_recursive_scan_malformed_risk_score_falls_back_to_zero— mocks a skill withrisk_score="not-a-number"and asserts the aggregate max score falls back to 0 without raising.Both pass locally with
uv run pytest tests/unit/test_cli.py::test_recursive_scan_string_risk_score_counts_toward_exit_code tests/unit/test_cli.py::test_recursive_scan_malformed_risk_score_falls_back_to_zero -xvs.