Skip to content

Fix Pipenv-owned CodeQL reliability findings - #6696

Open
matteius wants to merge 1 commit into
mainfrom
maintenance/codeql-reliability-2026-08
Open

Fix Pipenv-owned CodeQL reliability findings#6696
matteius wants to merge 1 commit into
mainfrom
maintenance/codeql-reliability-2026-08

Conversation

@matteius

@matteius matteius commented Aug 3, 2026

Copy link
Copy Markdown
Member

The issue

GitHub Code Quality currently reports 186 open reliability findings and rates the repository Poor. Most of that backlog is imported code that Pipenv does not maintain directly, while a smaller set of Error- and Warning-level findings affects Pipenv-owned code and tests.

Inventory from the 2026-08-03 default-branch scan:

  • pipenv/patched/**: 84 reliability findings (317 total findings)
  • pipenv/vendor/**: 42 reliability findings (92 total findings)
  • docs/_static/konami.js: 5 reliability findings in an imported upstream asset
  • Pipenv-owned code/tests/tasks: 55 reliability findings, of which 9 are Error or Warning severity and 46 are Notes

The imported findings are useful dependency inventory, but should not drive changes to copied code. GitHub Code Quality's current generated setup does not expose a repository path-ignore option; the similarly named CodeQL paths-ignore configuration applies to code scanning, not the Code Quality dashboard. Those imported findings should therefore be dismissed as third-party/won't-fix in Code Quality so they remain auditable under Dismissed without affecting the open backlog.

The fix

This PR leaves all imported sources untouched and addresses the 9 Error- and Warning-level findings in Pipenv-owned files:

  • close Pipfile handles deterministically in the check/scan routines and integration test
  • fix corrupt-file exception initialization and the invalid unbound show call, retaining the real path, backup path, and file-specific message
  • suppress two deliberate wrong-argument calls that test the keyword-only API contract
  • add regression coverage for corrupt Pipfile and lockfile messages

Validation

  • python -m pytest -o addopts='' -q tests/unit/test_utils_exceptions.py tests/unit/test_routine_context.py — 32 passed
  • python -m pytest -o addopts='' -q tests/integration/test_project.py::test_pipfile_envvar_expansion — passed
  • python -m pytest -o addopts='' -q tests/integration/test_lock.py::test_lockfile_corrupted — passed
  • pre-commit hooks on all changed files — passed

A wider run of tests/integration/test_project.py had 39 passing tests and one unrelated failure because the private package-index fixture expected a service on localhost:8080, which was not running locally.

The checklist

  • Associated issue
  • A news fragment in the news/ directory

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Lockfile.load()’s JSONDecodeError recovery path calls cls.load(...) without returning it, which will continue execution with an uninitialized projectfile and can raise UnboundLocalError instead of recovering.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

Addresses GitHub Code Quality (reliability) findings in Pipenv-owned code by fixing exception/message handling for corrupt Pipfile/lockfile scenarios, ensuring file handles are closed deterministically, and adding targeted regression tests to prevent reintroducing these issues.

Changes:

  • Refactors corrupt-file exceptions to preserve file-specific messaging and removes the invalid unbound show call pattern.
  • Switches Pipfile loading to use context managers (with open(...)) in routines and an integration test to avoid leaked file handles.
  • Adds/updates unit tests and a news fragment documenting the behavior fix.
File summaries
File Description
tests/unit/test_utils_exceptions.py Adds regression tests asserting corrupt-file exceptions preserve specific messages and print to stderr.
tests/unit/test_routine_context.py Adds CodeQL suppression for intentional wrong-argument calls in keyword-only API contract tests.
tests/integration/test_project.py Ensures Pipfile file handle is closed deterministically during envvar expansion test.
pipenv/utils/locking.py Updates corrupt-lockfile error reporting call site to instantiate exception and call show().
pipenv/utils/exceptions.py Refactors corrupt-file exceptions to rely on base initialization and a unified show() implementation.
pipenv/routines/scan.py Closes Pipfile handle deterministically in PEP 508 check routine.
pipenv/routines/check.py Closes Pipfile handle deterministically in PEP 508 check routine.
news/+code-quality-reliability.bugfix.rst Documents the corrupt Pipfile/lockfile error messaging fix.
Review details

Suppressed comments (1)

pipenv/utils/locking.py:421

  • In the JSONDecodeError recovery path, cls.load(formatted_path, create=True) is called but its return value is ignored. After the except block, the function continues and later references projectfile, which is not set in this branch—this will raise UnboundLocalError and prevent recovery from working.
            LockfileCorruptException(formatted_path, backup_path=backup_path).show()
            path_obj.rename(backup_path)

            # Try loading again after backing up corrupted file
            cls.load(formatted_path, create=True)
  • Files reviewed: 8/8 changed files
  • Comments generated: 0
  • Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

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