Repository navigation
fix(ra1): keep double negatives from exempting the action - #957
Conversation
NVIDIA#929 lets a prohibition on recommending an action cover the action when the words between them are proved object syntax. One branch of that grammar accepted any "-ing" word as a gerund object, so a gerund that negates the action also passed: "Never recommend skipping `self-update`" instructs the self-update, yet RA1 dropped it as a prohibition and the HIGH Self-Modification finding vanished. Restrict the gerund branch to a closed list of verbs that perform the action, in line with the default-deny grammar the rest of the gap uses. "Do not advise running `self-update`" stays exempt. Signed-off-by: Udaya Tejas <udayatejas2004@gmail.com>
rng1995
left a comment
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Value and readiness
The focused correction restores the reported detection while preserving the intended prohibition exemption. Ready for code approval.
Material findings
No required changes found. Replacing arbitrary [a-z]+ing with the closed action-performing vocabulary prevents skipping/delaying/avoiding from being consumed as proof of prohibition. All three reported double negatives now fail that exemption grammar. The existing Do not advise running self-update positive exemption and affirmative, separate-clause, arbitrary-action controls remain intact.
PIC tradeoffs
Unknown benign gerunds retain findings; this is the existing conservative fallback rather than a new suppression.
Verification and gaps
Read the complete two-file diff, pinned logical-line/clause/negation caller and complete 20-case test file, description, linked issue evidence and review/thread history. All six exact-head hosted checks succeeded. Findings are supported by source tracing; no before/after runtime score is independently claimed. No contributor code, tests, snippets, imports, builds, model calls or scanner invocations were executed.
Reviewed head: c1570d6361adb1425c8724a2d46a77b71c356ce2.
Fixes #956.
Impact: silent-wrong-result
Who reaches this: any
skillspector scan(CLI or MCPscan_skill) through thestatic_patterns_rogue_agentanalyzer, on a skill whose text puts a negating gerund between a negated recommendation verb and an RA1 phrase.Triggered when: a line like
Never recommend skipping `self-update` after an executable failure.appears inSKILL.md. The sentence instructs the self-update, but the_gerundbranch of #929's object gap (src/skillspector/nodes/analyzers/static_patterns_rogue_agent.py:635) matchesskippingand the whole line is treated as a prohibition.What the user observes: on current
mainthe scan reports no issues, score 0,SAFE, and--fail-on-findingsexits 0. Before #929, and with this change, it reports RA1 HIGH Self-Modification, score 22, exit 1. The regression is not in a release yet.Fix
The gerund branch now takes a closed list of verbs that perform the action (
running,using,executing,invoking, ...) instead of[a-z]+ing, which matches how the classifier branch already works. #929's caseDo not advise running `self-update`stays exempt. Any other gerund gets no exemption, which is the direction the existing comment asks for.Tests
Three cases are added to
test_affirmative_or_ungoverned_recommendation_still_flagsintests/nodes/analyzers/test_rogue_agent_negated_recommendation.py:skipping,delayingandavoiding.Negative control: fix reverted, new tests kept:
With the fix: 3 passed, and the whole file gives 20 passed. The RA1/negation/prohibition tests in the neighbouring analyzer and unit test files give 497 passed.
ruff checkandruff format --checkare clean.Signed-off-by: Udaya Tejas udayatejas2004@gmail.com