Repository navigation
Require Python 3.11+ for permission doc generators and fail on parse errors - #74654
Merged
potiuk merged 2 commits intoOct 18, 2026
Merged
Conversation
Pushkal-Gupta
requested review from
amoghrajesh,
ashb,
bugraoz93,
gopidesupavan,
jason810496,
jscheffl and
potiuk
as code owners
October 13, 2026 14:18
Andrushika
approved these changes
Oct 14, 2026
1 task done
potiuk
approved these changes
Oct 18, 2026
1 task done
Contributor
Backport successfully created: v3-4-testNote: As of Merging PRs targeted for Airflow 3.X In matter of doubt please ask in #release-management Slack channel.
|
potiuk
pushed a commit
that referenced
this pull request
Oct 18, 2026
…il on parse errors (#74654) (#75019) * Pin permission doc generators to Python 3.11 and fail on parse errors * Raise instead of exiting on parse failure and point docs at uv run (cherry picked from commit 2fab095) Co-authored-by: Pushkal Gupta <161060351+Pushkal-Gupta@users.noreply.github.com> Co-authored-by: Rahul Vats <43964496+vatsrahul1001@users.noreply.github.com>
vatsrahul1001
added a commit
that referenced
this pull request
Oct 20, 2026
…il on parse errors (#74654) (#75019) * Pin permission doc generators to Python 3.11 and fail on parse errors * Raise instead of exiting on parse failure and point docs at uv run (cherry picked from commit 2fab095) Co-authored-by: Pushkal Gupta <161060351+Pushkal-Gupta@users.noreply.github.com> Co-authored-by: Rahul Vats <43964496+vatsrahul1001@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
If prek runs the permission-doc hooks on a Python older than 3.11, they quietly delete 84 lines from
api_permissions_ref.rstand another 84 from the FABaccess-control.rst, and then tell you it worked.What's going on
Two hooks regenerate those docs,
generate-api-permissions-docandgenerate-fab-permissions-doc. Both statically parse the route files underapi_fastapi/core_api/routes/public/withast, which means the interpreter running the hook has to be at least as new as the code it is reading. Two route files use PEP 646 star-unpacking in a subscript (assets.py:217,dag_stats.py:78), so they need 3.11 to parse at all. That's fine, since everypyproject.tomlin the repo sets a Python floor of 3.11.Neither script carries a PEP 723 block though, so there is nothing telling prek which interpreter they need and it falls back to
default_language_version: python: python3. Here that resolved to a managed 3.10.20, which cannot parse those two lines, and this is what came out:137 entries on 3.11, 116 on 3.10. The script exits 0 and prints
[OK]. Running it straight from the shell with a 3.9.6python3gives the same 116.The reason it degrades instead of stopping is
extract_from_file: it catches(OSError, SyntaxError), warns on stderr, and returns an empty list. The caller has no way to tell that apart from a route file that genuinely declares no routes, so the endpoints just vanish and the run still succeeds.fab_permissions_doc.pyimports the same helper, which is why both docs shrink together.How much is affected
CI is fine, because CI's
python3resolves to 3.11+. That is luck rather than design though: nothing in the hook definitions requires it, which is how my machine ended up on 3.10. I think that is exactly why this has survived: the hook rewrites the doc short, you see two docs you never touched show up ingit diff, and the hook output says[OK]. I nearly committed the 168-line deletion myself while working on an unrelated PR.It is also an odd way for one of these scripts to behave. Plenty of hooks in
scripts/ci/prek/read the repo's own source withast, and most of them pin the Python they need, so they never meet an interpreter that cannot parse it. Six do not pin, and of those sixextract_permissions.pyis the only one that responds to a parse failure by carrying on regardless.check_registry_types_json_sync.pyandcheck_applies_to_roots_in_sync.pyhave no handler at all and just traceback,check_contextmanager_class_decorators.pyreports the parse error as a finding, andcheck_secrets_search_path_sync.pyreturnsNoneso its caller bails.The fix
Two small changes.
Pin both scripts to
requires-python = ">=3.11"with a PEP 723 block, which is how 112 of the other scripts inscripts/ci/prek/already declare their interpreter. prek then picks a conforming Python and the hooks simply work.Then make a parse failure fatal rather than silent. The pin removes today's trigger, but the generator's job is to produce a complete table, so any future unparseable or unreadable file should stop the run instead of shrinking a security reference nobody is going to re-count by hand.
After the change both hooks pass and leave the docs byte-identical. With a route file deliberately broken, the script now says
[FAIL] Could not parse <path>: invalid syntax, exits 1, and leaves the doc alone instead of rewriting it short.Tests
The existing suite had
test_syntax_error_returns_empty_list, which asserted the old behaviour directly (entries == [],"WARN" in captured.err), so it had to change. It's nowtest_syntax_error_aborts_instead_of_dropping_entries, and I addedtest_unreadable_file_abortsto cover theOSErrorarm of the sameexcept.Both new tests fail against unpatched
extract_permissions.py; I checked by stashing just that file.scripts/tests/passes at 1678, and ruff check and format are clean on all three files.No newsfragment, since this is dev tooling with no user-facing change.
One open question
I pinned
>=3.11rather than>=3.11,<3.12like most siblings use, because for these two scripts the floor is what matters and a ceiling would only get stale. Happy to match the majority if you'd rather these look like the rest.The swallow came in with #67606 and was inherited by #72230. It reads like a reasonable choice at the time, when a warning on stderr would plausibly be seen, and it only turns into silent data loss because prek routes hook output the way it does.
Was generative AI tooling used to co-author this PR?