Skip to content

Commit 535a961

Browse files
committed
Address review: guard hook names, use --from-ref by default, point agents at breeze verify
The manual-stage hook names in the mapping are now checked against ci-amd.yml, so a renamed hook fails a test instead of going stale. The default prek row uses --from-ref, the scope AGENTS.md recommends, and --full keeps CI's --all-files form. Step 7 of the publish-changes skill, the AGENTS.md commands block and the selective-checks doc now point at breeze verify. All patches in the CLI tests use autospec.
1 parent 1f7871e commit 535a961

9 files changed

Lines changed: 83 additions & 26 deletions

File tree

‎.agents/skills/airflow-publish-changes/SKILL.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,7 @@ code review checklist in [`.github/instructions/code-review.instructions.md`](..
115115
and fix any failures. The skipped hooks start long-running local servers or provision the
116116
opt-in Codex environment rather than run checks that complete.
117117
6. Run relevant individual tests and confirm they pass.
118-
7. Find which tests to run for the changes with selective-checks and run those tests in parallel to confirm they pass and check for CI-specific issues.
118+
7. Run `breeze verify` and execute every command it lists. The list comes from the same selective-checks logic CI uses. `breeze verify --full` shows everything CI runs for the default matrix cell, including the full suite CI adds when a change touches CI tooling or dependency files. Confirm the commands pass and check for CI-specific issues.
119119
8. Check for security issues — no secrets, no injection vulnerabilities, no unsafe patterns.
120120

121121
Before pushing, always rebase your branch onto the latest target branch (usually `main`)

‎AGENTS.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@ Don't spell out **Directed Acyclic Graph** except for historical context.
5454
- **Run regular (fast) static checks:** `prek run --from-ref <target_branch> --stage pre-commit`
5555
- **Run manual (slower) checks:** `prek run --from-ref <target_branch> --stage manual --skip compile-ui-assets-dev --skip view-skill-eval --skip run-skill-eval-codex` (the skipped hooks start long-running local servers or provision the opt-in Codex environment rather than run checks that complete)
5656
- **Build docs:** `breeze build-docs`
57+
- **List the local verification for your changes:** `breeze verify` (add `--full` for everything CI runs for the default matrix cell, `--json` for machine-readable output). It uses the same selective-checks logic as CI.
5758
- **Determine which tests to run based on changed files:** `breeze ci selective-check --commit-ref <commit_with_squashed_changes>`
5859
<!-- END generated-commands, please keep comment here to allow auto update -->
5960

‎contributing-docs/08_static_code_checks.rst‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -236,6 +236,7 @@ To check other usage types of the pre-commit framework, see `Pre-commit website
236236
- "- **Run regular (fast) static checks:** `prek run --from-ref <target_branch> --stage pre-commit`"
237237
- "- **Run manual (slower) checks:** `prek run --from-ref <target_branch> --stage manual --skip compile-ui-assets-dev --skip view-skill-eval --skip run-skill-eval-codex` (the skipped hooks start long-running local servers or provision the opt-in Codex environment rather than run checks that complete)"
238238
- "- **Build docs:** `breeze build-docs`"
239+
- "- **List the local verification for your changes:** `breeze verify` (add `--full` for everything CI runs for the default matrix cell, `--json` for machine-readable output). It uses the same selective-checks logic as CI."
239240
- "- **Determine which tests to run based on changed files:** `breeze ci selective-check --commit-ref <commit_with_squashed_changes>`"
240241
.. AGENT-SKILL-END
241242

‎dev/breeze/doc/03_developer_tasks.rst‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -335,7 +335,8 @@ Each row says what kind of check it is, whether it runs on the host or needs Doc
335335
CI image (``breeze``), and the exact command. Jobs CI runs on every PR regardless of the change
336336
(breeze's own unit tests, the shared distributions) only show up with ``--full``. The translation
337337
check is never listed: CI runs it with ``|| true``, so it cannot fail a PR.
338-
Use ``--json`` for machine-readable output with the same fields.
338+
Use ``--json`` for machine-readable output with the same fields. The ``prek`` row uses
339+
``--from-ref`` by default, the scope AGENTS.md recommends; ``--full`` shows CI's ``--all-files`` form.
339340

340341
When a change touches CI tooling or dependency files, selective checks make CI run the full suite.
341342
The default list leaves that expansion out and only shows what the changed files match themselves,

‎dev/breeze/doc/ci/04_selective_checks.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,11 @@ contributors in case of simpler changes.
5050
> [`dev/breeze/src/airflow_breeze/utils/selective_checks.py`](../../src/airflow_breeze/utils/selective_checks.py).
5151
> When you change that file, **update this document in the same PR** so the behaviour and the
5252
> documentation stay in sync.
53+
>
54+
> `breeze verify` maps the `run_*` flags to local commands in
55+
> [`dev/breeze/src/airflow_breeze/utils/verification_plan.py`](../../src/airflow_breeze/utils/verification_plan.py).
56+
> Adding a `run_*` flag to `SelectiveChecks` also means classifying it there;
57+
> `dev/breeze/tests/test_verification_plan.py` fails until you do.
5358
5459
## Why selective checks exist (the optimisation goal)
5560

‎dev/breeze/src/airflow_breeze/utils/verification_plan.py‎

Lines changed: 22 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -84,11 +84,21 @@ class VerificationItem:
8484
)
8585

8686

87-
def build_prek_item(sc: SelectiveChecks, base_ref: str) -> VerificationItem:
87+
def build_prek_item(sc: SelectiveChecks, base_ref: str, *, full: bool) -> VerificationItem:
88+
from_ref = f"prek run --from-ref {shlex.quote(base_ref)} --to-ref HEAD"
8889
if sc.basic_checks_only:
89-
command = f"SKIP_BREEZE_PREK_HOOKS=true SKIP={sc.skip_prek_hooks} prek run --from-ref {shlex.quote(base_ref)} --to-ref HEAD"
90-
return VerificationItem("prek", command, "host")
91-
return VerificationItem("prek", f"SKIP={sc.skip_prek_hooks} prek run --all-files", "breeze")
90+
return VerificationItem(
91+
"prek", f"SKIP_BREEZE_PREK_HOOKS=true SKIP={sc.skip_prek_hooks} {from_ref}", "host"
92+
)
93+
# CI's static-checks job runs every file, because a non-basic change can affect hooks that
94+
# read files outside the diff. Locally the diff is the practical scope, as AGENTS.md advises.
95+
return VerificationItem(
96+
"prek",
97+
f"SKIP={sc.skip_prek_hooks} prek run --all-files"
98+
if full
99+
else f"SKIP={sc.skip_prek_hooks} {from_ref}",
100+
"breeze",
101+
)
92102

93103

94104
def build_unit_test_items(group: str, test_types_json: str | None) -> list[VerificationItem]:
@@ -125,7 +135,7 @@ def build_local_verification_plan(
125135
full_tests_needed: bool,
126136
full: bool = False,
127137
) -> dict[str, Any]:
128-
items = [build_prek_item(sc, base_ref)]
138+
items = [build_prek_item(sc, base_ref, full=full)]
129139
if sc.run_unit_tests:
130140
items += build_unit_test_items("core", sc.core_test_types_list_as_strings_in_json)
131141
if not sc.skip_providers_tests:
@@ -142,14 +152,14 @@ def build_local_verification_plan(
142152
breeze_tests = VerificationItem("unit", "cd dev/breeze && uv run --locked pytest", "host")
143153
if breeze_tests not in items:
144154
items.append(breeze_tests)
145-
dists = " ".join(sorted(json.loads(sc.shared_distributions_as_json)))
146-
items.append(
147-
VerificationItem(
148-
"unit",
149-
f"for d in {dists}; do (cd shared/$d && uv run --group dev pytest) || exit 1; done",
150-
"host",
155+
if dists := " ".join(sorted(json.loads(sc.shared_distributions_as_json))):
156+
items.append(
157+
VerificationItem(
158+
"unit",
159+
f"for d in {dists}; do (cd shared/$d && uv run --group dev pytest) || exit 1; done",
160+
"host",
161+
)
151162
)
152-
)
153163
return {
154164
"base_ref": base_ref,
155165
"default_python_version": sc.default_python_version,

‎dev/breeze/tests/test_verification_plan.py‎

Lines changed: 41 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818

1919
import inspect
2020
import json
21+
import re
2122
import shlex
2223
from functools import cached_property
2324
from unittest.mock import Mock
@@ -62,6 +63,7 @@ def _mock_selective_checks(**flags: object) -> Mock:
6263
setattr(sc, flag, False)
6364
sc.skip_providers_tests = True
6465
sc.skip_prek_hooks = "identity"
66+
sc.shared_distributions_as_json = "[]"
6567
sc.default_python_version = DEFAULT_PYTHON_MAJOR_MINOR_VERSION
6668
for flag, value in flags.items():
6769
setattr(sc, flag, value)
@@ -75,16 +77,27 @@ def test_each_flag_maps_to_its_commands(flag: str):
7577
)
7678
commands = [item["command"] for item in result["items"]]
7779
assert commands == [
78-
"SKIP=identity prek run --all-files",
80+
"SKIP=identity prek run --from-ref main --to-ref HEAD",
7981
*(command for _, command, _ in FLAG_COMMANDS[flag]),
8082
]
8183

8284

8385
@pytest.mark.parametrize(
84-
("basic_checks_only", "expected"),
86+
("basic_checks_only", "full", "expected"),
8587
[
86-
(False, ("prek", "SKIP=identity prek run --all-files", "breeze")),
88+
(False, False, ("prek", "SKIP=identity prek run --from-ref main --to-ref HEAD", "breeze")),
89+
(False, True, ("prek", "SKIP=identity prek run --all-files", "breeze")),
8790
(
91+
True,
92+
False,
93+
(
94+
"prek",
95+
"SKIP_BREEZE_PREK_HOOKS=true SKIP=identity prek run --from-ref main --to-ref HEAD",
96+
"host",
97+
),
98+
),
99+
(
100+
True,
88101
True,
89102
(
90103
"prek",
@@ -94,9 +107,15 @@ def test_each_flag_maps_to_its_commands(flag: str):
94107
),
95108
],
96109
)
97-
def test_prek_command_follows_basic_checks_only(basic_checks_only: bool, expected: tuple[str, str, str]):
110+
def test_prek_command_follows_basic_checks_only_and_full(
111+
basic_checks_only: bool, full: bool, expected: tuple[str, str, str]
112+
):
98113
result = build_local_verification_plan(
99-
_mock_selective_checks(basic_checks_only=basic_checks_only), (), "main", full_tests_needed=False
114+
_mock_selective_checks(basic_checks_only=basic_checks_only),
115+
(),
116+
"main",
117+
full_tests_needed=False,
118+
full=full,
100119
)
101120
prek = result["items"][0]
102121
assert (prek["kind"], prek["command"], prek["runs_in"]) == expected
@@ -107,7 +126,7 @@ def test_docs_only_change_mirrors_the_ci_cell():
107126
sc = _selective_checks(files)
108127
result = build_local_verification_plan(sc, files, "main", full_tests_needed=False)
109128
assert [item["command"] for item in result["items"]] == [
110-
f"SKIP={sc.skip_prek_hooks} prek run --all-files",
129+
f"SKIP={sc.skip_prek_hooks} prek run --from-ref main --to-ref HEAD",
111130
"breeze build-docs apache-airflow",
112131
]
113132

@@ -225,8 +244,9 @@ def test_full_plan_adds_the_jobs_ci_runs_on_every_pr():
225244
sc = _selective_checks(files)
226245
lean = build_local_verification_plan(sc, files, "main", full_tests_needed=False)["items"]
227246
full = build_local_verification_plan(sc, files, "main", full_tests_needed=False, full=True)["items"]
228-
assert [i["command"] for i in full] == [
229-
*(i["command"] for i in lean),
247+
assert full[0]["command"] == f"SKIP={sc.skip_prek_hooks} prek run --all-files"
248+
assert [i["command"] for i in full[1:]] == [
249+
*(i["command"] for i in lean[1:]),
230250
"cd dev/breeze && uv run --locked pytest",
231251
"for d in "
232252
+ " ".join(sorted(json.loads(sc.shared_distributions_as_json)))
@@ -240,3 +260,16 @@ def test_full_plan_does_not_duplicate_breeze_tests_for_a_breeze_change():
240260
_selective_checks(files), files, "main", full_tests_needed=True, full=True
241261
)
242262
assert [i["command"] for i in full["items"]].count("cd dev/breeze && uv run --locked pytest") == 1
263+
264+
265+
def test_manual_stage_hooks_in_the_mapping_are_the_ones_ci_runs():
266+
workflow = (AIRFLOW_ROOT_PATH / ".github" / "workflows" / "ci-amd.yml").read_text()
267+
hooks = [
268+
hook
269+
for commands in FLAG_COMMANDS.values()
270+
for _, command, _ in commands
271+
for hook in re.findall(r"prek run --stage manual (\S+)", command)
272+
]
273+
assert hooks
274+
for hook in hooks:
275+
assert f"--stage manual {hook} " in workflow, hook

‎dev/breeze/tests/test_verify_commands.py‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131

3232
@patch(
3333
"airflow_breeze.commands.verify_commands.get_changed_files_against",
34+
autospec=True,
3435
return_value=("airflow-core/docs/index.rst",),
3536
)
3637
def test_json_output_keeps_stdout_parseable(mock_files, monkeypatch):
@@ -55,6 +56,7 @@ def test_json_output_keeps_stdout_parseable(mock_files, monkeypatch):
5556

5657
@patch(
5758
"airflow_breeze.commands.verify_commands.run_command",
59+
autospec=True,
5860
return_value=CompletedProcess(
5961
args=[], returncode=128, stdout="", stderr="fatal: Not a valid object name"
6062
),
@@ -68,6 +70,7 @@ def test_unknown_base_ref_is_a_clean_usage_error(mock_run):
6870

6971
@patch(
7072
"airflow_breeze.commands.verify_commands.get_changed_files_against",
73+
autospec=True,
7174
return_value=("airflow-core/docs/index.rst",),
7275
)
7376
def test_selective_checks_narration_is_hidden_unless_verbose(mock_files):
@@ -85,7 +88,9 @@ def test_selective_checks_narration_is_hidden_unless_verbose(mock_files):
8588
],
8689
)
8790
def test_full_suite_expansion_is_explained(files: tuple[str, ...], expanded: bool):
88-
with patch("airflow_breeze.commands.verify_commands.get_changed_files_against", return_value=files):
91+
with patch(
92+
"airflow_breeze.commands.verify_commands.get_changed_files_against", autospec=True, return_value=files
93+
):
8994
result = CliRunner().invoke(verify, [], catch_exceptions=False)
9095
full = CliRunner().invoke(verify, ["--full"], catch_exceptions=False)
9196
assert result.exit_code == 0
@@ -95,7 +100,7 @@ def test_full_suite_expansion_is_explained(files: tuple[str, ...], expanded: boo
95100
assert "breeze testing core-tests" not in " ".join(result.output.split())
96101

97102

98-
@patch("airflow_breeze.commands.verify_commands.run_command")
103+
@patch("airflow_breeze.commands.verify_commands.run_command", autospec=True)
99104
def test_changed_files_list_renames_like_ci_diff_tree(mock_run):
100105
mock_run.side_effect = [
101106
CompletedProcess(args=[], returncode=0, stdout="abc123\n", stderr=""),
@@ -108,11 +113,12 @@ def test_changed_files_list_renames_like_ci_diff_tree(mock_run):
108113

109114
@patch(
110115
"airflow_breeze.commands.verify_commands.get_changed_files_against",
116+
autospec=True,
111117
return_value=("airflow-core/docs/index.rst",),
112118
)
113119
def test_long_commands_are_folded_not_truncated(mock_files):
114120
result = CliRunner().invoke(verify, [], catch_exceptions=False)
115121
assert result.exit_code == 0
116122
assert "\u2026" not in result.output
117123
compact = re.sub(r"[\u2502\s]", "", result.output)
118-
assert "update-uv-lockprekrun--all-files" in compact
124+
assert "prekrun--from-refmain--to-refHEAD" in compact

‎dev/skill-evals/last-eval-hash.txt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,3 @@
11
# Generated by dev/skill-evals/eval.py — do not edit or resolve conflicts by hand.
22
# Run `prek run run-skill-eval --hook-stage manual --all-files` to regenerate.
3-
485f916101818db48c8311bd3e96770695e8681411e9c063a1ff05bc1d051d7c
3+
feffd0248dc326fd6bf082528de786a9770481947aadba9382ef1f1e3f8d8ee9

0 commit comments

Comments
 (0)