You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(taint): track sensitive data through reflective urllib sinks - #589
Resolve bounded, statically-computable module and attribute strings used by importlib.import_module(...) and getattr(...).
Map reflective handles onto existing sinks at the exact call site, allowing the existing taint engine to report credential/file-read flows through reflective urllib calls.
Model lexical scope, reassignment, closures, global/nonlocal effects, class namespaces, branch joins and expression evaluation order. The remaining control-flow gaps below are not yet fixed.
Bound the reflective prepass per file to depth 16, 1,024 body analyses and 20,000 work units, charging frame bookkeeping before copying. On a prepass limit or RecursionError, retain already resolved sinks, continue ordinary taint analysis and mark the affected artifact partial. Later files remain independently analyzable. The shared workflow deadline still propagates normally.
Remaining review work and scope
This PR is not ready for overall approval yet. The latest resource-bound batch addresses the call-expansion blocker only. Still outstanding: deferred async/generator binding effects; terminated/short-circuited code effects; zero-iteration loop bindings; unequal-known-sink branch joins; keyword-value local bindings. Non-blocking default/decorator binding and broader reflective-sink regression notes are also outstanding.
Stopping the prepass may miss later reflective calls, so coverage is explicitly partial. Constants are conservative ceilings, not a promise that all ordinary programs fit within them. Dynamic URLs, raw sockets/chunk streams and arbitrary runtime-only attribute names remain outside scope. The directly nested urlopen(os.environ["API_KEY"]) source-collector gap is unchanged; ordinary fallback tests use the baseline-supported assignment-then-call form. Inline/stored build_opener detection is separately tracked in #721.
Latest local validation
Before the resource fix: fan-out exceeded a fixed 50,000-check guard and a deep call chain raised RecursionError (two failing cases, one passing control).
423 behavioral/ledger tests passed, including depth/body/work caps, retained prefix findings, ordinary TT3 and later-file detection, RecursionError fallback and shared-deadline propagation.
Parsed-source stress checks: 25-level fan-out used 291 checks; 200-function chain used 928; 10,000 trivial definitions used 10,001. Fixture code was not executed.
Full non-integration/non-provider suite: 8,827 passed, 14 skipped, 139 deselected, 4 xfailed in 374.51 seconds; four existing warnings.
Repository-wide Ruff lint/format, targeted mypy and whitespace checks passed.
No live or paid provider calls were made. The local Anaconda readline workaround was bypassed in the pytest launcher only, without editing project or environment files. AI assistance was used to investigate, implement and verify this change.
The resolver treats every unqualified call spelled getattr as the builtin, but it never checks the active scope for a parameter or local binding with that name. For example, def send(getattr): ... opener = getattr(module, "urlopen") ... opener(secret) is recorded as a urllib sink even though the call is user-defined; require the builtin/unshadowed getattr before creating the alias.
Alias invalidation misses non-assignment rebinding constructs
Alias invalidation is implemented only for Assign and AnnAssign. Rebinding through constructs such as for opener in ..., with ... as opener, an assignment expression, an exception target, or del opener is not processed, so the previous reflective sink identity can survive to a later opener(secret) after the runtime binding has changed or been removed. Handle these binding/deletion nodes (or conservatively clear the alias) in source order.
Lambda parameters do not shadow reflective aliases
Only named function definitions establish a new resolver scope; lambdas are traversed generically. A lambda parameter can therefore fail to shadow an outer reflective handle, e.g. lambda opener: opener(secret) is resolved against the module's urlopen alias and can produce a false TT3. Add lambda argument/body scope handling consistent with the named-function path.
Nested methods are omitted from reflective sink analysis
This handler never traverses node.body, so every class method is omitted from call_sinks, including a method that creates and invokes its own reflective urlopen handle. A valid class Client: def send(...): module = ...; opener = getattr(...); opener(secret) therefore remains unreported; the added regression only proves that a class-body handle does not leak into a method. Traverse nested function/async-function definitions with fresh function scopes while still skipping class assignments.
The reason will be displayed to describe this comment to others. Learn more.
Requesting changes for six additional issues reproduced on 1d8a648. The latest commit fixes importlib parameter shadowing and function-local imports. I also reproduced the class-method, rebinding/lambda, and shadowed-getattr issues already described in the existing review; those are referenced rather than duplicated inline.
Compared d162d9b to 1d8a648 using fresh wheels and isolated offline source/wheel runs. The selected regression tests pass (87 base, 98 head), while 15 of 33 focused behavior cases fail identically in both head modes. All eight concurrency checks and seven string-boundary checks pass. Nine CLI fixtures and 12 complete skill directories ran in all four lanes, with source/wheel parity. Nine corpus reports per lane remain partial because of missing references or obfuscated text; this does not establish global rule accuracy or live-provider coverage.
This review covers the reflective urllib scope of this PR, not the remaining dynamic-URL/socket/chunk-stream requirements of #586.
Pushed a797b06 to address the requested reflective-scope follow-ups.
In addition to the six inline cases, this update covers the four previously referenced findings:
unqualified getattr must be the unshadowed builtin;
reflective handles are invalidated by for/async for, with as, assignment expressions, exception targets, del, and augmented assignment;
lambda arguments establish their own shadowing scope;
class methods are analyzed with fresh function scope without closing over the class namespace.
The resolver now uses iterative expression traversal, deferred function-body analysis, isolated comprehension/lambda scopes, and branch-state joins. I added 19 regression tests for the reported cases and adjacent positive/negative controls.
Pushed 1c3bce4 to address the latest review findings. The reflective resolver now preserves callee selection before argument evaluation while analyzing the function body against post-argument bindings; uses the function’s lexical environment with live globals; checks the shared workflow runtime budget during reflective traversal/expansion; and records method-name bindings in class-body order. Added positive/negative regressions for each case, including a bounded function-fan-out fixture. Local validation: 256 related tests passed; Ruff lint/format and mypy passed. The commit includes the required DCO sign-off. Hosted checks for this head are running. Could you please re-review when convenient?
The cloned global scope is discarded when this call returns, so bindings made through global declarations do not reach later call sites. For example, configure() can assign global opener = getattr(module, "urlopen"), but a later opener(secret) is analyzed without that binding even though the runtime call uses the reflective sink. Propagate global/nonlocal effects from analyzed calls or model these side effects explicitly.
Analyze only methods surviving final class namespace
Methods are invoked unconditionally after the class scope is discarded, even when a later class-body assignment removed or rebound their name. For example, class C: def send(...): opener(secret); send = lambda ... leaves the lambda as C.send, but this post-pass still analyzes the dead method and can record a reflective sink. Invoke only methods that remain bound in the final class namespace.
The reason will be displayed to describe this comment to others. Learn more.
[SkillSpector Review]
Thanks for the fixes @agentsope.I re-reviewed exact head 1c3bce47729114e466fed24535e67336676dc765, including the complete three-file diff, the resolver and taint/ledger integration, all previous review bodies/threads and author replies, and the added analyzer/report tests. The latest fixes address the reported argument-before-function-body ordering, caller-local leakage into a separately defined function, shared runtime deadline checks, and method-name rebinding in a class body. The earlier import/shadowing, relative imports, annotation-only binding, deep-expression traversal, assignment/target invalidation, lambda/comprehension scope, ordinary branch joins, inline reflective calls, function registry rebinding, and tuple-order cases remain covered.
Not all review comments are addressed. These three source-traced findings already appear in the latest substantive Copilot review; I am linking them rather than duplicating inline threads.
[P1] Nested functions still capture stale local bindings (behavioral_taint_tracking.py:630-633,646). The open closure finding remains unchanged. _register_function clones the defining non-global scopes at definition time, and invocation clones that snapshot again. When outer() first defines send() using opener, then binds opener = getattr(module, "urlopen"), then calls send(), the nested function still sees the pre-binding shadowed entry and loses the reflective sink. The inverse rebind can preserve a stale sink. Keep the defining lexical frame live or model closure-cell updates, and add both late-binding and clearing regressions.
[P1] Called functions discard global/nonlocal effects (behavioral_taint_tracking.py:642-650). _binding_scope correctly directs global assignments into the active global frame, but _invoke_function makes that frame a clone and discards it on return. For opener = lambda value: value; def configure(): global opener; opener = getattr(module, "urlopen"); configure(); opener(os.environ.get("API_KEY")) (with ordinary multiline function syntax), the later call is analyzed against the original harmless binding although runtime invokes the network sink. The inverse assignment leaves false sink identity. Propagate actual call effects into the caller's global/nonlocal frames while keeping speculative uncalled-function analysis isolated; add positive and negative global/nonlocal tests.
[P2] Rebound class methods are still analyzed as surviving methods (behavioral_taint_tracking.py:787-804). The post-pass iterates the original methods list after discarding the class scope and checks only called_functions, not the final namespace. class C: def send(self): opener(os.environ.get("API_KEY")); send = lambda self: None (formatted as a normal class) still records the dead method's reflective sink. Restrict this fallback to methods that remain reachable in the final class namespace, accounting for preserved aliases, and add a negative replacement control plus a surviving-method positive.
All six exact-head hosted checks pass. The added tests exercise the four latest requested fixes but do not cover these remaining closure/effect/final-namespace cases. This re-review used static source and test inspection only; contributor code/tests or scanned network examples were not executed locally. Changes are still required before approval.
Thanks for the detailed re-review. I've addressed the three remaining closure/effect/final-class-namespace findings and merged current main without rewriting the previously reviewed commits.
Nested functions now capture live defining lexical frames, rather than definition-time copies. Branch and match joins restore/merge bindings in those frames without replacing the cells that closures reference. Positive late binding and negative clearing regressions cover straight-line, if, and exhaustive match paths.
Actual calls share their global and captured nonlocal frames, so assignments and clearing survive return. Uncalled-function/method/lambda scans instead use an isolated environment that clones each shared frame once and remaps closure references; speculative call tracking is also restored afterward. Tests cover positive/negative global/nonlocal effects and ensure speculative bodies do not alter later calls. Global lookups also bypass a same-named enclosing local when the callee declares global.
Class-method fallback now uses the completed class namespace, including surviving aliases and definitions inside class-body branches, rather than the original methods list. Replaced/deleted methods are skipped. Class frames are excluded from method/lambda lexical closures, and a prior speculative method scan does not prevent re-analysis when its defining function runs again.
Validation:
The initial reproduction/control selection had 16 failures and 5 passes on the previous implementation. Additional self-review controls reproduced three scope/caching failures and four indirect class-definition/closure failures before their corresponding fixes.
All 292 tests in the behavioral-taint and report test files pass on the final implementation, including all 36 newly added parameterized cases and the existing shared-runtime deadline regression.
An independent check of 36 synthetic straight-line/branch/match, closure/global/nonlocal and function-alias cases agrees with a pure in-memory runtime oracle; no network or other external I/O was performed.
Ruff lint/format checks, targeted mypy and git diff --check pass.
The existing static-only graph regression test_constructed_network_getattr_tracks_sensitive_data_to_report also passes end-to-end; scanned example text is not executed and LLM use is disabled.
Full offline unit selection on the final source: 8,739 passed, 14 skipped, 135 deselected and 4 expected failures. Live provider/LLM integration tests were excluded. The local Anaconda readline issue was bypassed only in the pytest launcher, without changing project files/environment.
This remains scoped to statically resolvable reflective sinks. It does not claim coverage for arbitrary dynamic URLs, raw sockets or the remaining stream cases in #586. Please re-check these three findings on the updated head once hosted CI completes.
Thanks for the follow-up review. This signed follow-up addresses the three latest alias/import/comprehension findings without rewriting the reviewed history.
Name assignments now copy the current module/callable identity before invalidating targets, so aliases retain their own identity after the original name is rebound, but rebinding the alias clears it. Annotated and chained assignments are covered.
Dotted getattr bases resolve through the active import bindings, including qualified/static imports and module aliases. Shadowed roots, relative imports and a shadowed getattr remain negative controls.
Comprehension walrus targets bind in the containing non-comprehension scope and honor global/nonlocal declarations, as specified by PEP 572. Local-binding collection includes these targets without treating iteration targets or lambda-body locals as containing-function locals. Lambda defaults are evaluated in the containing scope.
Empty literal iterables and constant-false filters do not execute the remaining body. Unknown iterables/filters retain both possible binding states; generator bodies are scanned in isolation rather than applying deferred effects to live frames. Nested comprehensions, positive/clearing updates, lambda boundaries and RHS shadowing have controls. This is not a general interpreter or a claim to model generator consumption.
Validation:
Initial reproduction/control matrix on the previous implementation: 45 failed, 15 passed.
Added 81 analyzer regression cases and four offline graph/report cases.
Final taint/report selection: 373 passed; graph/report selection: 5 passed (four new and one existing). The shared-runtime-budget regression also passes separately.
An independent set of 72 synthetic resolver cases agrees with an import-free, pure in-memory Python execution oracle; no network samples are executed.
Repository-wide Ruff lint/format, targeted mypy, and git diff --check pass.
Final full offline unit selection: 8,820 passed, 14 skipped, 139 deselected and 4 expected failures. Live provider/LLM tests were excluded. The local Anaconda readline workaround was confined to the pytest launcher, with capture enabled and no project/environment change.
Current upstream main is already included. The scope remains statically resolvable reflective sinks; this does not close the remaining dynamic-URL/socket/stream cases in #586. Please re-check the three latest findings on this head once hosted CI completes.
This collector is used to determine names that are local for the entire function, but a nested function's decorators and default/keyword-default expressions execute in the containing scope. Skipping them means a walrus there is not collected: in send, an earlier opener(secret) can incorrectly resolve the module-level reflective opener even though def helper(x=(opener := lambda value: value)) makes opener local to send. Traverse those expression parts while continuing to skip the nested function body.
Lambda defaults are skipped during local binding collection
The binding prepass returns without visiting a lambda's defaults, but defaults are evaluated in the enclosing function and a walrus there makes its target a local for the whole function. Consequently a call before callback = lambda value=(opener := ...) can still resolve opener against the outer reflective handle and record a false TT3 instead of respecting Python's local-scope rule. Visit the lambda defaults (while still skipping its body) when collecting local bindings.
The reason will be displayed to describe this comment to others. Learn more.
[SkillSpector Review]
Hi @agentsope, thank you for working through every closure, global/nonlocal and class-namespace case from the last round, and for the paired positive and negative regressions for each one!
Value and readiness: This PR closes a real TT3 evasion from #586: credentials sent through a urlopen handle built with getattr and importlib.import_module. The three findings from my last review are fixed and tested. So are Copilot's three alias, import and comprehension notes, and all six of yashrajp22's threads. It is still not ready to merge. The reflective prepass inlines every call to a known function with no bound. A small crafted .py file can therefore either crash the taint analyzer, which drops every TT finding in the skill, or keep it running until the shared 600 s deadline. main analyzes the same files in linear time. My last review wrongly counted the new deadline checks as fixing this, and I'm sorry I didn't catch it then.
Previous findings:
2026-09-23 review: the scope and control-flow items (starred targets, relative-import shadowing, inline reflective callees, class-body calls, match captures, call-site globals) and expression evaluation order: Resolved. These were confirmed in later rounds and still hold.
_visit_expression (L823-865) visits sibling expressions in source order, and a walrus binds after its value.
Covered by test_walrus_respects_expression_order, test_starred_assignment_replaces_reflective_handle, test_relative_import_replaces_reflective_helper, test_inline_reflective_callee, test_class_body_reflective_call_is_detected, test_match_capture_replaces_reflective_handle and test_function_reads_global_at_call_site.
2026-09-26 threads:
Evaluate arguments before the called body: Resolved. finish_call_callee (L832-839) records the callee, queues the invoke, then visits the arguments first. Covered by test_function_call_uses_callee_before_arguments_and_globals_after_arguments.
Analyze called functions in their lexical environment: Resolved. _invoke_function uses [caller_scopes[0], *closure] (L752). Covered by test_function_call_uses_lexical_scope_not_caller_locals.
Bound repeated function expansion and honor the runtime budget: Partly resolved, still open.
The prepass now checks the shared deadline (L483-489, plus checks inside its loops).
Nothing bounds or memoizes expansion. _invoke_function (L740-757) re-analyzes the callee at every call, and active_functions only stops recursion.
The PR's own test_reflective_prepass_checks_shared_runtime_budget_during_expansion (test L551-575) has to abort the 616-byte fixture after 500 checks, which shows the expansion is still unbounded. See finding 1.
Record method definitions in the class namespace: Resolved. Method names are bound in class-body order (L946-952). Covered by test_class_method_definition_replaces_reflective_class_binding.
2026-09-28 review:
Nested functions captured stale local bindings: Resolved in 8aed90bb.
_register_function (L691-704) keeps the live defining frames, and calls run against them.
test_nested_function_uses_live_closure_binding covers late binding and clearing, in straight-line code, if and match.
Called functions discarded global/nonlocal effects: Resolved.
Actual calls share the global frame and the closure frames (L750-752). Uncalled bodies run on clones through _run_isolated (L718-735).
I traced the global and nonlocalconfigure() examples: the binding survives the return.
Covered by test_called_function_retains_external_binding_effects, test_uncalled_function_analysis_isolates_global_effects and test_speculative_bodies_do_not_apply_external_effects.
Rebound class methods were still analyzed: Resolved.
The fallback iterates the final class_scope.functions (L953-959), so replaced or deleted methods are skipped.
Covered by test_class_fallback_only_analyzes_surviving_methods and test_conditional_methods_wait_for_final_class_namespace.
Copilot's 2026-10-04 notes (name aliases, dotted getattr bases, comprehension walrus scope): Resolved in 51787aca, in _bind (L596-625) and _module_name (L537-545).
yashrajp22's six threads from 2026-09-19 (deep expressions, relative imports, bare annotations, definition-time globals, branch joins, global/nonlocal and comprehension locals): Resolved. Evidence is in the replies drafted on each thread.
Material findings
[Blocker]src/skillspector/nodes/analyzers/behavioral_taint_tracking.py:740-757 (_invoke_function), called through _build_reflective_sink_aliases at L1322: the prepass inlines function bodies with no depth or work bound. Each case below is a new failure mode, because main handles these files in linear time.
Exponential fan-out. Take def f0(): pass, then each def f{i}(): f{i-1}(); f{i-1}().
That needs 2^(n+1)-1 body analyses. For the PR's own 20-level, 616-byte fixture that is 2,097,151, and at 25 levels (771 bytes) it is about 67 million.
My 2026-09-26 timing (about 20 µs per body analysis) puts 25 levels at about 20 minutes, so the prepass runs until the shared 600 s deadline.
node() then records RUNTIME_LIMIT (L1555-1570) and marks every later .py file partial without analyzing it. The scan takes the full deadline, and other nodes still running see the same exhausted deadline.
Deep call chain. Take def f0(): pass, then each def f{i}(): f{i-1}().
Each inlined level adds about 11 Python frames: _visit_statements, visit, NodeVisitor.visit, generic_visit, NodeVisitor.generic_visit, visit, NodeVisitor.visit, visit_Call, _visit_expression, _invoke_function and _analyze_function.
In my model of this call structure, RecursionError starts at 90 functions (1.9 KB) from an almost empty stack. A real scan starts deeper, so it needs fewer.
No module-level call is needed, because the uncalled-function pass (L1123-1128) walks the same chain.
node() catches only _BehavioralResourceLimitError (L1555), so the RecursionError reaches guard_analyzer_node (src/skillspector/inspection_ledger.py:1086-1140). That marks the analyzer FAILED and returns findings: []. Every TT finding in the skill is dropped, including a real TT3 in another file.
Quadratic bookkeeping, even with no calls.
Each uncalled function, lambda and generator expression goes through _run_isolated, which rebuilds function_closures for every registered function (L725-728).
_visit_statements also scans scope.functions.values() once per deferred function (L1124-1126).
My micro-model of just these loops takes about 14 s for 10,000 trivial defs (about 210 KB).
Expected fix: bound the prepass and make it fail soft.
Cap the inline depth and the total number of inlined body analyses per file, treating further calls as opaque, or memoize a body while its visible bindings are unchanged.
Catch RecursionError, and any cap you add, around _build_reflective_sink_aliases. Keep the sinks found so far, and let the normal sink pass run. Ideally, record a partial note so coverage is not overclaimed.
Replace the fan-out test with one asserting that the 20-level fixture finishes within a fixed check count without raising.
Add a node()-level test with a ~200-function chain plus an ordinary urllib.request.urlopen(os.environ["API_KEY"]), asserting that TT3 is still reported.
[Non-blocking]behavioral_taint_tracking.py:390-400,446-447: Copilot's latest two notes are valid. _LocalBindingCollector does not visit nested-def decorators and defaults, or lambda defaults, so a walrus there is not counted as a function local. An earlier opener(secret) in that function then resolves to the outer reflective handle. The only effect is a TT3 on code that would raise UnboundLocalError before the call, so this is a cleanup, not a blocker.
[Non-blocking]behavioral_taint_tracking.py:643: _reflective_callable accepts any _ALL_SINKS member, not only urllib sinks.
For example, getattr(os, "system")(input()) now resolves to os.system and reports TT5. Deserialization and file-write sinks behave the same way.
This is a welcome extension, but it is untested and not mentioned in the PR body.
Please add one exec-sink positive case and a shadowed-os negative case so the broader behavior is intentional and protected.
PIC tradeoffs:
Complexity. About 860 new lines in the taint analyzer model Python scoping and execution order: closures, global/nonlocal, class namespaces, comprehensions, match, branch joins and call-site inlining.
Every review round so far has found new scope edge cases, and finding 1 shows the attack surface this adds to the analyzer itself.
The rest of the taint analyzer is name-keyed with no scope model, and #611 makes it fully flow-insensitive.
A flow-insensitive variant would be linear and much smaller: treat any name ever bound to a resolvable reflective sink as a possible sink. The cost is extra TT3 findings when a handle is later rebound to something harmless.
Recall gap by design. Global effects of uncalled functions are discarded.
test_uncalled_function_analysis_isolates_global_effects[False] (test L278) asserts there is no TT3 in this case: opener starts as a harmless lambda, def configure(): global opener; opener = getattr(module, "urlopen") rebinds it, def send(): opener(os.environ.get("API_KEY")) uses it, and neither function is called in the module.
Skills are often driven by an agent or framework that calls module functions directly. Splitting the binding and the use across two entry points is therefore an easy way around the new detection. The flow-insensitive variant would catch it.
The PIC should decide whether this precision is worth the maintenance cost and attack surface, or whether a simpler may-analysis fits the analyzer better.
Verification and gaps:
New since my last review:
8aed90bb is a merge of main at 4a550627 that also carries the author's closure, effect and class fixes. Because of that, I reviewed the PR's own diff against the new merge-base (4a550627..51787aca, the same three files, no unrelated changes) rather than the merge commit alone.
51787aca adds the alias, dotted-base and comprehension changes.
The author's two replies (2026-10-04 and 2026-10-05) match the code.
I read the full resolver and its use in _analyze_python, and traced each earlier example by hand at this head.
Models (stdlib only; no contributor code imported or run):
A model of the resolver's call and frame structure, for the fan-out counts and the recursion threshold.
A micro-model of the _run_isolated and _visit_statements bookkeeping.
Inlining counts for 13 CPython stdlib modules stayed modest, with a maximum of about 21,000 body analyses for email/_header_value_parser.py. Ordinary code is not the concern; crafted or generated call graphs are.
Merges:
Clean with current main, including #623. The merged module passes ruff check --select F.
With #611, the module auto-merges into a coherent result (the reflective prepass plus _collect_tainted), and only the test imports conflict (keep textwrap, pytest and time).
Overlap with #721: they complement each other, and neither supersedes the other.
This PR maps handles built with getattr/import_module onto existing sinks (part of #586). It does not recognize build_opener(...).open(...).
#721 adds build_opener(...).open(...) as a sink (part of #714) and does not resolve reflection.
Whichever lands second should check inline_openers first, then call _resolve_sink_name(..., reflective_sinks), and keep arguments_only for inline openers.
Suggested order: #611 (approved pending a rebase), then #721 (smaller and linear; changes were requested in its own review on 2026-10-05), then this PR once finding 1 is fixed.
The body says this "addresses part of #586" and has no closing keyword, so #586 stays open as intended.
yashrajp22's CHANGES_REQUESTED review is still recorded. They will need to re-review, or a maintainer will need to dismiss it, before merge.
CI: all 6 checks are green at 51787aca. I did not run tests locally, per policy.
Decision: Changes Requested (reviewed head 51787aca02b83aa108235c209263d40f02b5f81d)
The reason will be displayed to describe this comment to others. Learn more.
[Blocker] Inlining here has no depth or work bound. active_functions only stops recursion, so every call re-analyzes the callee.
Fan-out: with def f{i}(): f{i-1}(); f{i-1}(), the prepass does 2^(n+1)-1 body analyses. That is 2,097,151 for the 616-byte fixture in this PR's own test, and about 67 million at 25 levels (771 bytes). The prepass then runs to the shared 600 s deadline, and node() marks every later .py file partial.
Depth: each inlined level adds about 11 Python frames. A chain def f{i}(): f{i-1}() of about 90 functions (under 2 KB) raises RecursionError, even with no module-level call. node() only catches _BehavioralResourceLimitError, so guard_analyzer_node marks the analyzer failed and drops every TT finding in the skill.
Please cap the inline depth and the total inlined analyses per file, or memoize. Also catch RecursionError around _build_reflective_sink_aliases, so the normal sink pass still runs. For tests: the 20-level fixture should finish within a fixed check count without raising, and a ~200-function chain should still report an ordinary TT3 through node().
The reason will be displayed to describe this comment to others. Learn more.
Rechecked the author fixes at 51787ac. All 33 retained cases from the earlier review now pass, as do the selected repository suites (263 baseline / 456 candidate tests). Five distinct binding/control-flow issues remain below. I also reproduced the existing call-expansion finding, so I have not duplicated it.
Verification used fresh base/head wheels and source runs on Linux arm64/Python 3.12, with baseline a50b9c9. The 54 focused cases pass 54/54 on the baseline and 42/54 on the candidate in each mode; two of the candidate failures are separately characterized builtin-getattr alias coverage gaps outside the current syntactic subset. Source/wheel results agree across 68 focused CLI and 48 sample-directory reports. Nine of the 12 sample directories remain partial because of reference/parse/opaque-content limitations; this is not a global accuracy or live-provider validation. Integration with newer main is not covered by this pinned comparison.
The reason will be displayed to describe this comment to others. Learn more.
Please keep deferred function bodies from changing live bindings when they are only instantiated. With a reflective opener = getattr(module, "urlopen"), the following call still targets the original network handle:
The resolver applies clear immediately and loses TT3. An unconsumed generator with the same assignment also reproduces it. Both source and installed-wheel CLI runs return complete/SAFE reports without TT3; the synchronous-call control behaves correctly. Please analyze deferred bodies in isolated state until their execution is established.
This loop still visits the assignment after return, so both source and installed-wheel CLI runs miss TT3 and report complete/SAFE. The expression visitor also loses TT3 for False and clear() when clear is an ordinary synchronous function that reassigns opener: it applies effects from the skipped operand. Please retain the reachable binding state when scanning terminated blocks or short-circuited expressions.
The reason will be displayed to describe this comment to others. Learn more.
Please preserve the zero-iteration path before changing the loop target or body state. After opener = getattr(module, "urlopen"), for opener in []: pass cannot replace the handle, but this code clears it and the following opener(os.environ.get("API_KEY")) loses TT3. Rebinding inside an empty loop, and inside while False, also erase the live sink. The empty-target case returns complete/SAFE in both source and installed-wheel CLI runs; the nonempty harmless-target control correctly has no TT3. Skip effects for known-empty loops and retain possible pre-loop bindings when iteration is uncertain.
The reason will be displayed to describe this comment to others. Learn more.
Please preserve feasible sink identities when the two branches resolve to different known sinks. For example, assigning getattr(urllib.request, "urlopen") on one branch and getattr(requests, "post") on the other reaches this else and drops opener entirely. The later opener(os.environ.get("API_KEY")) then produces no TT3 in either source or installed-wheel CLI and is reported complete/SAFE, even though both choices are network sinks. The same-sink control and the earlier sink-versus-harmless regression pass; this is the remaining unequal-known-values case in the new join.
The reason will be displayed to describe this comment to others. Learn more.
Please traverse keyword argument values when collecting function locals. In a function with opener(secret) followed by use(value=(opener := (lambda value: value))), the assignment makes opener local throughout that function, so the earlier load cannot use a global reflective handle. Because ast.keyword is not an ast.expr, this filter skips the binding and resolves the earlier call through the global sink. Both source and installed-wheel CLI emit a false TT3 and return DO_NOT_INSTALL. The positional-walrus negative control and the keyword-assignment-before-use positive control both pass.
Resource-bound follow-up for the call-expansion review finding
Head: f881b4b; parent before this batch: 51787ac. All three follow-up commits carry DCO sign-offs. This is an incremental update, not a claim that all review findings are addressed.
Addresses the call-expansion blocker only:
Bound reflective inlining to depth 16, 1,024 body analyses and 20,000 work units per file. Charge captured-frame bookkeeping and cloning before doing the expensive work.
Replace repeated deferred-function membership scans with one surviving-function set.
Catch reflective work limits and RecursionError inside the reflective prepass, retaining already resolved call sites and continuing ordinary taint analysis. Do not catch the shared workflow deadline.
Mark the affected artifact partial/static_parse_limit, retaining finding IDs. Later files remain independently analyzable.
Tests:
Before the fix: fan-out exceeded a fixed 50,000-check guard and a deep call chain raised RecursionError (two failing cases, one passing bookkeeping control).
423 behavioral/ledger tests pass, including separate depth/body/work cap controls, retained reflective-prefix findings, ordinary TT3 and later-file detection, synthetic RecursionError fallback and shared-deadline propagation.
Independent parsed-source stress checks: fanout-25 took 291 checks; chain-200 took 928 checks; 10,000 trivial definitions took 10,001 checks. No fixture code executed.
Whole-repository Ruff lint/format, targeted mypy and whitespace checks pass.
Full non-integration/non-provider suite: 8,827 passed, 14 skipped, 139 deselected, 4 xfailed and 4 existing warnings in 374.51 seconds. Pytest's local Anaconda readline workaround was bypassed in the launcher only; no project or environment files were edited for that workaround.
The ordinary TT3 control uses secret = os.environ["API_KEY"]; urllib.request.urlopen(secret). The directly nested urlopen(os.environ["API_KEY"]) form has an existing source-collector gap, already discussed in #714/#721; this batch does not silently expand source detection.
Tradeoff: stopping the optional prepass can miss later reflective calls, so coverage is explicitly partial. Constants are conservative implementation ceilings, not a claim that all ordinary programs fit within them.
Still outstanding in #589: deferred async/generator binding effects; terminated/short-circuited code effects; zero-iteration loop bindings; unequal-known-sink branch joins; keyword-value local bindings. Non-blocking default/decorator binding and broader reflective-sink regression notes are also separate.
Every direct function call invokes the entire body immediately, but async functions and generators defer their body effects until awaited or iterated. For example, configure() where configure is an async def or generator with global opener; opener = getattr(...) must not change opener before opener(secret); this path currently mutates the live reflective scope and can emit a false TT3. Defer these bodies or analyze them without committing effects until the deferred operation occurs.
Bind positional and keyword arguments to function parameters
Arguments are traversed, but no positional or keyword argument is bound to the callee's local frame before _invoke_function. Consequently def send(opener): opener(secret) called as send(opener=getattr(module, "urlopen")) is analyzed with opener shadowed and misses the reflective sink. Bind the call arguments, including keyword values, to the corresponding parameters before analyzing the body.
Model short-circuit paths before applying assignment effects
Expression traversal visits every child unconditionally, so it applies assignments inside short-circuited operands. For example, opener = getattr(module, "urlopen"); False and (opener := print); opener(secret) is reported as safe even though the assignment never executes and the final call still uses the reflective sink. Model the feasible evaluation paths for and/or (and other short-circuit expressions) before applying binding effects.
Preserve possible sink bindings when branch identities differ
When both branches resolve a name to different known sink identities, this merge removes the binding entirely. A post-join call can still invoke a network sink on either path, so clearing the name causes a false negative for cases such as one branch resolving urllib.request.urlopen and the other requests.post; retain a possible sink (for example, an explicit set of candidates) instead of dropping both values.
Preserve pre-loop bindings for zero-iteration paths
The loop target is unconditionally shadowed and the body effects are applied after the loop, with no zero-iteration join. Thus opener = getattr(module, "urlopen"); for opener in (): pass; opener(secret) loses the valid pre-loop sink, and an unknown iterable can similarly suppress a sink that remains possible when the body does not run. Preserve the pre-loop state as a feasible post-loop state.
Stop analysis after unreachable terminating statements
This statement pass continues through code after unconditional return, raise, break, or continue, allowing unreachable assignments to overwrite live bindings. For example, a called function containing global opener; return; opener = getattr(module, "urlopen") changes the post-call scope here even though the assignment cannot execute, producing a false TT3 on a later opener(secret). Stop each path at terminating statements and join only reachable states.
🧠 Review effort: Lite
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
The reason will be displayed to describe this comment to others. Learn more.
Rechecked f881b4b. The five behavior findings in my previous review remain reproducible. The resource fix passes all 20 targeted checks in both source and wheel modes, but the local-binding collector still bypasses the optional work counter: the separate collector follow-up passes 2/4 checks per mode. The existing resource finding remains partially addressed. Two additional sink-tracking issues are documented below without duplicating existing threads.
Verification used fresh BASE/HEAD wheels and isolated offline source runs on Linux arm64/Python 3.12, with BASE 3c8e4b9. Selected repository tests pass: 274 baseline and 463 candidate. Focused checks pass 65/65 baseline and 46/65 candidate per mode; failed scope characterizations remain recorded. Source/wheel behavior agrees, with timing-only differences in resource ledgers. All 80 focused CLI and 48 sample-directory reports completed execution; 2 CLI and 36 sample reports remain partial. This does not establish global rule accuracy, live-provider coverage or integration with newer main.
The reason will be displayed to describe this comment to others. Learn more.
Please retain every sink classification observed for a call site across repeated invocations. This assignment replaces the earlier classification when the same function body is analyzed again:
Both source and installed-wheel CLI runs report TT1 but omit TT3 for the first invocation. Reversing the two bindings loses TT1 in the focused checks; repeating the same network sink passes. Please preserve both observed sink identities so a later invocation cannot erase an earlier finding.
Returning all frames here lets the class attribute hide the real network sink, so both source and installed-wheel CLI runs miss TT3. Swapping the global and class bindings instead produces a false TT3. The outermost iterable must still use the class scope; that control and the comprehension-target shadowing control already pass.
The reason will be displayed to describe this comment to others. Learn more.
[SkillSpector Review]
@agentsope thank you for the focused resource-bound follow-up: separate depth, body and work caps, a fail-soft fallback that keeps ordinary TT findings, and an honest partial ledger entry, each with its own test.
Value and readiness
The PR still targets a real TT3 evasion from #586, and the call-expansion blocker from my last review is fixed. It is not ready yet, for three reasons:
The new work cap is low enough that ordinary Python files with no reflection, from about 1,000 lines up, are now reported as partial.
yashrajp22's seven threads are still open, as the PR body acknowledges.
The branch now conflicts with main.
Material findings
Prior findings
Unbounded inlining: fan-out, RecursionError and quadratic bookkeeping (Blocker, 2026-10-05; also the 2026-09-26 thread): Resolved.
Caps: behavioral_taint_tracking.py:207-209, :502-505 and :779-783.
Limits and RecursionError are caught at :1185, and the file is marked partial/static_parse_limit with its finding IDs kept (:1636-1645).
The shared deadline still propagates.
Test L551-670 covers each cap, prefix retention, later files and the fallback.
How the cap is calibrated is a new problem; see finding 1.
The collector skips nested-def decorators and defaults, and lambda defaults (Non-blocking): Still open (:400-410, :458-459).
_reflective_callable accepts every _ALL_SINKS member, and there are no exec or deserialization tests (Non-blocking): Still open (:663).
yashrajp22's threads: Still open. They are:
deferred async/generator bodies (:794);
code after return, and short-circuited operands (:1160);
class scope leaking into comprehension bodies (:525, new).
New
Blocker: behavioral_taint_tracking.py:209. The 20,000-unit work cap is spent on linear traversal, not only on expansion.
_check_runtime charges one unit per visited statement or expression node.
_snapshot_frames (:726-751) charges len(function_closures), which counts every function registered so far, plus the frame sizes.
A snapshot is taken for every uncalled-function analysis (one per def), three times per if, and once per comprehension, lambda and genexp. A file with F functions therefore pays about F² before any inlining.
I wrote my own stdlib-only lower-bound model of these charges; it doesn't import or run PR code. On ordinary modules it exceeds the cap:
File
Lines
Estimated units
shutil.py
1,566
~35k
configparser.py
1,333
~25k
tempfile.py
956
~20k
subprocess.py, zipfile, argparse
61k-82k
Your own figure fits this: "10,000 definitions → 10,001 checks" is the cap tripping at the surviving-set charge (:1163-1165) before any body is analyzed. test_many_uncalled_functions_have_bounded_bookkeeping passes the same way.
On such files, node() records partial, the analyzer becomes degraded and scan completeness becomes partial. --fail-on-incomplete and --min-coverage then fail, even though the file has no reflection and main reports it complete.
Suggested fixes:
Skip the prepass, and its partial event, when a file has no getattr or import_module call.
Scale the budget with file size (for example k × AST nodes), or charge only repeated work.
Avoid copying every closure for each snapshot.
Add a node() test showing that a ~1,500-line, ~150-function module without reflection stays completed.
Blocker: the branch conflicts with main (CONFLICTING).
In _resolve_sink_name, #721's build_opener(...).open check collides with the reflective lookup. Keep both; they match disjoint shapes.
In the test imports, keep both time and textwrap.
No CI has run on f881b4b.
PIC tradeoffs
Approach. Most of the open threads come from modeling kills, where a handle is rebound to something harmless in code that may not run.
A flow-insensitive may-analysis would treat any name ever bound to a resolvable reflective sink as a possible sink.
It would be linear, would need no caps, and would close those false negatives by construction.
The cost is extra TT3 findings after a benign rebinding.
After several rounds of new edge cases, the PIC should decide this before the next round.
Coordination.#799 rewrites _collect_tainted, _find_tainted_names_in_args and _analyze_python, and it conflicts with this PR in both files. The merge order needs a decision.
Verification and gaps
I read the incremental diff 51787ac..f881b4b (3 commits) and the resolver, node() and ledger paths at head.
I traced every _charge_work call site by hand.
The model behind finding 1 counts only the charges I could confirm and ignores inlining, so the real totals are higher.
yashrajp22 also notes that _LocalBindingCollector bypasses the work counter. I agree it checks only the deadline (:372-375). Its cost tracks the body that is charged right after it, so I fold it into finding 1.
The reason will be displayed to describe this comment to others. Learn more.
Blocker: this budget is spent on linear work. _check_runtime charges one unit per visited node. Every snapshot charges len(function_closures) plus the frame sizes, and snapshots happen once per uncalled def, three times per if, and for each comprehension and lambda. My stdlib model of just those charges goes past 20,000 on ordinary modules of about 1,000 lines or more (shutil.py about 35k, configparser.py about 25k). Those files are then marked partial/static_parse_limit even when they contain no reflection, so --fail-on-incomplete fails where main reports complete. Please skip the prepass when a file has no getattr or import_module call, and scale the budget with file size (or charge only repeated work). Please also add a node() test showing that an ordinary large module stays completed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
yashrajp22
left a comment
rng1995
left a comment
Addresses part of #586; leaves the issue open.
Summary
importlib.import_module(...)andgetattr(...).Remaining review work and scope
This PR is not ready for overall approval yet. The latest resource-bound batch addresses the call-expansion blocker only. Still outstanding: deferred async/generator binding effects; terminated/short-circuited code effects; zero-iteration loop bindings; unequal-known-sink branch joins; keyword-value local bindings. Non-blocking default/decorator binding and broader reflective-sink regression notes are also outstanding.
Stopping the prepass may miss later reflective calls, so coverage is explicitly partial. Constants are conservative ceilings, not a promise that all ordinary programs fit within them. Dynamic URLs, raw sockets/chunk streams and arbitrary runtime-only attribute names remain outside scope. The directly nested
urlopen(os.environ["API_KEY"])source-collector gap is unchanged; ordinary fallback tests use the baseline-supported assignment-then-call form. Inline/storedbuild_openerdetection is separately tracked in #721.Latest local validation
No live or paid provider calls were made. The local Anaconda readline workaround was bypassed in the pytest launcher only, without editing project or environment files. AI assistance was used to investigate, implement and verify this change.