Skip to content

fix(testmap): Python modules were offered as runnable test scripts - #236

Open
qinghuanandejiangshi wants to merge 3 commits into
redhat-et:mainfrom
qinghuanandejiangshi:feature/20260915-python-test-runners
Open

qinghuanandejiangshi wants to merge 3 commits into
redhat-et:mainfrom
qinghuanandejiangshi:feature/20260915-python-test-runners

Conversation

@qinghuanandejiangshi

Copy link
Copy Markdown
Contributor

Python test modules received run="python3 ..." even when importing the file runs no tests or fails without Django settings. Run hints now require a top-level main guard for direct Python execution, or pytest configuration between the test directory and its crawl root. Modules without that evidence retain the existing unknown-runner disclosure.

The existing parsers distinguish main guards and TOML tables from documentation examples. Candidate commands are cached, fallback runners stay within their own workspace root, and shell quoting plus option termination apply to pytest paths too. Django settings and custom test runners are left unknown rather than inferred.

Closes #229.

Validation (macOS):

  • The added regression fails on the original binary; test/runhintcheck.sh passes on the fix, covering test-gate JSON, affected XML, situ text, unknown-runner disclosures, root boundaries, fallback and hostile paths.
  • Real pytest execution verifies both shell-metacharacter and leading-dash filenames actually run the test without the injected side effect.
  • Sanitizer build: full-repository cold scan and runhintcheck.sh pass with no sanitizer error output. Three fixture scans are byte-identical, cold/warm caches agree, XML parses, and the committed golden matches.
  • Full suite: 632 gates exercised, 628 passed and 3 conditionally skipped. The remaining gate, ripwirepubliccheck.sh, failed because pdftotext was missing; it passes after installing Poppler. Final runhint/cppqual checks and the format gate pass.
  • The conditional skips are two optional reference-binary comparisons and the freshness probe for an in-tree asan/ripwire; the sanitizer build and execution above used a separate build directory. Local checks used base 216802ad; git merge-tree confirms a clean merge into current main 30f14a27.
  • The quality ledger records only the measured TestRunnerIndex size/churn footprint. Complexity and cache duplication in the initial implementation were refactored; other findings remain visible.

@coderabbitai

coderabbitai Bot commented Sep 15, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 9d7b2212-668a-4650-8ec0-e3e36c391989

📥 Commits

Reviewing files that changed from the base of the PR and between 2d2b10f and 6995356.

📒 Files selected for processing (3)
  • src/pythonrunner.h
  • src/testmap.h
  • test/runhint_python.py
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/testmap.h
  • test/runhint_python.py
  • src/pythonrunner.h

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


📝 Summary

Summary by CodeRabbit

  • New Features

    • Improved Python test-runner detection using main guards and pytest project configuration.
    • Runner hints now respect project boundaries, support multiple roots, and safely handle unusual file paths.
    • Generated commands use the detected runner and safely separate options from file paths.
    • Repeated runner detection is more efficient through command caching.
  • Tests

    • Added end-to-end coverage for Python runner detection, configuration boundaries, fallback behavior, and hostile paths.
    • Updated quality checks for newly covered file-reading scenarios.

Walkthrough

The change adds evidence-based Python runner detection, root-scoped matching, command caching, safe path handling, and end-to-end tests for pytest, main guards, fallback runners, and isolated project roots.

Changes

Python runner detection

Layer / File(s) Summary
Python evidence parsing
src/pythonrunner.h, test/cppqualcheck.sh
Adds parsing for Python main guards and pytest configuration. The qualification gate updates the expected whole-file read count.
Root-scoped runner selection
src/testmap.h
Uses main-guard and pytest-project evidence, restricts candidates to matching roots, skips candidates without commands, caches generated commands, and places -- before unsafe paths.
Runner hint validation
test/runhint_python.py, test/runhintcheck.sh, .ripwire_quality_acks
Adds end-to-end coverage for Python runner hints, root isolation, configuration parsing, unsafe paths, and shell-gate integration. Updates quality acknowledgments.

Priority: ➖ Normal

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant TestRunnerIndex
  participant PythonEvidence
  participant ProjectRoot
  participant RunnerHintTests
  TestRunnerIndex->>PythonEvidence: inspect source and pytest evidence
  PythonEvidence->>ProjectRoot: read configuration within crawl root
  ProjectRoot-->>PythonEvidence: return matching project evidence
  PythonEvidence-->>TestRunnerIndex: return validated runner
  TestRunnerIndex-->>RunnerHintTests: emit runner hint
  RunnerHintTests->>TestRunnerIndex: verify roots, commands, and unsafe paths
Loading

Merge Risk: ⚪ Minimal · up to 69953

The runner-hint paths have no identified merge-blocking issue.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 18 functions across 5 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the main change: preventing Python modules from being incorrectly offered as runnable test scripts.
Description check ✅ Passed The description directly explains the Python runner-hint changes, evidence requirements, boundary handling, tests, and validation results.
Linked Issues check ✅ Passed The PR satisfies the coding requirements in [#229]. src/pythonrunner.h recognizes only top-level main guards and valid pytest configuration. src/testmap.h emits pytest <path> only when configura…
Out of Scope Changes check ✅ Passed The changes remain within [#229]. The parser helpers implement runner evidence detection. The command cache, crawl-root checks, quoting, option termination, and fallback handling prevent unsafe or mis…
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@joyful-ii-V-I

Copy link
Copy Markdown
Collaborator

Thanks @qinghuanandejiangshi. This makes run= trustworthy on Python repos: an agent in a Django/pytest tree now gets pytest <path> or an honest run_unknown instead of a python3 <module> that dies with ImproperlyConfigured. The negative fixtures are exactly the cases that matter: guards inside docstrings, comments and nested defs; [tool.pytest.ini_options] inside a TOML string; [tool:pytest] on a setup.cfg continuation line; config above the crawl root; and multi-root isolation. Giving runhintcheck arm (6)'s dash runner a real main guard, so that arm stays live, was a careful catch.

Must fix before merge

  1. A .py row with no evidence now falls through to the stem/mention fallback in derive() (src/testmap.h:612-617), which it never reached before. The stem pass then picks a different file with the same basename. Take a/pytest.ini, a/tests/test_api.py and b/tests/test_api.py: the row for b/tests/test_api.py gets run="pytest a/tests/test_api.py", which passes without running the changed file. The mention pass is a substring match, so a Django app/tests.py also matches any script that says runtests.py. It also sends every evidence-less Python row through a scan of all runner texts, which grows as rows × bytes on big unittest/Django repos. Suggested fix: for a self-runnable kind, return spell( fileId ); even when empty, as before this PR (or at least skip same-extension candidates in the stem pass). Please also add a runhint_python.py arm: same basename in two directories, only one with evidence, and the other row must read run_unknown.

Optional, fine as a follow-up

  • pytest's -p plugin pre-parse ignores -- (checked on pytest 9.1.1), so a file named -pfoo_test.py gives "Error importing plugin" instead of running the test. Spelling a dash-leading path as ./-… fixes that for bash, python3 and pytest; otherwise narrow the comment at testmap.h:737.
  • pytest also reads .pytest.ini, pytest.toml/.pytest.toml, tox.ini [pytest] and [tool.pytest] (pytest ≥ 9). For now those projects honestly read run_unknown.
  • If the fix moves TestRunnerIndex's measured size or churn, re-heal .ripwire_quality_acks with the binary (--quality-ack), not by hand.

CodeRabbit comments you can safely ignore

  • Docstring Coverage (66.67% < 80%): a generic CodeRabbit threshold, not a project rule. The /// contracts you added are enough.

Queue: this PR is 3rd in landing order — #231#237#236#235 + #233 (reconciled together) → #224#44 (Windows, being refactored).

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.

--situ/--affected emit run="python3 <file>" for Django/pytest test modules, which cannot be run that way

2 participants