Pushkal-Gupta opened a new pull request, #74654:
URL: https://github.com/apache/airflow/pull/74654
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?
- [X] Yes — Claude Code (Opus 5)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
