Skip to content

fix(static): stop TR2/TR3 over-reading trigger descriptions (#669) - #709

Merged
rng1995 merged 4 commits into
NVIDIA:mainfrom
Zhuoxi2000:fix-669-trigger-overinterpret
Oct 6, 2026
Merged

rng1995 merged 4 commits into
NVIDIA:mainfrom
Zhuoxi2000:fix-669-trigger-overinterpret

Conversation

@Zhuoxi2000

@Zhuoxi2000 Zhuoxi2000 commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

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-in ask, even though the only slash token is the unrelated /ask-matt. The shadowed command now has to be the object of the interception evidence:

  • a slash-command token is matched by its whole name (/ask-matt -> ask-matt, not a built-in), and a home-relative path (~/build) is not a slash command;
  • an interception verb (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;
  • a passive claim names the built-in right before its auxiliary: is/are (optionally being), gets, will be or has/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 -ly words, so a negation (The deploy command is not intercepted by this skill) is not reported; main reports it.

One side effect: a slash token wrapped in backticks (Use whenever the user runs `/review` on a PR.) is now reported as TR2 for review. 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_RE only treated a following about as 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 as about already does. Up to three determiners, quantifiers or of after the qualifier are transparent, and of is never taken as the object, so about this codebase, about any AWS service, about all the services, about all of the services and with any PDF file are 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, so whenever the user sends any message in the chat still 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 PostgreSQL and Show available build commands.

Tests

Benign/risky pairs in TestTriggerAnalysis, including every input from the review rounds:

  • TR2 risky, still positive (command name asserted): /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 ...
  • TR2 benign, negative: /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 skill
  • TR3 benign, negative: anything 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 services
  • TR3 risky, still positive: anything 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 them

Existing positives (Intercepts the /build command, whenever the user sends any message, bare all messages) are unchanged.

$ pytest -q tests/unit/test_patterns_new.py
601 passed

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 services and is not intercepted) already passed and are pinned so the wider rules cannot regress them.

ruff check and ruff format --check (v0.15.2, as pinned in .pre-commit-config.yaml) pass on the changed files. mypy reports no issues in static_patterns_supply_chain.py. TR1/TR2/TR3 results on the repo's SKILL.md descriptions are unchanged from main (27 at the merge base, 33 on current main; none fire).

Remaining tradeoffs

  • A regex qualifier cannot tell a real domain from a bait object such as 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.
  • With overridden recognised, passive configuration prose such as The default branch is overridden by --base reports TR2, as the active voice (--base overrides the default branch) already does on main.

AI assistance: this change was drafted with an AI coding assistant (Claude) and verified locally with the tests above.

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 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

  1. [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 first and/or/,. It also counts only the next 4 tokens, including articles and adjectives. main flags each of these real interception claims as TR2, but this head misses them:

    • Intercepts and replaces the built-in deploy command (the and right after the verb leaves an empty span)
    • Overrides the default behavior of the git commit command (commit is the 7th token)
    • Intercepts every invocation of the deploy command
    • Invoked 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 next command(s) noun or about 8 words). Add these inputs to test_description_shadow_command_object_still_tr2.

  2. [Blocker] src/skillspector/nodes/analyzers/static_patterns_supply_chain.py:787-788 (_DESCRIPTION_UNIVERSAL_SCOPE_RE): the excluded objects are it/this/that/them/anything/everything/whatever. Quantifiers and personal pronouns are not excluded, so the new qualifiers turn these universal triggers (TR3 on main) 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 topic
    • Use whenever the user sends any messages with any content

    Expected: add any|all|every|each|some and you|me|us|him|her to the excluded objects, and add these inputs to test_description_unqualified_scope_still_tr3. about any topic is already a gap on main, and the same exclusion closes it.

  3. [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.sh reports TR2 for build on this head, but not on main. The same happens with ./test.sh and ~/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)

Comment thread src/skillspector/nodes/analyzers/static_patterns_supply_chain.py Outdated
Comment thread src/skillspector/nodes/analyzers/static_patterns_supply_chain.py Outdated
Comment thread src/skillspector/nodes/analyzers/static_patterns_supply_chain.py Outdated
…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>
@Zhuoxi2000

Copy link
Copy Markdown
Contributor Author

Thanks @rng1995, this is very helpful. All three points are addressed in 83e784e:

  1. TR2 verb objects. I went with your first option and kept main's verb behaviour: an interception verb now covers any built-in after it, up to the end of the clause, with no word window and no stop at a conjunction. To avoid a new false positive on that path, tokens inside a path are skipped, so Invokes ./build.sh to compile gets nothing from the verb branch. All four of your inputs are now in test_description_shadow_command_object_still_tr2.
  2. TR3 objects. The excluded objects now also cover any|all|every|each|some and you|me|us|him|her. Your three inputs and the with me variant are in test_description_unqualified_scope_still_tr3.
  3. Slash lookbehind. It is now (?<![\w/.~]), so ~/build and ~/deploy/config.yaml no longer report TR2. Negative tests are in test_description_relative_path_not_shadow_command.

One correction on 3: ./build.sh and ./test.sh also report TR2 on main (2226747). The description is split into clauses at ., so the clause the rule sees is just /build. This PR doesn't change that, and ~/build was the only case where this head differed from main. Fixing ./ properly means changing how descriptions are split into clauses, so I left it out of this PR. Happy to follow up separately if you want it.

Checks on 83e784e:

  • main, the previous head 388af29 and this head were compared on every input above. This head matches main on all of your risky inputs and still clears the two Trigger and quality-policy checks overinterpret descriptions and tool catalogs #669 false positives.
  • On the repo's 27 SKILL.md descriptions, the TR1/TR2/TR3 results are identical between main and this head (none fire on either).
  • tests/unit/test_patterns_new.py: 577 passed.
  • ruff check, ruff format --check and mypy are clean.

On the PIC tradeoff, I agree a regex can't tell a real domain from a bait object such as anything with Claude. I've left that call to you: either accept the residual gap for the precision gain, or keep qualified clauses as TR3 at lower confidence.

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 described main's verb rule as "any built-in after the verb". In fact main takes a built-in from anywhere in the clause, so a built-in named before the verb still differs from main (Finding 2). I am not asking you to restore main's rule. That rule is also what makes main flag capability prose such as Diagnose 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 ~/build and ~/deploy/config.yaml from reporting TR2, and test_description_relative_path_not_shadow_command (:2675) pins this. You were right about ./build.sh: main splits descriptions at ., so it reports build there too. My earlier claim was wrong.

Material findings

  1. [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 after about/with/regarding/etc. That causes errors in both directions.

    • New false positives. this, that, any, all, every, each and some count as "no object" even when a domain noun follows. Each of these descriptions reports TR3 at this head, and none does on main, which accepts any about object:

      • Use this skill when the user asks any questions about this codebase
      • Use whenever the user asks anything about that service
      • Use this skill whenever the user asks any questions about any AWS service
      • Use whenever the user asks anything about each Terraform module

      The this/that cases 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 anyone
      • Use this skill whenever the user asks anything related to something
      • Use 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|nobody to the pronouns.
    • Pin about this codebase, about any AWS service and with any PDF file as benign cases. (with any PDF file also reports TR3 on main today.)
    • Keep related to any topic and with any content as risky cases, and add with anyone.
  2. [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 that main reports, such as The built-in deploy command is intercepted by this skill and Whenever the user types deploy it is intercepted by this skill. A narrow passive rule would recover them without bringing back main's noun false positives: a built-in, then is/are/gets, then the participle. Separately, overridden is missing from the verb list on both branches.

  3. [Non-blocking] PR body: #710 did not close #669, so this PR completes the issue. Consider changing Refs #669 to Closes #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 Claude and any messages with text all read as qualified. These are TR3 on main and 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 as Invoke 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 with ast.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.md descriptions are unchanged (none fire on either branch), as you reported.
  • Real-world corpus: on 970 real-world SKILL.md descriptions, TR2 fires on 18 on main and 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 DCO Signed-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)

Comment thread src/skillspector/nodes/analyzers/static_patterns_supply_chain.py Outdated
…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>
@Zhuoxi2000

Copy link
Copy Markdown
Contributor Author

Thanks @rng1995, done in 6e6d54b.

  1. TR3 objects (blocker). A leading determiner or quantifier after the qualifier is now transparent, and the check judges the word after it:
    • The scope stays universal only when nothing follows, or when the next word is a pronoun or a generic noun (topic, subject, thing(s), content).
    • The pronoun list now includes anyone/anybody/someone/somebody/something/everyone/everybody/nothing/nobody.
    • Pinned as benign: about this codebase, about that service, about any AWS service, about each Terraform module and with any PDF file.
    • Pinned as risky, next to the existing related to any topic and with any content: with anyone, related to something and regarding everyone.
  2. Passive TR2 (non-blocking). A built-in right before is/are/gets plus intercepted/overridden/shadowed/invoked now counts. An optional command(s) or it may sit in between. Both of your examples and The git commit command is overridden by this skill are pinned. overridden is also in the verb list. Diagnose and fix build failures caused by stale overrides is pinned as a TR2 negative.
  3. PR body. It now says Closes #669 and describes the current rules. The four-word window and the "does not close" note are gone.

Checks:

  • Every input from both rounds was compared on main, 83e784e and 6e6d54b; 6e6d54b classifies all 25 as expected.
  • The 11 cases added this round fail on 83e784e and pass now.
  • tests/unit/test_patterns_new.py: 589 passed.
  • ruff and mypy are clean.
  • TR1/TR2/TR3 on the 27 repo SKILL.md descriptions are still identical to main.

The anything with Claude tradeoff is noted in the PR body as the PIC's call.

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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) and test_description_unqualified_scope_still_tr3 (:2842), together with with any PDF file.
    • The check still skips at most one determiner (Finding 1).
  • Round 2, Finding 2 (passive TR2 claims; non-blocking): Resolved as suggested.
    • _DESCRIPTION_PASSIVE_INTERCEPTION_RE (L837-841) catches both examples and The git commit command is overridden by this skill.
    • overridden is in both verb lists.
    • Diagnose and fix build failures caused by stale overrides stays 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

  1. [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 be of. 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 any about object:
      • Use this skill whenever the user asks any questions about all the services in the cluster
      • Use whenever the user asks anything about all your Terraform modules
      • Use whenever the user asks anything about all these files
    • of after a quantifier reads as a domain noun. Each of these reports TR3 on main, but not at this head:
      • Use this skill whenever the user does anything with any of them
      • Use 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 judge of itself. 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 as about all of the services. Every pinned benign and risky case stays unchanged. Please add the five inputs as tests, plus about all of the services as a benign case. My last suggestion covered only a single determiner, so this gap is partly mine.

  2. [Non-blocking] static_patterns_supply_chain.py:837-841: the passive rule needs is, are or gets directly before the participle. An auxiliary or adverb in between therefore still slips past. Each of these is TR2 on main and clean here:

    • The built-in deploy command is always intercepted by this skill
    • The deploy command is being intercepted by this skill
    • The deploy command will be intercepted by this skill
    • Deploy commands are now shadowed by this skill

    The git commit command has been overridden by this skill is missed on both branches. Accepting is being, will be and has been/have been alongside is/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 Claude
    • any messages with text
    • anything with any file
    • anything related to any matter
    • anything regarding any issue
    • anything with stuff
    • anything 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 overridden also pulls passive configuration prose into TR2. The default branch is overridden by --base (TR2 branch) and Default values can be overridden with the set command (TR2 set) are clean on main. This mirrors how main already treats the active voice: --base overrides the default branch is 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 DCO Signed-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 into main without conflicts (git merge-tree), and the TR constants, helpers and _analyze_triggers are 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 main and on 3 at both 83e784e and 6e6d54b. TR3 fires on none. On the 33 SKILL.md descriptions in current main, 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)

Comment thread src/skillspector/nodes/analyzers/static_patterns_supply_chain.py Outdated
…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>
@Zhuoxi2000

Copy link
Copy Markdown
Contributor Author

Thanks @rng1995, both points are done in 912dadc.

  1. TR3 stacked determiners and of (blocker). I used your regex shape. Up to three determiners, quantifiers or of after the qualifier are now skipped, and of is never judged as the object.
    • Pinned as benign in test_description_domain_qualified_scope_not_tr3: about all the services in the cluster, about all your Terraform modules, about all these files and about all of the services.
    • Pinned as risky in test_description_unqualified_scope_still_tr3: with any of them and with each of them.
  2. Passive auxiliaries (non-blocking). The passive rule now accepts is/are (optionally being), gets, will be and has/have been, plus one optional adverb before the participle.
    • All five of your examples are pinned in test_description_shadow_command_object_still_tr2, each asserting the command name.
    • The adverb slot takes a short list (always/now/also/still/already) plus -ly words, so not and never don't count. The deploy command is not intercepted by this skill is pinned as a TR2 negative. main reports it, and 6e6d54b already did not.

Checks on 912dadc:

  • tests/unit/test_patterns_new.py: 601 passed (589 before, plus 12 new cases). Ten of the 12 fail on 6e6d54b. The other two, about all of the services and is not intercepted, already passed and are pinned to guard the wider rules.
  • Every pinned case from the earlier rounds is unchanged, and so are the PIC tradeoff examples (with Claude, with text, --base, the set command).
  • make test-unit: 8527 passed, 14 skipped, 4 xfailed.
  • ruff check and ruff format --check are clean with v0.15.2 (the pre-commit pin) on the changed files, and make lint/make format-check are clean with v0.15.19. mypy is clean.
  • TR1/TR2/TR3 on the repo's SKILL.md descriptions match main: 27 at the merge base and 33 on current main, with none firing.
  • The branch still merges cleanly into current main, and test_patterns_new.py passes on the merge result (626 passed).

On the PIC tradeoffs: I left the generic-noun list and the overridden passive behaviour as they are. Both are noted in the PR body, which also now describes the three-word skip and the new auxiliaries.

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@rng1995
rng1995 merged commit 17e9e85 into NVIDIA:main Oct 6, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Trigger and quality-policy checks overinterpret descriptions and tool catalogs

2 participants