Repository navigation
PDFCLOUD-6418: Upgrade repository quality scaffolding - #48
Conversation
✅ Deploy Preview for pdfrest-python ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
867a795 to
9db1576
Compare
|
From Codex
|
datalogics-tsmith
left a comment
datalogics-tsmith
left a comment
There was a problem hiding this comment.
Requesting changes for the two inline findings below: add durable regression coverage for the new filename quality gate, and keep the required contributor hook aligned with the repository's Python 3.10 baseline.
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
datalogics-tsmith
left a comment
datalogics-tsmith
left a comment
There was a problem hiding this comment.
Requesting changes for the three inline findings below.
Please also correct the PR description: its Risks section says broader .gitignore refreshes and unrelated tool-version upgrades were deferred, but both are present in this diff. The branch is also 19 commits behind main; update it and rerun the official checks against the current base before merge.
The earlier Python 3.10 tooling comment is no longer blocking—the repository's Python 3.11 contributor/quality baseline adequately supports the current declaration while the SDK matrix continues to cover Python 3.10–3.14.
Keep service inputs out of mutating formatters and preserve their Git checkout bytes. Add explicit LF and whitespace defaults while retaining Markdown hard breaks and the existing Python attributes. Exclude the uv lockfile from TOML sorting. Assisted-by: Codex
Check JSON syntax, case conflicts, merge markers, private keys, and GitHub Actions workflows. Exempt only the public signing-key fixture from private-key detection. Add a standalone NFC filename checker with an always-run hook and README usage. Resolve Git explicitly and report unavailable Git, invalid UTF-8 paths, and normalization collisions. Assisted-by: Codex
Run basedpyright through the frozen project environment and retire its separate workflow. Run all quality hooks in the shared job with read-only repository permissions, a timeout, pinned actions, and a hook cache keyed by platform, tool versions, and configuration. Assisted-by: Codex
- Vendor the upstream NFC checker with provenance and byte exclusions. - Refresh complete ignore modules while preserving local amendments. - Pin compatible quality tools and CI actions. - Retain the established Ruff rule selection across the tool upgrade. Assisted-by: Codex
- Include Markdown in Ruff formatting while preserving fixture bytes. - Format Python examples in the file-usage guide. Assisted-by: Codex
- Remove the Docs workflow and its GitHub Pages deployment. - Update CI guidance for Docs Check and Netlify hosting. Assisted-by: Codex
Assisted-by: Codex
1367d7e to
03190a6
Compare
datalogics-kam
left a comment
datalogics-kam
left a comment
There was a problem hiding this comment.
Re-reviewed rebased head 03190a6 against main at ada92c5, scoped to repository quality-tooling adoption and the outstanding requests. No new blocking findings in that scope.
| Concern | Current assessment |
|---|---|
| Private-source disclosure | Addressed in public documentation, the PR description, and the seven reachable rebased branch commits. The sanitation is folded into 65d290e; the previously reported source identifiers are absent from the scanned messages and added patch lines. |
| Local checker implementation tests / public upstream test suite | Outside the established local review boundary. The checker remains vendored and upstream-owned, with unchanged installed SHA-256. No local implementation tests or public upstream suite were added, and comprehensive upstream failure-path coverage is not claimed. Local integration passed current-head pre-commit. |
| Python 3.10 tooling declaration | Already withdrawn by the reviewer. Contributor quality CI uses Python 3.11 while SDK tests and examples retain Python 3.10–3.14. |
| basedpyright workflow documentation | Addressed. AGENTS.md describes pre-commit and Test and Publish, with basedpyright included in the pre-commit job. |
| Misstated deferred upgrades and stale base | Addressed. The description acknowledges the ignore-module/tool upgrades, and the rebased branch is zero commits behind main. |
Current-head Test and Publish and pre-commit passed: Docs Check, all Python 3.10–3.14 unit-test and live-service example jobs, and Python 3.11 live tests. The security scan and deployment preview also passed. Diff coverage and documentation preview uploads remain PR-only; publishing is correctly skipped for this PR. Local suites were not repeated because the rebased final tree exactly matches the previously validated tree and hosted checks validate the new SHA.
One non-blocking description follow-up remains: the body still says the fixup needs autosquashing, describes the old merge, and links validation for 1367d7e. The fixup is now handled. Refresh that status wording and use the current-head CI links above.
I posted evidence-backed replies on all five threads and left their resolution status unchanged. No tracked files were edited during this review.
This branch was successfully deployed
2 participants
PDFCLOUD-6418
Why this change
The contributor quality setup lacked structural and filename checks, ran basedpyright in a separate workflow, and allowed mutating formatters to rewrite service fixtures. Consolidate the quality gate and protect fixture bytes while retaining the established development tools.
What changed (high level)
Run basedpyright through the frozen project environment in the pre-commit job. Add structural checks, actionlint, and an upstream-maintained vendored UTF-8 NFC filename checker. Refresh the complete sourced ignore and attribute modules, hook revisions, development-tool pins, and quality-workflow action pins. Keep maintained Python under resource directories checked, preserve fixture and vendored bytes, and exclude uv.lock from TOML sorting.
The vendored tooling documentation identifies the installed copy by SHA-256 and defines the local review boundary as artifact identity, updates, public documentation, and hook integration. Implementation review and implementation tests remain upstream; this repository does not add duplicate checker implementation tests.
Remove the obsolete GitHub Pages deployment workflow while retaining strict documentation builds and PR documentation previews in Test and Publish. Contributor guidance now correctly describes basedpyright as part of the pre-commit job.
Behavior changes
Quality CI runs on Ubuntu/Python 3.11 for pull requests and pushes to main, develop, and feature-*, using read-only repository permissions, frozen dependency installation, pinned actions, a runtime-aware hook cache, and a 20-minute timeout. Filename violations fail the installed hook without renaming files. The private-key detection exception remains limited to the public signing-key fixture.
Test and Publish retains non-live tests and live-service examples on each Python 3.10–3.14 matrix entry. Live tests run on Python 3.11 for PRs and scheduled runs. Diff coverage and documentation preview uploads are PR-only; strict Docs Check runs on Python 3.11. Publishing remains release-only and depends on documentation, tests, and examples. This PR introduces no SDK API or package-version changes relative to current main.
Validation
Merged current main (ada92c5) without conflicts and validated the combined head in a clean disposable clone. Ruff formatting left all 149 Python files unchanged, Ruff lint passed, basedpyright reported zero errors or warnings, the full pre-commit suite passed, and the strict MkDocs build passed. The installed filename hook passed and the vendored checker checksum is unchanged.
uvx nox -s tests -- -m 'not live'passed on Python 3.10–3.14, with 2,838 passed and 10 existing format-matching skips per interpreter. Client class-function coverage gates passed on all five interpreters. Diff coverage found no covered source lines changed relative to current main. Reports remain undercoverage/py<version>/in the validation clone.Hosted checks passed on head 1367d7e: pre-commit, Docs Check, every Python 3.10–3.14 test and example job, Python 3.11 live tests, the security scan, and the deployment preview. Test and Publish results and pre-commit results are available for review.
Live tests and examples were validated in hosted CI rather than repeated locally. Package publishing was skipped because this is not a release. Unrelated staged and unstaged changes and all eight untracked user files were verified unchanged.
Risks and follow-ups
Basedpyright now contributes to the pre-commit status instead of a separate workflow. The diff includes the ignore-module refresh and quality-tool upgrades described above; they are not deferred. The provenance sanitization is committed as a fixup targeting the commit that introduced the disclosure. Autosquash that fixup before merge so the private-source references also disappear from branch commit patches. Review thread resolution remains unchanged.