fix(analyzer): stabilize TM1 window identity

[chrisknvidia]

TM1 reconciliation can drop lexical findings or collapse distinct long calls when displayed evidence collides. Preserve the lexical signal during companion abstention, use complete source identities for retained-owner matching, and retain source order and canonical metadata under bounded windows and output caps.

Review fixes keep uninvoked global/nonlocal stores, annotation-only declarations, unrelated attribute stores and same-slot assignments from suppressing lexical HIGH. Explicit called-slot replacement has a bounded lifetime: cached-module changes are tracked across ordinary imports, while unknown eager effects discard replacement certainty. Native callable captures, cross-API assignments and native values wrapped in RHS expressions cannot establish replacement proof. The legacy first-statement direct shadow control remains bounded to a single completed store with no native receiver/Popen reference and no later eager effect. Receiver binding, report ownership and cached slot state remain separate.

This branch includes the still-open #577 changes. #576 is merged.

Validation of this revision: 663 affected analyzer/graph/input cases passed; 51 fresh installed-wheel CLI scans passed across positive/negative source cases, real reports and strict exits. Ruff lint/format for src/tests and diff checks passed. Static verification uses real native analyzer/graph execution and an installed wheel with --no-llm; remote provider inference, production deployment and other operating systems were not exercised. Earlier broad-suite results were from older heads and do not establish a green full suite for this update. Hosted CI and human re-review remain required.

[rng1995]

Reviewed exact draft head d856d88c289c5ef3389060ef69651c9981aae38f. I found no additional blocker in the focused TM1 identity range (f2ae98f..d856d88): 220 focused tests and 392 current-main merged-tree regressions pass, and lint/format/diff checks are clean.

I am requesting changes because the current combined tree still contains both confirmed dependency blockers from #576 and #577. On this exact head, a three-row ledger cap drops the second distinct fatal fact, and shell=enabled still evades TM1 when a later argument expression is effectful. The PR is also still draft and its own review contract requires #576/#577 to land, a rebase onto current main, fresh CI, and current-head review.

Please propagate the dependency fixes and rebase. If the focused range remains semantically unchanged and CI stays green, I found it otherwise suitable for approval.

[chrisknvidia]

@rng1995 The final #577 receiver-invalidation fix is included, the merge conflicts are resolved, and the cap/source-order findings remain fixed. PTAL.

[rng1995]

The focused #578 cap/source-order work looks correct and its prior #576/#577 reproductions now pass, but this stacked head contains the exact #577 receiver-invalidation blocker: an effectful argument after shell= may replace the trusted subprocess receiver, yet later proxy calls are still classified as subprocess and receive false-positive TM1 findings. Please update the stack after #577 clears trusted_names on this effectful path and add the regression. The current head is otherwise conflict-free and its affected/integration suites are clean.

[rng1995]

[SkillSpector Review]

Changes requested on exact head 1c03702961323bd8d322c89928480197282d1d0b for a confirmed remaining issue inherited from #577.

The earlier effectful-argument fix is present: direct subprocess calls now invalidate receiver trust after unsafe arguments, including expression, assignment, and annotated-assignment paths. However, an ordinary effectful function call can still replace the receiver without invalidating that trust; a later call through the replacement object is then reported as HIGH TM1. The inline comment provides the source-traced fixture and expected correction. Please propagate the #577 correction through this stack and cover this case.

Scope: I inspected the prior review history, current dependency fix, receiver-trust collector and both scan passes, its caller integration, tests for the direct-argument correction, and current checks. This is a focused blocking review, not approval or certification of the entire large combined stack. The remaining combined diff still needs complete current-head assessment after the dependency issue is addressed. Six hosted checks pass, but passing checks do not establish this untested receiver semantic.

No contributor-provided code or tests were executed locally.

[rng1995]

[SkillSpector Review]

Re-reviewed the previous receiver-invalidation finding on exact head 193bb90a3eed190562a9e97103b0aba5d29773b3, including its current implementation, forward/prepass effects, caller/ownership integration, all prior reviews/replies, and relevant regression tests.

The exact standalone-call examples and the earlier effectful-argument ordering fixes are present. However, the required receiver-safety correction is still incomplete: a generic effectful call nested in a tuple/list RHS leaves the receiver trusted, and a subsequent fresh true binding makes a proxy call a false-positive HIGH TM1. This is the same remaining defect in the current #577 implementation. The inline continuation describes the source-traced case and required regression coverage.

All six hosted checks pass, but their generic-call tests cover only outer ast.Call values. Please fix this in the shared implementation and propagate the correction through the stack.

Review scope remains a focused blocking re-review of the dependency and its integration, not full clearance of the nine-file, 6,064-added-line combined TM1/window stack. Complete current-head stack assessment remains required before approval after the blocker is fixed. No contributor code/tests were executed locally; no merge was performed.

[rng1995]

[SkillSpector Review]

Hi @chrisknvidia, thank you for the persistence on the receiver-trust work and for adding the nested-container regressions!

Value and readiness: The nested tuple/list receiver finding is fixed. However, 68e22d0 makes the shared receiver-trust model conservative enough that TM1 shell=<name> findings main reports today are now silenced in ordinary script layouts. That reopens the issue #475 evasion class, so this needs a fix before the stack is ready. The TM1 window/identity work that earlier reviews assessed is unaffected by this update apart from merge resolutions, which look correct. #577 is still open with changes requested.

Previous findings:

• Nested tuple/list/annotated RHS receiver invalidation (last review, inline thread): Resolved. The first part landed in 7fee7af and 68e22d0 extended it. _expression_preserves_receiver_trust (static_python_shell_truthiness.py:780-835) now rejects any eager expression that is not passive or a passive direct subprocess call. It recurses into tuples, lists and comprehensions, and both the forward scan and the _advance_trusted_names prepass use it. Covered by test_nested_generic_call_invalidates_receiver_trust[tuple-rhs|list-rhs|annotated-tuple-rhs], test_nested_generic_call_invalidates_receiver_trust_for_called_function and test_unsupported_eager_statement_invalidates_receiver_*. Real positives remain in test_direct_subprocess_call_remains_detected.
• Earlier findings (inherited #576/#577 ledger-cap and later-effectful-argument blockers, effectful arguments after shell=, standalone generic calls): Resolved in earlier rounds and still in place at this head.
• Rebase and fresh CI: Resolved for main (main merged at 8831219, 6/6 checks green). #577 remains open.

On your inline reply: I confirmed the fix. Note that 7fee7af is a "Merge remote-tracking branch" commit that also carries the fix itself: +34/-11 lines in the truthiness module and +37 test lines, plus static_runner.py / test_shared_python_ast.py conflict resolutions. The resolutions adopt main's coordinate_view and include_source_offsets correctly. 912263f is a clean main merge. Please land fixes as their own commits so reviewers can isolate them.

Material findings

1. [Blocker] src/skillspector/nodes/analyzers/static_python_shell_truthiness.py:1135, :1565 with static_patterns_tool_misuse.py:5480: TM1 detections that main reports are now lost (introduced by 68e22d0). _reconcile_variable_shell_findings drops every same-scope x = True ... shell=x lexical finding (same_scope or ownership False) and leaves it to the companion. After 68e22d0 the companion declines in common code:

   • _advance_trusted_names now clears trust for every unsupported statement (line 1135; before, it removed only explicit rebinds). A trailing if __name__ == "__main__": therefore records an invalidation after every earlier def, and line 1519 scans those function bodies with no trust.
   • Once any generic call sets the new sticky unknown_unsafe_bindings, every later import clears trust, including import subprocess (line 1565).

   Source-traced failing inputs. Main reports TM1 HIGH for both via _VARIABLE_SHELL_FLAG_RE, and 7fee7af also detected both through the companion. This head reports nothing:

   import subprocess
   
   def run(cmd):
       use_shell = True
       subprocess.run(cmd, shell=use_shell)
   
   if __name__ == "__main__":
       run("ls")

   import os
   payload = input()
   import subprocess
   enabled = True
   subprocess.run(output, shell=enabled)

   The second input is the original test_preparsed_python_is_reused_by_all_ast_analyzers fixture. 68e22d0 reorders it to put import subprocess first instead of keeping the detection. The same reconciliation also drops main's finding for return subprocess.run(cmd, shell=use_shell) because Return is never inspected. That part predates this update.
   Expected fix: when the companion cannot prove receiver identity or truthiness, keep main's lexical TM1 (reduced confidence is fine). Suppress it only when the receiver is explicitly rebound (store, import alias, attribute mutation) or the companion emits its own finding for that call. Fix this in the shared #577 module and propagate it through this PR and #579. Add node()-level regressions for both inputs above and restore the original fixture order.

2. [Non-blocking] Test parity across the stack: #579 carries about 600 more lines of tests for the byte-identical truthiness module, for example test_unknown_unsafe_binding_blocks_receiver_reestablishment and the class-body release cases. This PR ships the same code without them. Please sync the tests into the layer that introduces the code.

3. [Non-blocking] The PR body is stale. It lists #576 as unmerged, cites #577 at c3b0ff9, and its validation counts predate 68e22d0. Please refresh it.

PIC tradeoffs: The receiver-trust model now treats any statement that could in theory rebind subprocess as if it did. That includes a later call, a compound statement, a class with any base, and a __del__ finalizer. This removes contrived proxy false positives but costs detections in ordinary scripts. The PIC should confirm that an uncertain receiver keeps the lexical TM1 finding rather than dropping it.

Verification and gaps: I read all four prior rng1995 reviews, both inline threads and the replies. I compared the PR diff against its merge base at 193bb90 and at 68e22d0 per file. I inspected git show --remerge-diff for 7fee7af (conflicts in static_runner.py and test_shared_python_ast.py, plus author edits) and 912263f (clean), and read all of 68e22d0. I traced the forward scan, both prepasses, _reconcile_variable_shell_findings and main's _VARIABLE_SHELL_FLAG_RE / _variable_shell_flag_same_scope path for the inputs above, and compared _advance_trusted_names at 193bb90 and 68e22d0. I did not re-assess the window/identity range beyond the merge resolutions. All six checks pass. Per policy, tests and contributor code were not executed locally.

---

Decision: Changes Requested (reviewed head 68e22d068437bd9b3e933fdd7f6a7e9047275a47)

[rng1995]

Re-reviewed exact head 481a8fbe57774fbb9e8311436d36e9ceed0ab511 against all earlier reviews and the full current diff. The reported original lexical-abstention cases now retain HIGH TM1, and focused analyzer/runner/end-to-end tests pass. Independent base-to-head reproductions confirm three P1 groups: untaken expression stores, identity-preserving receiver/flag stores, and an earlier compound-statement invocation hidden by a future module store. Each still executes the native subprocess call with shell=True but loses its HIGH finding on this head. Details and minimal reproductions are inline. The green hosted checks do not cover these regressions. Changes requested; do not merge until they are fixed. The three earlier threads whose exact examples are now fixed are resolved; #577 remains an open dependency.

[rng1995]

[SkillSpector Review]

Hi @chrisknvidia, thank you for keeping the window-identity work in step with #577 through every round!

Value and readiness: Ready once CI passes on the new head. #577 is now on main, and this branch carries the TM1 window-identity changes on top of it. The three P1 findings from the last round are fixed. While stacking this branch on the merged #577, I found and fixed two gaps where this branch reported differently from #577.

Previous findings (review at 481a8fb)

• [P1] Untaken expression stores: Resolved in 15b4e58, the same change as #577.
• [P1] Identity-preserving receiver and flag stores: Resolved in 15b4e58.
• [P1] Future outer store hiding an earlier invocation: Resolved in 15b4e58.
• Earlier rounds (main guard, input-before-import, lexical abstention): still resolved, and node-level regressions cover them.

What I changed on the branch (signed off)

• 9162964 merges main (additive conflicts in imports and static_runner.py).

• 15b4e58 applies the lexical-counterevidence fix byte-identical to #577's.

• 548b17c adapts main's #784 marker-ownership test, the same as #577.

• 7b643ff Duplicate TM1 findings. This branch emitted two findings for one call where #577 emits one. Both cases are on main too:

  • true = True + shell=true in unparseable Python, or after an unknown effect, reported both the variable window and the direct owner;
  • the raw and normalized (ff) views of a true_value window kept different identities.

  Now there is one owner per call. It also ports five #577-only tests.

• 327591b Detection lost against #577. This branch's \b-bounded direct pattern no longer owns shell=true_value. So true_value = True + shell=true_value in .js or Markdown got 0 findings, while #577 reports 1 and main 2. A second call after an unknown effect, and two calls on one unparseable line, were also missed. True-prefixed names now get a window per call, and outside Python that window is kept.

• 4031711 merges current main (with #577). In the four files #577 also changed, this branch's superset version is kept; no other main commit touched them. The shared-AST fixture keeps the input()-before-import subprocess order you restored.

Differential check: I ran 2,400 generated inputs (names, call forms, ff prefixes, layouts, malformed Python, .py, .js and SKILL.md). Every call #577 reports is reported here, with no new duplicates. The only extra findings in #577 are its double reports of subprocess.Popen calls (at column 0 and at the Popen token).

Non-blocking notes

• Unlike #577, which matches on the name prefix alone, this branch uses the AST, so shell=true_value with no True binding is no longer reported (a parameter, = False, or a shadowed name). That is the intended precision.
• Already on main and on #577: duplicate windows for non-true names with the ff prefix or Cyrillic look-alikes.

Verification:

• Full local non-integration suite on 4031711: 11,310 passed, 0 failed.
• The 20-layout TM1 probe matches #577.
• ruff is clean.

---

Decision: Ready to merge once CI passes. A human maintainer needs to approve (reviewed head 4031711b9622e9ece1cb2a04c5a985fb43e0f7ba). This bot pushed commits to the branch, so it does not approve the PR itself.