Repository navigation
fix(static): stop TR2/TR3 over-reading trigger descriptions (#669) - #709
Conversation
TR2 took the shadowed command from any built-in word in a description
clause, and any slash token or interception verb in the same clause
satisfied the gate. "Use when the user says /ask-matt or wants an ask
answered by Matt" was therefore reported as shadowing the built-in "ask".
The shadowed command now has to be the object of the interception
evidence: a slash token is matched by its whole name ("/ask-matt" is not
"/ask"), and an interception verb only names a built-in within the next
few words before a clause boundary.
TR3 only treated a following "about" as a scope qualifier, so the
description of the widely used pdf skill, "Use this skill whenever the
user wants to do anything with PDF files", was reported as keyword
baiting. Domain qualifiers (with, involving, regarding, concerning,
related/relating to) now bound the scope as "about" does, unless their
object is only a pronoun ("anything with it"). Broad prepositions
(in/for/on) are deliberately not qualifiers, so "any message in the
chat" still fires.
Add benign/risky pairs for both rules. Existing positives ("Intercepts
the /build command", "whenever the user sends any message", bare "all
messages") are unchanged. The model-dependent SQP-2/SQP-3 parts of NVIDIA#669
are not touched.
Signed-off-by: Edson <zhuoxi2000@gmail.com>
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Hi @Zhuoxi2000, thank you for taking the deterministic half of #669, agreeing the split with @nadirali1350, and adding benign and risky pairs for both rules!
Value and readiness: The slash-token change fixes the reported TR2 case (/ask-matt no longer shadows ask), and the new qualifiers fix the pdf-skill TR3 case. But both narrowings also drop detections that main reports today, and #669 asks to keep those ("Continue detecting real command interception, universal triggers"). Two targeted fixes with matching risky tests are needed before final maintainer review. #710 covers the SQP-2/SQP-3 half in different files. The two PRs merge cleanly, and neither one auto-closes #669, so the issue should be closed by hand once both land.
Material findings
-
[Blocker]
src/skillspector/nodes/analyzers/static_patterns_supply_chain.py:953-958(_description_shadowed_commands): the object window for an interception verb ends at the firstand/or/,. It also counts only the next 4 tokens, including articles and adjectives.mainflags each of these real interception claims as TR2, but this head misses them:Intercepts and replaces the built-in deploy command(theandright after the verb leaves an empty span)Overrides the default behavior of the git commit command(commitis the 7th token)Intercepts every invocation of the deploy commandInvoked whenever the user types deploy
The reported FP came only from the slash-token path. The verb path did not cause it. Expected: keep
main's verb behaviour (any built-in after the verb, to the end of the clause). At minimum, do not end the span at a conjunction directly after the verb, and widen the window (e.g. up to the nextcommand(s)noun or about 8 words). Add these inputs totest_description_shadow_command_object_still_tr2. -
[Blocker]
src/skillspector/nodes/analyzers/static_patterns_supply_chain.py:787-788(_DESCRIPTION_UNIVERSAL_SCOPE_RE): the excluded objects areit/this/that/them/anything/everything/whatever. Quantifiers and personal pronouns are not excluded, so the new qualifiers turn these universal triggers (TR3 onmain) into negatives:Use this skill whenever the user discusses anything with you(also... anything with me)Use this skill whenever the user asks anything related to any topicUse whenever the user sends any messages with any content
Expected: add
any|all|every|each|someandyou|me|us|him|herto the excluded objects, and add these inputs totest_description_unqualified_scope_still_tr3.about any topicis already a gap onmain, and the same exclusion closes it. -
[Non-blocking]
src/skillspector/nodes/analyzers/static_patterns_supply_chain.py:816(_DESCRIPTION_SLASH_COMMAND_RE): the lookbehind(?<![\w/])accepts.and~, so relative paths now read as slash commands.Use when the user asks to execute ./build.shreports TR2 forbuildon this head, but not onmain. The same happens with./test.shand~/build. Consider(?<![\w/.~]), or require start of text, whitespace, a quote, a backtick or(before the slash, plus a negative test.
PIC tradeoffs: A regex qualifier cannot tell a real domain from a bait object. Even after finding 2, whenever the user does anything with Claude still reads as qualified. The PIC should either accept that residual gap for the precision gain, or keep these clauses as TR3 at lower confidence.
Verification and gaps: I traced the description path of _analyze_triggers and the new helpers at head 388af29 against main 2226747. I compared behaviour on the inputs above by running the regex literals and helper logic from both versions, copied into a standalone script. No PR module was imported and no tests were run, per policy. _DESCRIPTION_COMMAND_INTERCEPTION_RE had no other callers. _DESCRIPTION_UNIVERSAL_SCOPE_RE also feeds the clause-extraction gate and signal windows. TR1 is unaffected because it needs an activation condition anyway. CI: all 6 checks green. git merge-tree with #710: no conflicts. I did not verify the claim that TR2/TR3 results on the repo's 27 SKILL.md descriptions are unchanged.
Decision: Changes Requested (reviewed head 388af29cda66ed8bcfb0e7396d6a2d6caeddc5ec)
…wing dropped - TR2: an interception verb again governs any built-in after it in the clause, not only the next four words before a conjunction. Tokens inside paths are skipped. - TR3: quantifier and personal-pronoun objects (any, all, every, each, some, you, me, us, him, her) do not bound a universal trigger. - A home-relative path such as ~/build is not a slash command. Signed-off-by: Edson <zhuoxi2000@gmail.com>
|
Thanks @rng1995, this is very helpful. All three points are addressed in 83e784e:
One correction on 3: Checks on 83e784e:
On the PIC tradeoff, I agree a regex can't tell a real domain from a bait object such as |
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Hi @Zhuoxi2000, thank you for the quick turnaround, for pinning every input from the last review as a test, and for the correction on ./build.sh!
Value and readiness: The four interception phrasings and the three universal-scope phrasings from the last review are detected again, and both #669 false positives stay fixed. On real-world descriptions, TR2 precision improves a lot over main (details under Verification). One TR3 problem remains before final review. The object check reads only the first word after the qualifier. As a result, it now reports some domain-bounded descriptions that main accepts, and it misses indefinite-pronoun objects that main reports. #710 merged on 2026-10-04, so this PR is the last open part of #669.
Previous findings:
- Finding 1 (TR2 verb window, blocker): Resolved. At 83e784e, all four inputs report TR2 with the expected command. They are pinned in
test_description_shadow_command_object_still_tr2(tests/unit/test_patterns_new.py:2729). One correction to my last review: I describedmain's verb rule as "any built-in after the verb". In factmaintakes a built-in from anywhere in the clause, so a built-in named before the verb still differs frommain(Finding 2). I am not asking you to restoremain's rule. That rule is also what makesmainflag capability prose such asDiagnose and fix build failures caused by stale overrides. - Finding 2 (TR3 quantifier and pronoun objects, blocker): Resolved for the reported inputs. All four now report TR3, and they are pinned in
test_description_unqualified_scope_still_tr3(:2791). The object check still has gaps in both directions; see Finding 1. - Finding 3 (slash-command lookbehind, non-blocking): Resolved.
(?<![\w/.~])(static_patterns_supply_chain.py:818) stops~/buildand~/deploy/config.yamlfrom reporting TR2, andtest_description_relative_path_not_shadow_command(:2675) pins this. You were right about./build.sh:mainsplits descriptions at., so it reportsbuildthere too. My earlier claim was wrong.
Material findings
-
[Blocker]
src/skillspector/nodes/analyzers/static_patterns_supply_chain.py:788-790(_DESCRIPTION_UNIVERSAL_SCOPE_RE): the excluded-object lookahead checks only the first word afterabout/with/regarding/etc. That causes errors in both directions.-
New false positives.
this,that,any,all,every,eachandsomecount as "no object" even when a domain noun follows. Each of these descriptions reports TR3 at this head, and none does onmain, which accepts anyaboutobject:Use this skill when the user asks any questions about this codebaseUse whenever the user asks anything about that serviceUse this skill whenever the user asks any questions about any AWS serviceUse whenever the user asks anything about each Terraform module
The
this/thatcases date from 388af29, and I missed them last round. The quantifier cases come from the list I suggested. -
New misses. Indefinite pronouns are not excluded. Each of these reports TR3 on
main, and none does at this head:Use this skill whenever the user says anything with anyoneUse this skill whenever the user asks anything related to somethingUse this skill whenever the user asks anything regarding everyone
Expected fix:
- Treat a leading determiner or quantifier as transparent and judge the word after it. The scope stays unbounded only when nothing follows it, or when a pronoun or a generic noun (
topic,subject,thing(s),content) follows. - Add
anyone|anybody|someone|somebody|something|everyone|everybody|nothing|nobodyto the pronouns. - Pin
about this codebase,about any AWS serviceandwith any PDF fileas benign cases. (with any PDF filealso reports TR3 onmaintoday.) - Keep
related to any topicandwith any contentas risky cases, and addwith anyone.
-
-
[Non-blocking]
static_patterns_supply_chain.py:954-957(_description_shadowed_commands): only built-ins after the verb count, so this head now misses passive claims thatmainreports, such asThe built-in deploy command is intercepted by this skillandWhenever the user types deploy it is intercepted by this skill. A narrow passive rule would recover them without bringing backmain's noun false positives: a built-in, thenis/are/gets, then the participle. Separately,overriddenis missing from the verb list on both branches. -
[Non-blocking] PR body: #710 did not close #669, so this PR completes the issue. Consider changing
Refs #669toCloses #669. The body also still describes the four-word window and says the PR does not close the issue.
PIC tradeoffs:
- TR3 cannot tell a domain from a bait object.
whenever the user does anything with the assistant,... with Claudeandany messages with textall read as qualified. These are TR3 onmainand clean here. The author has left this call to the PIC: either accept the gap for the precision gain, or keep qualified clauses as TR3 at lower confidence. - The restored verb window brings back
main's TR2 on activation prose such asInvoke when the user asks to create or update the config. These are the 3 remaining TR2 hits in the corpus below. 388af29 had none, but it also missed the risky inputs.
Verification and gaps:
- Diffs reviewed: the incremental diff 388af29..83e784e and the full diff from merge-base 2226747. Current
main(701baea) has not changed the TR code since then; its changes to this file are in SC2 and SC6. - How I tested: I compared
main, 388af29 and 83e784e on every input above. The regex literals and constant sets were loaded from each version withast.literal_eval, and the helper logic was transcribed by hand. No PR module was imported or run. This reproduction agrees with the PR's own benign and risky tests. - Repo descriptions: TR results on the repo's 27
SKILL.mddescriptions are unchanged (none fire on either branch), as you reported. - Real-world corpus: on 970 real-world
SKILL.mddescriptions, TR2 fires on 18 onmainand on 3 at this head. TR1 results are identical, and TR3 fires on neither. - Commit: 83e784e is by the PR author (
Edson <zhuoxi2000@gmail.com>, the same identity as 388af29) and carries a DCOSigned-off-by. - Merge and CI: the branch merges cleanly into
main. All 6 CI checks are green at 83e784e. - Not done: I did not run the tests locally, per policy.
Decision: Changes Requested (reviewed head 83e784eeab14b958d886f8d5266853b2d03d6009)
…assive TR2 claims
- TR3: a leading determiner or quantifier after the qualifier is transparent, so "about this codebase" and "about any AWS service" are bounded, while a pronoun (including anyone/something/everyone) or a generic noun (topic, subject, thing, content) keeps the trigger universal.
- TR2: a built-in named right before a passive interception verb ("deploy command is intercepted", "deploy it is intercepted") is a shadow command; "overridden" is recognized.
Signed-off-by: Edson <zhuoxi2000@gmail.com>
|
Thanks @rng1995, done in 6e6d54b.
Checks:
The |
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Hi @Zhuoxi2000, thank you for the careful third round, for pinning all eleven new cases, and for updating the PR body so it closes #669!
Value and readiness: The PR fixes both #669 false positives and still catches every risky input from the earlier rounds. The TR3 object check now skips a single leading determiner, as I asked last round. On 970 real-world descriptions, TR2 drops from 18 hits on main to 3, and TR3 fires on neither branch. One small TR3 gap remains. A second determiner (all the, all your) or a following of (any of them) still lands the check on the wrong word. Compared with main, that causes new false positives in one case and misses in the other. The fix is a one-line regex change plus tests. After that, I expect this to be ready for final maintainer review.
Previous findings:
- Round 2, Finding 1 (the TR3 object check reads only the first word; blocker): Partly resolved.
- A leading determiner is now skipped, and the pronoun list includes the indefinite pronouns (
static_patterns_supply_chain.py:785-802). - The four former false positives are now clean, and the three former misses report TR3 again. They are pinned in
test_description_domain_qualified_scope_not_tr3(tests/unit/test_patterns_new.py:2795) andtest_description_unqualified_scope_still_tr3(:2842), together withwith any PDF file. - The check still skips at most one determiner (Finding 1).
- A leading determiner is now skipped, and the pronoun list includes the indefinite pronouns (
- Round 2, Finding 2 (passive TR2 claims; non-blocking): Resolved as suggested.
_DESCRIPTION_PASSIVE_INTERCEPTION_RE(L837-841) catches both examples andThe git commit command is overridden by this skill.overriddenis in both verb lists.Diagnose and fix build failures caused by stale overridesstays a TR2 negative.- A small residual remains (Finding 2).
- Round 2, Finding 3 (PR body): Resolved. The body says
Closes #669, GitHub lists #669 as an issue this PR closes, and the body matches the current rules. - Round 1 findings (TR2 verb window, TR3 quantifier and pronoun objects, slash lookbehind): still resolved. Every input from both earlier rounds is classified as before.
Material findings
-
[Blocker]
src/skillspector/nodes/analyzers/static_patterns_supply_chain.py:800-801:(?:DET\s+)?skips at most one determiner, and the word it then judges can beof. Two common shapes therefore read the wrong word:- A predeterminer followed by a determiner reads as "no object". Each of these reports TR3 at this head, but not on
main, which accepts anyaboutobject:Use this skill whenever the user asks any questions about all the services in the clusterUse whenever the user asks anything about all your Terraform modulesUse whenever the user asks anything about all these files
ofafter a quantifier reads as a domain noun. Each of these reports TR3 onmain, but not at this head:Use this skill whenever the user does anything with any of themUse this skill whenever the user does anything with each of them
Expected fix: skip up to three words that are each a determiner or
of, and never judgeofitself. For example:(?:(?:DET|of)\s+){0,3}, followed by(?!(?:UNBOUNDED|DET|of)\b)[a-z0-9]. In my reproduction, this classifies all five inputs above correctly, as well asabout all of the services. Every pinned benign and risky case stays unchanged. Please add the five inputs as tests, plusabout all of the servicesas a benign case. My last suggestion covered only a single determiner, so this gap is partly mine. - A predeterminer followed by a determiner reads as "no object". Each of these reports TR3 at this head, but not on
-
[Non-blocking]
static_patterns_supply_chain.py:837-841: the passive rule needsis,areorgetsdirectly before the participle. An auxiliary or adverb in between therefore still slips past. Each of these is TR2 onmainand clean here:The built-in deploy command is always intercepted by this skillThe deploy command is being intercepted by this skillThe deploy command will be intercepted by this skillDeploy commands are now shadowed by this skill
The git commit command has been overridden by this skillis missed on both branches. Acceptingis being,will beandhas been/have beenalongsideis/are/gets, plus one optional adverb before the participle, would cover all five.
PIC tradeoffs:
-
Domain versus bait (already noted in the PR body): each of these reads as bounded here and as TR3 on
main:anything with Claudeany messages with textanything with any fileanything related to any matteranything regarding any issueanything with stuffanything with each other
A longer list of generic nouns would narrow this gap but cannot close it. The alternative is still to report qualified clauses as TR3 at lower confidence.
-
Adding
overriddenalso pulls passive configuration prose into TR2.The default branch is overridden by --base(TR2branch) andDefault values can be overridden with the set command(TR2set) are clean onmain. This mirrors howmainalready treats the active voice:--base overrides the default branchis TR2 on both branches. None of the 970 corpus descriptions changes, so I am not asking for a change.
Verification and gaps:
- Commits: one new commit since the last review, 6e6d54b. It is by the PR author (
Edson <zhuoxi2000@gmail.com>, the same identity as the earlier commits) and carries a DCOSigned-off-by. There are no merge commits. - Diffs: I reviewed the incremental diff 83e784e..6e6d54b and the full diff from merge-base 2226747. Since then, current
main(a5ba8b3) has changed this file only in supply-chain (SC) code, not in the TR code. The PR merges intomainwithout conflicts (git merge-tree), and the TR constants, helpers and_analyze_triggersare byte-identical between the PR head and that merge result. - Method: I compared
main, 83e784e and 6e6d54b on every input above. I rebuilt the regex constants from each version's source with a small AST evaluator (literals, concatenation and f-strings over names), and transcribed the clause extraction and the TR2 and TR3 logic by hand. No PR module was imported or run. The reproduction agrees with all 33 description cases in the PR's TR2/TR3 tests. - Corpus: on 970 real-world descriptions, TR2 fires on 18 on
mainand on 3 at both 83e784e and 6e6d54b. TR3 fires on none. On the 33SKILL.mddescriptions in currentmain, TR2 and TR3 fire on none in any version. - TR1: the TR1 code is unchanged. Every clause TR1 can match also carries an activation-condition signal, so the changed regexes do not affect which clauses TR1 sees.
- CI: all 6 checks pass at 6e6d54b.
- Not done: I did not run the tests locally, per policy.
Decision: Changes Requested (reviewed head 6e6d54be54445f889787447053710d4ac7ad4563)
…ve auxiliaries in TR2
- TR3: up to three determiners, quantifiers or "of" after a qualifier are
transparent and "of" is never judged as the object, so "about all the
services" and "about all your Terraform modules" stay bounded while
"with any of them" stays universal.
- TR2: a passive interception claim may use "is/are being", "will be" or
"has/have been" and one adverb before the participle ("is always
intercepted", "are now shadowed", "has been overridden"); a negation
("is not intercepted") still does not count.
Signed-off-by: Edson <zhuoxi2000@gmail.com>
|
Thanks @rng1995, both points are done in 912dadc.
Checks on 912dadc:
On the PIC tradeoffs: I left the generic-noun list and the |
rng1995
left a comment
There was a problem hiding this comment.
Re-reviewed the complete diff, all prior review feedback and replies, stacked determiner/of prefixes, passive auxiliaries, and TR2/TR3 rule boundaries. All review feedback is addressed, including the remaining inline thread; no additional actionable finding remains.
Validation: 601 pattern cases pass on the exact head, plus independent native boundary assertions. With current main and #704 integrated, all 626 current-main pattern cases pass, and the combined relevant suite passes all 697 tests. Approving this reviewed head.
Closes #669 (the deterministic TR2/TR3 part; SQP-2/SQP-3 were fixed in #710)
What / why
TR2 (shadow command). The shadowed command came from any built-in word in a description clause, and any slash token or interception verb in the same clause satisfied the gate. As a result,
Use when the user says /ask-matt or wants an ask answered by Matt.was reported as shadowing the built-inask, even though the only slash token is the unrelated/ask-matt. The shadowed command now has to be the object of the interception evidence:/ask-matt->ask-matt, not a built-in), and a home-relative path (~/build) is not a slash command;invokes/intercepts/overrides/overridden/shadows) governs the built-ins that follow it in the clause (Intercepts and replaces the built-in deploy command,Overrides the default behavior of the git commit command); tokens inside paths are skipped;is/are(optionallybeing),gets,will beorhas/have been, with one optional adverb before the participle (The built-in deploy command is intercepted by this skill,types deploy it is intercepted,is always intercepted,will be intercepted,are now shadowed,has been overridden). The adverb slot takes a short list (always/now/also/still/already) plus-lywords, so a negation (The deploy command is not intercepted by this skill) is not reported;mainreports it.One side effect: a slash token wrapped in backticks (
Use whenever the user runs `/review` on a PR.) is now reported as TR2 forreview. On main it was missed because the whitespace split kept the backtick on the word.This logic lives in a small helper,
_description_shadowed_commands. It replaces_DESCRIPTION_COMMAND_INTERCEPTION_RE, whose only caller was this check.TR3 (keyword baiting).
_DESCRIPTION_UNIVERSAL_SCOPE_REonly treated a followingaboutas a qualifier. That meant the description of Anthropic's pdf skill,Use this skill whenever the user wants to do anything with PDF files., was reported as keyword baiting. Domain qualifiers (with,involving,regarding,concerning,related to/relating to) now bound the scope asaboutalready does. Up to three determiners, quantifiers orofafter the qualifier are transparent, andofis never taken as the object, soabout this codebase,about any AWS service,about all the services,about all of the servicesandwith any PDF fileare bounded by the noun that follows. The scope stays universal when the object is only a pronoun (with it,with you,with anyone,with any of them,related to something,regarding everyone) or a generic noun (related to any topic,with any content). Broad prepositions (in/for/on) are deliberately not qualifiers, sowhenever the user sends any message in the chatstill fires.This follows the expected behaviour stated in #669 ("A noun does not imply a command invocation, 'PDF files' limits the trigger's scope"). It also matches the existing negatives
Answers any questions about PostgreSQLandShow available build commands.Tests
Benign/risky pairs in
TestTriggerAnalysis, including every input from the review rounds:/ask,Overrides the git commit command,Shadows /help,Intercepts and replaces the built-in deploy command,Overrides the default behavior of the git commit command,Intercepts every invocation of the deploy command,Invoked whenever the user types deploy, and the passive forms... deploy command is intercepted ...,... types deploy it is intercepted ...,... git commit command is overridden ...,... is always intercepted ...,... is being intercepted ...,... will be intercepted ...,Deploy commands are now shadowed ...,... git commit command has been overridden .../ask-matt ... an ask,/grill-me ... a question to ask,~/build,~/deploy/config.yaml,Diagnose and fix build failures caused by stale overrides,The deploy command is not intercepted by this skillanything with PDF files,anything related to Kubernetes,any questions regarding Terraform state,about this codebase,about that service,about any AWS service,about each Terraform module,with any PDF file,about all the services in the cluster,about all your Terraform modules,about all these files,about all of the servicesanything with it,any message in the chat,asks about anything,with you,with me,related to any topic,with any content,with anyone,related to something,regarding everyone,with any of them,with each of themExisting positives (
Intercepts the /build command,whenever the user sends any message, bareall messages) are unchanged.Of the 12 cases added in the latest round, 10 fail on the previous head 6e6d54b and pass now. The other two (
about all of the servicesandis not intercepted) already passed and are pinned so the wider rules cannot regress them.ruff checkandruff format --check(v0.15.2, as pinned in.pre-commit-config.yaml) pass on the changed files.mypyreports no issues instatic_patterns_supply_chain.py. TR1/TR2/TR3 results on the repo'sSKILL.mddescriptions are unchanged frommain(27 at the merge base, 33 on currentmain; none fire).Remaining tradeoffs
anything with Claude; this PR treats it as bounded. Keeping qualified clauses as TR3 at lower confidence is the alternative if the PIC prefers recall here.overriddenrecognised, passive configuration prose such asThe default branch is overridden by --basereports TR2, as the active voice (--base overrides the default branch) already does onmain.AI assistance: this change was drafted with an AI coding assistant (Claude) and verified locally with the tests above.