Skip to content

fix(python): derive runtime version from metadata - #332

Draft
NikolayS wants to merge 3 commits into
mainfrom
agent/fix-python-version
Draft

NikolayS wants to merge 3 commits into
mainfrom
agent/fix-python-version

Conversation

@NikolayS

Copy link
Copy Markdown
Owner

Summary

  • make installed pgque-py metadata authoritative for pgque.__version__
  • fall back to the source pyproject.toml for direct checkout imports
  • verify source, editable, wheel metadata, and isolated installed-wheel versions in the release workflow
  • document the single-source version contract

Fixes #326.

Validation

  • red-first version regression
  • Python 3.9 tests: 10 passed, 70 skipped (DB-gated)
  • clean Python 3.12 source/editable/build/wheel validation
  • twine check passed for wheel and sdist
  • isolated installed-wheel runtime matched wheel metadata
  • actionlint, YAML parsing, and git diff --check passed

@NikolayS

Copy link
Copy Markdown
Owner Author

Real-user verification evidence

The original RED is retrospective because implementation and tests were committed together. Loading the final version tests with the base package failed collection with ModuleNotFoundError: pgque._version; head version tests now pass 5/5, including the branch dry-run contract.

Supported-runtime package path:

docker run --rm -v "$PWD/clients/python:/src:ro" -w /tmp python:3.13-slim sh -c '
  set -eu
  cp -R /src /tmp/pkg && cd /tmp/pkg
  python -m pip install ".[dev]" build
  PGQUE_TEST_DSN=postgresql://nik@host.docker.internal/pgque_pr_rules_audit python -m pytest -q
  python -m build
  python -m pip install --force-reinstall dist/*.whl
  cd /tmp && python -c "import pgque; assert pgque.__version__ == \"0.2.0\""
'

Result: 80 passed; sdist and wheel built; installed-wheel runtime metadata was 0.2.0.

The branch dry_run=true executed source, editable, build, Twine, wheel-metadata, and isolated-install validation: https://github.com/NikolayS/PgQue/actions/runs/29156149467. Both PyPI publishing jobs were skipped, and no tag or release path ran. Final head CI: 17/17 checks passed.

@NikolayS NikolayS left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

REV rubric review

Automated ultrareview quota was unavailable, so I executed the five official
REV rubric prompts manually. SOC2 findings were omitted as instructed. This is
a comment-only review, not an approval.

Blocking findings

None. I found no unresolved functional, security, documentation, or test
blocker on head 423a76e157e45e35a92aeda32f010c2e83e467f2.

Rubric results

  • Bug hunter: installed distribution metadata is authoritative; an unpackaged
    source tree falls back to the [project] version and has an explicit unknown
    result when source metadata is absent.
  • Security reviewer: the parser does not evaluate project content, and publish
    jobs remain gated by dry_run, repository choice, environments, and OIDC.
  • Docs reviewer: the single-source version contract and dry-run validation
    path are documented accurately.
  • Guidelines checker: all current commits use allowed types, subjects are
    under 50 characters, the test-first dry-run correction is chronological,
    and all 17 CI checks pass.
  • Test analyzer: source, editable, wheel metadata, isolated wheel install, and
    branch dry-run/publish exclusions are covered. Exact-head workflow dispatch
    run 29156149467 passed; both PyPI publishers were correctly skipped.

Nonblocking findings

The original version fix has retrospective RED evidence because implementation
and tests share a commit; this is disclosed in the evidence comment and cannot
be reordered without rewriting history. No current change is requested.

@NikolayS

NikolayS commented Aug 3, 2026

Copy link
Copy Markdown
Owner Author

samorev Code Review Report

Pipeline Coverage
unknown Not reported

REVIEW FINDINGS (15)

HIGH MR/PR state - Review target is draft

The review target is still marked as draft.
Fix: Mark it ready for review before merge.

CRITICAL CI/Pipeline - Pipeline status is unknown

Provider CI reported status unknown.
Fix: Fix failing checks and rerun review.

MEDIUM [bugs] Installed pgque-py metadata unconditionally shadows the source tree that is actually being imported, so __version__ can report a version that does not describe the running code. The old static literal was always correct for this case.

clients/python/pgque/_version.py:
Fix: Cross-check that the resolved distribution actually owns the imported module before trusting it — e.g. compare metadata.distribution(_DISTRIBUTION_NAME)._path/files() against Path(__file__).parent, or resolve source_version() first when _PYPROJECT_PATH exists (an unpackaged checkout) and fall back to metadata otherwise. At minimum, document in RELEASE.md that a source checkout imported alongside an installed release reports the installed version.

MEDIUM [bugs] The source fallback reads whatever pyproject.toml sits two directories up without verifying it belongs to pgque-py, so a vendored or relocated copy of the package silently reports the host project's version.

clients/python/pgque/_version.py:
Fix: Parse name alongside version inside [project] and return _UNKNOWN_VERSION unless it normalises to pgque-py. The existing test test_source_tree_fallback_reads_project_version already writes name = "pgque-py" into its fixture, so the assertion is cheap to add.

MEDIUM [bugs] Hand-rolled line-oriented TOML parsing instead of tomllib/tomli. The table tracker flips on any line beginning with [, so a version that follows a multi-line array inside [project] is missed, and a [project] using dynamic = ["version"] degrades silently to 0+unknown.

clients/python/pgque/_version.py:
Fix: Use tomllib on 3.11+ with a tomli fallback (or tomli unconditionally, since this path only runs in a dev checkout where the extra dep is acceptable), and read data["project"]["version"]. If the hand-rolled parser must stay, track bracket depth so array continuation lines don't clear in_project_table, and normalise stripped.strip("[]").strip() for the table name.

MEDIUM [guidelines] The release workflow hardcodes the client's runtime dependency and installs the editable client with --no-deps, duplicating the dependency set that pyproject.toml owns. This contradicts the PR's own "single-source" premise and drifts silently.

.github/workflows/release-python.yml:
Fix: Install only the tooling here (build, packaging, pytest, twine), and let the project supply its own runtime deps — run the clean-source assertion first, then python -m pip install -e . (without --no-deps) before pytest -q. That keeps pyproject.toml the single source for dependencies as well as version.

MEDIUM [tests] test_branch_release_dry_run_executes_validation_without_publish asserts exact substrings of a CI YAML file located via parents[3]. It tests formatting rather than behaviour, and errors outright when the test suite runs anywhere the repo root is not three levels up.

clients/python/tests/test_version.py:
Fix: Parse the workflow with yaml.safe_load and assert on the parsed jobs.<id>.if values, and guard with pytest.importorskip/pytest.skip(...) when RELEASE_WORKFLOW does not exist. Better still, move CI-shape assertions out of the shipped client test suite into a repo-level lint (the PR already runs actionlint).

MEDIUM [tests] The new parser's realistic failure modes are untested. Every fixture is a two-key happy path, so none of the regex/table-tracking edge cases that will actually break in production are covered.

clients/python/tests/test_version.py covers exactly three source-tree shapes: a well-ordered [project], a missing file, and installed metadata. Not covered: [project] present but with dynamic = ["version"] and no version key; version appearing after a multi-line array (the startswith("[") bug above); single-quoted version = '0.2.0'; a trailing # comment (the regex has a branch for it); a pyproject.toml belonging to a different project. The default _PYPROJECT_PATH is also never asserted to resolve to clients/python/pyproject.toml — every test monkeypatches it away.
Fix: Add table-driven cases for each shape above asserting either the correct version or 0+unknown, plus one test that version_module._PYPROJECT_PATH == Path(pgque.__file__).parent.parent / "pyproject.toml" and that it exists in the checkout.

LOW [tests] test_runtime_version_matches_available_package_metadata re-implements resolve_version()'s own branch logic, so it is near-tautological and cannot fail for the drift it is named after.

clients/python/tests/test_version.py:
Fix: Assert against an independently-derived value instead: read [project] version from the real clients/python/pyproject.toml with tomllib/tomli and assert pgque.__version__ equals it (skipping when an unrelated pgque-py dist is installed). That is the invariant the PR actually claims.

MEDIUM [security] Relaxing the build job's ref guard lets code from any branch execute under the workflow-level permissions: block, which for a Trusted-Publisher release workflow normally includes id-token: write. The build job has no job-level permissions: override.

.github/workflows/release-python.yml:
Fix: Add an explicit job-level permissions: { contents: read } to build so the relaxed ref guard cannot inherit publish-capable scopes, and confirm the PyPI/TestPyPI Trusted Publishers are configured with their environment names (RELEASE.md steps 4–5 say they are).

MEDIUM [docs] The workflow now runs from any branch on dry runs — a user-visible behaviour change that appears in neither the PR summary nor RELEASE.md, which still reads as if the workflow only operates on main.

The diff changes if: github.ref == 'refs/heads/main' to ... || inputs.dry_run, but the PR's four summary bullets do not mention it, and the RELEASE.md step 6 rewrite documents only the new validation content:
Fix: State in RELEASE.md step 6 that dry_run=true may be run from any branch (and that dry_run=false still requires main), and add the branch-guard change to the PR summary so reviewers see it in scope.

LOW [docs] RELEASE.md claims dry runs "run the test suite", implying a meaningful release gate; per the PR's own validation notes the suite is DB-gated and ~70 of 80 tests skip without Postgres, which the release job does not provide. The 0+unknown sentinel is also undocumented.

clients/python/RELEASE.md: "They also run the test suite and twine check, but do not require testpypi / pypi environment approval". The PR description reports 10 passed, 70 skipped (DB-gated), and the workflow adds no Postgres service — so the release gate exercises roughly an eighth of the suite. Separately, the new version paragraph never mentions that an unresolvable version surfaces as _UNKNOWN_VERSION = "0+unknown".
Fix: Say "runs the non-database portion of the test suite" (or add a Postgres service to the job and run it for real), and document 0+unknown as the signal that neither distribution metadata nor a source pyproject.toml was resolvable.

LOW [bugs] source_version catches only OSError, so a non-UTF-8 or otherwise undecodable pyproject.toml raises UnicodeDecodeError out of import pgque, making the whole package unimportable rather than degrading to 0+unknown.

clients/python/pgque/_version.py:
Fix: Broaden to except (OSError, ValueError), or wrap the _resolve_version() call in __init__.py so that no failure in version resolution can prevent the package from importing.

LOW [tests] monkeypatch.setattr(version_module.metadata, "version", ...) patches the shared importlib.metadata module globally for the duration of the test, not just this module's view of it, and the stub returns None for unknown distributions instead of raising.

clients/python/tests/test_version.py:
Fix: Have the stub raise metadata.PackageNotFoundError(distribution) for non-pgque-py names, matching the real contract. Cleaner still: import version into _version.py as a module-level name and patch version_module.version, so the patch cannot leak into the stdlib module.

LOW [bugs] The editable install is never uninstalled before python -m build, leaving setuptools metadata/build artifacts in the source tree that feed the subsequent sdist.

.github/workflows/release-python.yml runs python -m pip install --no-deps -e . in "Verify source and editable runtime versions", and the very next step runs python -m build in the same working directory with no cleanup. The editable install materialises *.egg-info/ (and possibly build/) in clients/python; setuptools consults existing .egg-info when computing sdist contents, so the published sdist can pick up state that a clean checkout would not produce.
Fix: End the verification step with python -m pip uninstall -y pgque-py and remove *.egg-info/build/, or move the editable check into its own job so the build job always starts from a pristine checkout.


Summary

Area Findings Potential Filtered
CI/Pipeline 1 0 0
Security 0 1 0
Bugs 1 4 0
Tests 0 4 0
Guidelines 0 1 0
Docs 0 2 0
Metadata 1 0 0

Note:

  • Findings: High-confidence issues (8-10/10) - blocking or non-blocking per severity
  • Potential: Medium-confidence issues (4-7/10) - review manually
  • Filtered: Low-confidence issues (0-3/10) - excluded as likely false positives
Review metadata
provider=github
kind=pr
project=NikolayS/PgQue
number=332
target=github:NikolayS/PgQue#332
state=OPEN
draft=true
diff_lines=367
diff_added=260
diff_removed=15
diff_bytes=12749
comments_count=1
commits_count=3
ci_status=unknown
ci_summary=total=21 success=18 failure=0 pending=0 other=3
prompt=.claude/commands/review-mr.md
blocking=false
posted_by=gh
no_comment=false
live_posting=posted

samorev-assisted review (AI analysis by Tanya301/samorev)

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.

release(python): make runtime version follow package metadata

1 participant