Skip to content

fix(scanner): distinguish Python inference methods from dynamic eval - #2805

Open
reshuibuduo wants to merge 2 commits into
hashgraph-online:mainfrom
reshuibuduo:codex/python-eval-inference-false-positive
Open

fix(scanner): distinguish Python inference methods from dynamic eval#2805
reshuibuduo wants to merge 2 commits into
hashgraph-online:mainfrom
reshuibuduo:codex/python-eval-inference-false-positive

Conversation

@reshuibuduo

@reshuibuduo reshuibuduo commented Sep 5, 2026

Copy link
Copy Markdown

Summary

Fix DANGEROUS_DYNAMIC_EXECUTION false positives for Python no-argument inference methods without changing JavaScript/TypeScript detection or the rule's high severity.

Reproduction

The existing \beval\s*\( search reports all of these as dynamic code execution:

model.eval()
self.cross_model.eval()
self.fusion.eval()

PyTorch documents Module.eval() as switching a module into evaluation mode. It takes no code argument. Python's builtin eval(source, ...) has different semantics.

This was reproduced with published plugin-scanner==3.0.94 and upstream main cf617e7b7a4c2d26344dc083e1745f802f4b3be4. The first regression run against unchanged main produced 20 failures / 70 passes, covering both the reported false positives and previously missed builtin aliases/parenthesized calls.

Changes and security boundary

  • Parse Python with the standard-library AST; source is never imported or executed.
  • Keep bare builtin references and builtins.eval / __builtins__.eval conservative, including imported aliases and references used by partial application.
  • Keep unknown .eval(...) receivers flagged whenever positional, keyword, *args, or **kwargs arguments are supplied.
  • No-argument methods alone do not establish dynamic execution. Nested builtin calls, including method bodies and f-string expressions, are still inspected.
  • Handle UTF-8 BOM input; syntax/value/recursion errors fall back to the existing text detector.
  • Leave JavaScript/TypeScript, new Function, severity, scoring, exclusions, repository-policy trust, dependencies and Cisco integration unchanged.

This is a bounded heuristic, not Python type inference or a guarantee that every arbitrary method implementation is safe. Comments and ordinary Python string literals no longer masquerade as calls.

Validation

  • Python 3.12: 186 passed across code-quality, scanner, false-positive regressions, Action reporting/gating, path boundaries, security and policy tests.
  • Python 3.10.21: 93 passed in tests/test_code_quality.py.
  • Production lint/format plus the changed test file: passed.
  • Focused basedpyright: 0 errors.
  • Structural code-quality debt ratchet: passed.
  • git diff --check: passed.
  • Read-only scan of the unchanged TMCRA bundled runtime: 175 code files; the original regex flags 8 Python files, while the patched code-quality check reports zero findings. No runtime file was changed or excluded.
  • Additional local whole-repository scan through the Action's policy pipeline: repository policy untrusted, Cisco static skill scanning enabled, minimum score 80 and high-severity gate retained. Score 89; zero dynamic-execution findings; 30 remaining HARDCODED_SECRET findings across 15 synthetic test fixtures reported under two ecosystems. The command correctly exits 1. This is a local experiment using the patched source, not an official Action run.

Commands:

python -m pytest tests/test_code_quality.py tests/test_scanner.py tests/test_scanner_false_positive_regressions.py tests/test_action_runner.py tests/test_action_runner_step_summary_findings.py tests/test_path_support.py tests/test_security.py tests/test_policy.py --tb=short -q
python -m ruff check src tests/test_code_quality.py
python -m ruff format --check src tests/test_code_quality.py
python -m basedpyright src/codex_plugin_scanner/checks/code_quality.py --level error
python scripts/ci/code_quality_audit.py --root . --baseline ci/code-quality-baseline.json --json-output <temporary-report>

Full-suite attempt on Windows stops during collection because tests/guard_daemon_acceptance_fixtures.py imports the Unix-only resource module. Full ruff check src tests reports 10 existing issues in unchanged tests; full formatting check reports 42 unchanged files. These unrelated files were left alone. The Linux full matrix remains for upstream CI. The current upstream Actions runs are action_required and await maintainer approval.

Downstream context: TMCRA Codex release PR. Its release remains a draft pending the official scanner gate; separate synthetic test-credential findings are outside this fix. This PR does not claim that the complete marketplace scan is passing.

Summary by CodeRabbit

  • Bug Fixes

    • Improved Python eval detection to distinguish dynamic code execution from safe no-argument methods such as Module.eval().
    • Recognizes aliased imports, built-in references, and .eval(...) calls with arguments.
    • Ignores comments and string literals while inspecting expressions inside f-strings.
    • Retains conservative text-based detection when Python parsing fails.
    • JavaScript and TypeScript detection behavior remains unchanged.
  • Documentation

    • Clarified Python eval detection behavior and its heuristic limitations in the README.

@greptile-apps

greptile-apps Bot commented Sep 5, 2026

Copy link
Copy Markdown

PR author is not in the allowed authors list.

@qodo-code-review

Copy link
Copy Markdown

ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing

@qodo-free-for-open-source-projects

Copy link
Copy Markdown

PR Summary by Qodo

Distinguish Python inference .eval() from dynamic execution

🐞 Bug fix 🧪 Tests 📝 Documentation 🕐 20-40 Minutes

Grey Divider

AI Description

• Parse Python syntax to distinguish builtin dynamic evaluation from no-argument inference methods.
• Preserve conservative detection for aliases, argument-bearing methods, parse failures, and
 non-Python files.
• Document and test the security boundary across Python and JavaScript/TypeScript.
Diagram

graph TD
    A["Code file"] --> B{"Python file?"} -->|Yes| C["AST parser"] --> D{"Parse succeeds?"} -->|Yes| E["Syntax eval check"] --> G["Check result"]
    D -->|No| F["Text eval check"] --> G
    B -->|No| F
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Refine the regular expression
  • ➕ Smaller implementation change
  • ➕ Avoids parsing overhead and parser-version compatibility concerns
  • ➖ Cannot reliably distinguish comments, strings, builtin references, and method calls
  • ➖ Alias and parenthesized-call handling would remain incomplete
  • ➖ Complex patterns would be difficult to audit as a security boundary
2. Add Python type inference
  • ➕ Could identify known inference APIs more precisely
  • ➕ Could distinguish receiver implementations beyond call shape
  • ➖ Requires substantially more complexity or new dependencies
  • ➖ Third-party and dynamically assigned types remain uncertain
  • ➖ An allowlist risks missing dangerous custom methods
3. Use tokenization instead of AST parsing
  • ➕ Can inspect some malformed or incomplete Python source
  • ➕ Ignores many comment and string false positives
  • ➖ Requires custom structural analysis for calls, attributes, and aliases
  • ➖ Provides weaker semantic structure than the standard-library AST
  • ➖ Would still need conservative handling for ambiguous syntax

Recommendation: Keep the PR's standard-library AST approach. It provides the strongest bounded distinction without executing source or adding dependencies, while conservative alias handling, argument-bearing method detection, and regex fallback preserve the security posture. Full type inference is disproportionate, and regex or token-based approaches are less reliable for the covered syntax.

Files changed (3) +175 / -12

Bug fix (1) +48 / -1
code_quality.pyAdd AST-based Python eval classification +48/-1

Add AST-based Python eval classification

• Parses Python source to detect builtin eval references, imported aliases, and argument-bearing '.eval(...)' calls while allowing no-argument methods. Handles UTF-8 BOMs and falls back to existing text detection on syntax, value, or recursion failures; other languages retain regex detection.

src/codex_plugin_scanner/checks/code_quality.py

Tests (1) +117 / -11
test_code_quality.pyCover Python eval false positives and security boundaries +117/-11

Cover Python eval false positives and security boundaries

• Adds regression coverage for safe inference methods, dangerous builtin and aliased eval usage, argument-bearing methods, f-strings, parser failures, BOM input, and preselected files. Verifies unchanged high severity and JavaScript/TypeScript behavior, with minor formatter-only consolidation in existing shell-injection fixtures.

tests/test_code_quality.py

Documentation (1) +10 / -0
README.mdDocument syntax-aware Python eval detection +10/-0

Document syntax-aware Python eval detection

• Explains which Python eval references and calls remain findings, why no-argument methods are allowed, and when regex fallback applies. Clarifies that JavaScript and TypeScript behavior is unchanged and the heuristic is not type inference.

README.md

@coderabbitai

coderabbitai Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI (base), Organization UI (inherited)

Review profile: CHILL

Plan: Team

Run ID: f6d49547-7589-48d3-8bd4-8f8d33c580ca

📥 Commits

Reviewing files that changed from the base of the PR and between d9fc95e and 249276d.

📒 Files selected for processing (3)
  • README.md
  • src/codex_plugin_scanner/checks/code_quality.py
  • tests/test_code_quality.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/codex_plugin_scanner/checks/code_quality.py

Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review.


📝 Walkthrough

Walkthrough

The scanner now uses Python AST analysis to distinguish dynamic eval usage from no-argument inference methods. It preserves conservative text fallback on parse failures and keeps non-Python detection unchanged. Tests and README documentation cover the updated behavior.

Changes

Python eval detection

Layer / File(s) Summary
AST-based Python detection
src/codex_plugin_scanner/checks/code_quality.py
Python files use AST bindings and call inspection to detect dynamic eval usage while excluding no-argument inference methods.
Detection dispatch and validation
src/codex_plugin_scanner/checks/code_quality.py, tests/test_code_quality.py
Python files use AST detection, other files retain text detection, and tests cover dynamic calls, safe inference calls, parser failures, selected files, and existing JavaScript and TypeScript behavior.
Documented eval behavior
README.md
The README documents detection rules, fallback behavior, unchanged non-Python checks, and heuristic scope.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 24927

Python scanning now avoids flagging no-argument inference methods while retaining detection of dynamic eval patterns and existing non-Python behavior. No concrete merge-blocking risk remains.

Sequence Diagram(s)

sequenceDiagram
  participant Scanner
  participant PythonParser as ast.parse
  participant AST as Python AST
  participant TextCheck as EVAL_RE
  Scanner->>PythonParser: Parse Python content
  PythonParser-->>Scanner: Return AST
  Scanner->>AST: Inspect bindings and eval calls
  PythonParser-->>Scanner: Raise parser error
  Scanner->>TextCheck: Apply conservative text detection
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 5.88% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 17 functions across 2 files. (1 skipped: 1… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: preventing Python inference methods from being misidentified as dynamic eval usage.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 5.88% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 17 functions across 2 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@qodo-free-for-open-source-projects

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Grey Divider

Tip of the day
💡 Did you know, you can keep summaries lean with Finding overflow, which tucks the rest behind 'View more'

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

@coderabbitai

coderabbitai Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants