Repository navigation
fix(tm1): use the final repeated assignment target - #820
Conversation
Signed-off-by: Christopher Kevin <christopherk@nvidia.com>
Signed-off-by: Christopher Kevin <christopherk@nvidia.com>
c274605 to
0ecb722
Compare
|
Ready for review at Five hosted checks have passed. Full unit CI is still running on the latest main-sync commit. |
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
@chrisknvidia thank you for tracking down this TM1 false positive and for keeping the fix so tightly scoped. The new test file is solid: 23 of its 43 cases fail on main, so it pins exactly the behavior this PR changes.
Value and readiness
When one statement assigns the same name more than once, Python keeps the last value. TM1 did not. For enabled, enabled = True, False followed by subprocess.run(cmd, shell=enabled), main reports TM1 HIGH even though shell is False. The PR restores Python's left-to-right target order (static_patterns_tool_misuse.py:3932). It then drops the earlier restore event only when three things hold: the final source is provably falsy, the RHS is passive, and the prior bindings are proven safe. The proof is _passive_assignment_name_snapshots at line 3817, and the check is at line 3964.
The fix works. List targets, nested unpacking, chained targets, None/0/''/() finals, NFKC-normalized names and CRLF sources all give the result Python actually produces. Some shapes the pre-pass cannot prove, and those keep main's behavior: starred, attribute or subscript targets, an unknown RHS, class scope, loops, if/try, and global/nonlocal. A call that uses the flag before the repeated assignment is still flagged. That includes a call through a function defined and called before the assignment.
Ready to merge once CI runs on the current head.
Material findings
None.
Tradeoffs
- At module scope, the fix applies only when nothing comes before the assignment except
import subprocessorfrom subprocess import Popen. An earlierimport os,import sysorfrom __future__ import annotationskeeps the false positive. Inside a function, the fix applies whatever the module imports. The description already calls this out and the tests pin it, so I see it as a reasonable first step. - Some skills that main flags only because of this false positive will now scan clean. Two shapes stand out: the tuple reset followed by a truthy rebinding inside a
for/if/with/matchblock, and a second shell flag assigned inside the same TM1 window. Main already misses both when the reset is written as plainenabled = False, so the PR adds no new evasion. The root causes arerecord_bindingskipping stores in compound statements and_variable_shell_matchesnot overlapping windows. That is worth an optional follow-up issue, not a blocker here. - Building the shell AST index over 746 stdlib and repo files takes about 1.4-1.7x longer. End-to-end TM1 time is unchanged within noise, both on real files and on large flat or deeply nested inputs.
Verification and gaps
- PR head
e9b4e2af, which already includes current main:- The new test file: 43 passed.
- The focused TM1, shared-AST and shell-ownership suites: 541 passed.
- The broader end-to-end suites: 1,130 passed.
- The new test file copied onto main: 23 failed, 20 passed.
- Over 100 hand-written fixtures through the TM1 node on main and on the PR. Every difference is either a correct suppression or the same result main already gives for the equivalent plain
enabled = Falsestatement. - A differential fuzz of random nested and chained assignments, checked against the runtime value from
exec(), over three seeds. Leftover false positives dropped from 17/29/23 to 3/7/3, and the false-negative sets were identical on both trees. - CLI check with
skillspector scan <skill> --no-llm --format json. The repeated-target skill gets TM1 HIGH on main and no TM1 on the PR, and completeness stayscomplete. - CI:
- The run on
e9b4e2afis waiting for maintainer approval. - The last completed run on the feature commits (
e811918f) passed. - The one failed run (
dabde149) failed onlytest_real_binary_structure_is_not_instruction_noise, on a timing assertion. That test also fails on main right now.
- The run on
- Not verified: LLM-enabled scans and macOS runners.
A repeated boolean target such as `enabled, enabled = True, False` leaves `enabled` false, but TM1 kept the earlier true store and reported HIGH on a later `shell=enabled` call. Follow Python's left-to-right target order and drop earlier truthy stores only when the final value is provably false, the whole RHS is passive and prior target bindings are proven safe in a straight-line module or function scope; anything else keeps the conservative event policy. Applied unchanged: it builds only on the #790 variable-shell index and static_python_shell_truthiness helpers, which match main on this branch. No prerequisites added; the new test module is ported as is. (cherry picked from commit 62cab16) Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…1 notes List the five analyzer, report and CLI fixes backported onto the v2.13.0 base (#820, #823, #836, #837, #840) in the CHANGELOG and release notes, and raise the intro count to eight backported fixes. Document the compatibility notes: scan --mcp-registry now exits 2 with --use-shipped-baseline or any --transitive* option, including an explicit --transitive-depth 1; recursive multi-skill scans exit 2 with --use-shipped-baseline; a complete recursive MEDIUM aggregate reports CAUTION instead of SAFE in JSON and SARIF with exit codes unchanged; and the TM1 and MP3 detection changes can change findings and scores. main has no CHANGELOG entries for these PRs, so the text is written from each backport. Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A repeated boolean target such as
enabled, enabled = True, Falseleavesenabledfalse, but TM1 retains the earlier true value and reports HIGH on a subsequentshell=enabledcall.Use Python's left-to-right assignment order to identify the final value. Suppress earlier affirmative stores only when the final value is provably false, the entire RHS is passive, and prior target bindings are proven safe in a supported straight-line module or function scope. Unknown values/effects, identity assignments, class scopes and unsupported contexts preserve the conservative event policy. Imports add safety only for their own freshly proven native binding.
Regression coverage includes tuple/list targets, nested unpacking, chained assignments, false/true finals, identity/unknown finals, and prior-binding/import controls. This is a narrow false-positive fix; uncertain preceding operations can retain existing false positives.
Validation of the unchanged detector/test bytes:
53c40020passed all six hosted CI checks, including 13,001 unit tests. Earlier macOS startup timeouts remain separate from the successful Linux evidence.Automatic main updates changed CLI preflight and report-provider metadata while leaving this PR's complete feature diff unchanged. Current integration head
f8ac51bbrestarts CI without changing source; earlier green checks do not clear this new head. Current CI remains pending. All scan fixtures are synthetic and use--no-llm; model-enabled and hosted-consumer execution remain unverified. Native Bugbot and Security Review passes are unavailable and have not been claimed; the PR remains draft pending the outstanding review requirements. No merge or auto-merge is enabled.