Skip to content

Require Python 3.11+ for permission doc generators and fail on parse errors - #74654

Merged
potiuk merged 2 commits into
apache:mainfrom
Pushkal-Gupta:fix-permission-doc-silent-truncation
Oct 18, 2026
Merged

potiuk merged 2 commits into
apache:mainfrom
Pushkal-Gupta:fix-permission-doc-silent-truncation

Conversation

@Pushkal-Gupta

Copy link
Copy Markdown
Contributor

If prek runs the permission-doc hooks on a Python older than 3.11, they quietly delete 84 lines from api_permissions_ref.rst and another 84 from the FAB access-control.rst, and then tell you it worked.

What's going on

Two hooks regenerate those docs, generate-api-permissions-doc and generate-fab-permissions-doc. Both statically parse the route files under api_fastapi/core_api/routes/public/ with ast, 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 every pyproject.toml in 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:

Generate REST API permission reference documentation.......................Failed
  [WARN] Could not parse assets.py: invalid syntax (<unknown>, line 217)
  [WARN] Could not parse dag_stats.py: invalid syntax (<unknown>, line 78)
  [OK] Written 116 entries to .../api_permissions_ref.rst

 airflow-core/docs/security/api_permissions_ref.rst | 84 ----------------------
 providers/fab/docs/auth-manager/access-control.rst | 84 ----------------------
 2 files changed, 168 deletions(-)

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.6 python3 gives 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.py imports the same helper, which is why both docs shrink together.

How much is affected

CI is fine, because CI's python3 resolves 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 in git 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 with ast, 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 six extract_permissions.py is the only one that responds to a parse failure by carrying on regardless. check_registry_types_json_sync.py and check_applies_to_roots_in_sync.py have no handler at all and just traceback, check_contextmanager_class_decorators.py reports the parse error as a finding, and check_secrets_search_path_sync.py returns None so 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 in scripts/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 now test_syntax_error_aborts_instead_of_dropping_entries, and I added test_unreadable_file_aborts to cover the OSError arm of the same except.

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.11 rather than >=3.11,<3.12 like 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?
  • Yes — Claude Code (Opus 5)

Comment thread scripts/ci/prek/extract_permissions.py Outdated
Comment thread scripts/ci/prek/extract_permissions.py Outdated
@potiuk potiuk changed the title Pin permission doc generators to Python 3.11 and fail on parse errors Require Python 3.11+ for permission doc generators and fail on parse errors Oct 18, 2026
@potiuk
potiuk merged commit 2fab095 into apache:main Oct 18, 2026
72 checks passed
@potiuk potiuk added the backport-to-v3-4-test Backport to v3-4-test label Oct 18, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Backport successfully created: v3-4-test

Note: As of Merging PRs targeted for Airflow 3.X
the committer who merges the PR is responsible for backporting the PRs that are bug fixes (generally speaking) to the maintenance branches.

In matter of doubt please ask in #release-management Slack channel.

Status Branch Result
✅ v3-4-test PR Link

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants