build: make formatting enforcement actually work, and reformat the repo once

[ayeshurun]

Why

PR microsoft#265 arrived with large amounts of unexplained formatting churn. tests/test_utils/test_fab_ui.py alone shows 87+/58− in that PR, of which 83+/58− is reproduced by running black on the file at the merge base. Roughly 95% of that file's diff is formatting, not the Azure CLI auth feature it claims to add.

That is not the contributor's fault. Formatting enforcement in this repository has never worked. I found four independent defects, each verified experimentally.

1. The CI lint job was a no-op and could never fail

tox.toml ran black . — the mutating form, not black --check. Black reformats and exits 0. In CI it rewrote files inside the ephemeral runner, threw them away, and reported green. The lint job has never once failed on formatting.

2. Black was unpinned

deps = ["black"] floated to whatever was latest. Measured on the same tree:

black	files flagged
24.10.0	114
26.5.1	122

An 8-file swing from version drift alone. Two contributors on different black versions produce different output and each blames the other.

3. [tool.black] was in tox.toml, which black does not read

Black auto-discovers configuration from pyproject.toml only. Verified directly: setting line-length = 40 in tox.toml changes nothing; the same setting in pyproject.toml reformats immediately. The block was dead. It went unnoticed because the value it declared (88) happens to equal black's default — so it looked like it worked.

4. isort was VS Code-only

isort was configured solely through .vscode/settings.json isort.args. It was absent from tox.toml, from CI, and from requirements-dev.txt. Contributors not using VS Code — or using it without the extension — had no import sorting at all.

What this changes

Superseded in e7e58d2: the sections below describe .pre-commit-config.yaml, which was added in ae4be9d and removed again in the final commit. Those paragraphs are kept as the audit trail of how the design arrived here; see Final change — removing pre-commit for what is actually on the branch. Everything else still stands.

Eleven commits, deliberately ordered so the repository ends green and git blame survives.

ae4be9d — fix the configuration

• pyproject.toml — adds [tool.black] and [tool.isort]. This is the single source of truth for formatter settings; isort uses profile = "black" so the two cannot disagree. (Settings were only half the problem — the tools also disagreed about which files exist. See fabfaf1.)
  • known_first_party = ["fabric_cli"] and src_paths are load-bearing: tox uses skip_install = true and pre-commit runs in an isolated venv, so without them isort can classify fabric_cli as third-party and produce output that differs from CI.
  • The old exclude regex is dropped rather than ported. Setting exclude replaces black's defaults; black's DEFAULT_EXCLUDES is a strict superset of what was there, so dropping it is strictly safer. extend-exclude is the correct knob if exclusions are ever needed.
• tox.toml — [env.lint] becomes read-only (isort --check-only --diff, then black --check --diff) and pins both tools. A new [env.format] does the mutating pass, and is deliberately not in env_list so it never runs in CI. The dead [tool.black] block is deleted.
• .pre-commit-config.yaml (new) — catches drift before it reaches CI.
• requirements-dev.txt — pins black, isort, pre-commit at the same versions.
• .vscode/settings.json — removes isort.args (the second source of truth). Also scopes editor.defaultFormatter under "[python]": it was global, so VS Code was applying black to JSON, YAML and Markdown too.
• .github/workflows/fab-build.yml — lint job moves from Python 3.12 to 3.13, and paths: gains .ai-assets/** and .pre-commit-config.yaml.
• CONTRIBUTING.md / AGENTS.md — document the commands and the blame-ignore setup.

8e8472f — the one-time reformat

144 files, +1459/−1235. Run with the pinned versions and iterated to a fixed point; a second pass produces no changes.

67c9675 — .git-blame-ignore-revs

So the reformat does not destroy blame.

Reworked in 5cab38e. This repository is squash-merge only, so no commit SHA from inside a PR ever reaches main — the entry would have been dead on arrival. Worse, git silently ignores entries that do not resolve to a real commit, so it would have failed open: blame keeps pointing at the formatting commit and nothing warns you. The file now carries the mechanism and the procedure instead of a SHA, and CONTRIBUTING.md tells maintainers to append the squashed SHA after merge.

fc7b387 — unrelated one-line doc fix

AGENTS.md told contributors to run pytest tests/test_commands --playback. That flag does not exist — conftest.py only registers --record — so pytest exits immediately with unrecognized arguments. Anyone following the file could not run the integration suite. Playback is already the default. Happy to split this out if you would rather keep the PR pure.

fabfaf1 — close the file-selection gap (found in review)

The first three commits fixed configuration. Review found they did not fix file selection, which was a second, independent hole:

.gitignore excludes tests/test_commands/data/*. Black honours .gitignore when walking directories; isort does not; pre-commit sidesteps the question entirely by passing tracked filenames explicitly. The result was that black --check . reported 347 files would be left unchanged and exited 0 while silently skipping three tracked Python files, two of which were unformatted. Naming them explicitly exited 1.

So tox -e lint would have stayed green while pre-commit went red — the exact class of divergence this PR exists to eliminate.

• .gitignore re-includes *.py under that directory. Git cannot re-include a file whose parent directory is excluded, which conveniently leaves sample_code/ hidden.
• sample_code/ is excluded from both formatters on purpose: those fixtures are uploaded verbatim by tests and their bytes are recorded in VCR cassettes, so reformatting them would change recorded request bodies. Black needs force-exclude, not extend-exclude — only the former applies to explicitly-passed paths, which is how pre-commit invokes it.
• models.py and static_test_data.py are now formatted. Verified AST-identical.
• CI now runs pre-commit run --all-files alongside tox -e lint. They resolve file lists differently, so running both turns any future divergence into a CI failure. It also validates .pre-commit-config.yaml, which nothing in CI previously exercised. This is ~10s of deliberate redundancy; push back if you disagree.
• .vscode/settings.json sets importStrategy: fromEnvironment for both extensions. They default to their bundled black/isort, which update on the extensions' own schedule and drift from the pins in requirements-dev.txt.
• .pre-commit-config.yaml revs are now full commit SHAs, matching how upstream pins GitHub Actions. Tags are mutable.
• Corrected a comment claiming black's DEFAULT_EXCLUDES covers __pycache__. It does not — that is .gitignore doing the work. That inaccuracy is precisely what masked this bug.

Regression test: append a malformatted function to models.py; both black --check . and pre-commit run --all-files now fail. Before this commit, both passed.

Evidence that the reformat is safe

AST equivalence, all 144 files. Every file was parsed before and after and the trees compared after normalising for import order and docstring whitespace. Three classes of difference were found, all confirmed no-ops:

1. isort splitting, merging and reordering import aliases.
2. black stripping trailing whitespace inside docstrings and re-indenting them.
3. isort removing a duplicate import of set_item_metadata_success_params in tests/test_commands/test_set.py — it was listed twice in the same from-import. This is the only non-whitespace change in the entire commit.

Tests, identical before and after. Same invocation, same environment, run from a clean state on each of three revisions:

revision	result
ae4be9d (pre-reformat)	6 failed, 1622 passed, 2 skipped
fc7b387 (post-reformat)	6 failed, 1622 passed, 2 skipped
fabfaf1 (HEAD, incl. review fixes)	6 failed, 1622 passed, 2 skipped

pytest tests/test_commands tests/test_core tests/test_utils tests/test_parsers -q

Caveat, and it matters. These absolute numbers are not stable across invocations. This suite leaks global state, so the failure count depends on what ran before it in the same environment:

• Running the four directories in one process: 6 failures.
• Running test_cp.py and test_mv.py individually first, then the full suite: 19 failures on the exact same tree.
• Running test_cp.py alone: 4 failures. test_mv.py alone: 4 failures and 1 error.
• An independent reviewer running this branch in a different environment reported a full pass.

So do not read "6 failures" as a property of the code. The load-bearing claim is the comparison: under an identical invocation from an identical starting state, the three revisions produce identical results. The reformat and the review fixes change nothing.

The underlying cross-test pollution is a real pre-existing problem, but it is out of scope here.

5cab38e — second review iteration

• isort ignored its own skip globs for explicitly-passed files. extend_skip_glob is consulted only while walking directories. tox -e format -- tests/test_commands/data/sample_code/spark_job_simple.py therefore rewrote a VCR payload fixture whose exact bytes are compared against a recorded cassette. Confirmed on a clean tree before and after. Fixed by passing --filter-files in both tox environments — pre-commit's official isort hook already injects it, which is why pre-commit was never affected. The comment in pyproject.toml asserting that isort needed no "force" variant was wrong and is corrected.
• isort and black disagreed about .gitignore. black honours it during traversal; isort did not. The test suite drops generated .py files into tests/test_commands/data/, so running the tests and then tox -e lint produced an isort failure black never reported. skip_gitignore = true aligns them. Verified the tracked, negated files in that directory are still checked by isort, black and pre-commit.

d997a21 — third review iteration

• The branch referenced GitHub Actions by tag while claiming it did not. Upstream pinned every action to a full-length SHA in chore: Pin GitHub Actions to full-length commit SHAs microsoft/fabric-cli#271; this branch is based on a commit that predates that, so the lint job still used checkout@v3, setup-python@v4 and cache@v3. Two consequences: the comment in .pre-commit-config.yaml justifying its SHA pins as "matching how this repo pins GitHub Actions" was false for this branch, and the pre-commit cache step I added was a new actions/cache@v3 reference — a supply-chain regression against upstream. The lint job's actions are now pinned to the SHAs copied verbatim from upstream/main, so those lines merge cleanly on rebase. Jobs this PR does not otherwise touch are deliberately left alone rather than pinned wholesale. This also removes a latent problem: the job asks setup-python@v4 for Python 3.13, which postdates v4.
• Nothing verified the three formatter pins agreed. The black/isort versions are declared in tox.toml, requirements-dev.txt and .pre-commit-config.yaml. Running both tools in CI only catches drift once two versions format this codebase differently, which can lag a release by months. scripts/check_formatter_pins.py compares the declared pins directly. Correction: this bullet originally claimed the script "cannot silently pass". That was false. The version committed here counted occurrences of a pin and rejected only two cases — zero matches anywhere, and disagreement between the matches it did find. 5fdb8ac below documents the four ways it could be bypassed, and rewrites it.
• CONTRIBUTING.md claimed the lint job "is a required check". Branch protection is repository administration state, not something this tree can assert — and I had already disproved it myself while investigating the paths: filter (see below). Reworded to describe what CI does, not what the repo is configured to require.
• skip_gitignore has an undocumented dependency on the git binary. isort shells out to git; black parses .gitignore directly. In a tree with no .git (an sdist, or a container built without the checkout) isort prints fatal: not a git repository per file and stops honouring .gitignore. Verified: in a git archive extract with a stray untracked artefact, isort exits 1 while black exits 0. This fails in the safe direction — a spurious failure, never missed drift — and does not affect a real checkout, so it is kept, but the caveat is now recorded. The obvious alternative, an extend_skip_glob over the data directory, does not work: it would also skip models.py and static_test_data.py, which are tracked and must stay checked.
• Bounded the pre-commit CI step with timeout-minutes, added scripts/** to the workflow path filter, and trimmed the 21-line banner in .git-blame-ignore-revs that duplicated CONTRIBUTING.md.

5fdb8ac — fourth review iteration

• The pin check could silently pass. Four bypasses were reproduced on disk against the committed script, each exiting 0: loosening black to >= in [env.lint] while [env.format] kept == (the surviving == was self-consistent, so the check was satisfied); replacing the pre-commit rev: SHA but leaving # frozen: 26.5.1 stale (the version was only ever read from the annotation); setting rev: to a mutable tag rather than a SHA; and adding a second psf/black block at a different rev, which re.search never saw. The root cause was that the script counted occurrences instead of validating structure, so it could not detect a pin disappearing from one of two places it is required to appear. My own earlier tests missed all four because they mutated every occurrence of a pin — a different failure mode from a partial edit that leaves one consistent match behind. The script now parses requirement specifiers (extras, whitespace, environment markers), normalises names per PEP 503, requires both tox environments to declare an exact pin, rejects any operator other than ==, asserts exactly one matching pre-commit repo block, and requires a 40-hex SHA. Re-verified with a harness that asserts each mutation actually changed the file bytes before running the check — a guard added because an earlier harness silently applied no mutation at all and reported false passes. Baseline exits 0; 13 failure modes are caught; a legitimate black[jupyter]==26.5.1 with a matching version is still accepted, since the contract is version agreement, not extras equality. One limitation is now documented rather than left implied: whether a rev SHA really is the tagged release cannot be checked offline.
• The paths: filter had the same shape of hole. isort's config search order is ('.isort.cfg', 'pyproject.toml', 'setup.cfg', 'tox.ini', '.editorconfig') and stops at the first hit, so a PR adding a root .isort.cfg would redefine import sorting while skipping the job that enforces it. None of those files were listed. Rather than enumerate directories a fourth time — .ai-assets/, .gitignore and scripts/** were each added retroactively after being missed — the filter now matches Python by extension and lists the alternate config files explicitly.
• .git-blame-ignore-revs contained a contradiction. It requires listed commits to be purely mechanical, but this repository squash-merges, which collapses a PR's commits into one. A PR mixing the reformat with configuration changes therefore produces a squash commit that is not purely mechanical and must not be listed — leaving the file permanently empty and the stated benefit unrealised. Three prior review iterations missed this. The file and CONTRIBUTING.md now state that the reformat has to land as its own pull request for the entry to be valid. This is a merge-strategy decision for whoever lands the work, not something the branch can fix by itself — see below.
• Measured the objection that running the format check in both tox and pre-commit doubles CI time: tox 3.26s vs pre-commit 1.34s cached. It does not.

Things I did not resolve

• The paths: filter. A reviewer called this a branch-protection deadlock: a PR touching only unlisted paths would leave a required lint check pending forever. I could not reproduce that. Upstream PR chore(release): revert changes to previous release notes microsoft/fabric-cli#274 changed only docs/release-notes.md, never triggered fab:build, and merged — the required checks (license/cla, changelog, PR title) all run unconditionally. The filter is also pre-existing upstream, not something I introduced. The genuine gap was narrower: .gitignore decides which files black even sees, so a .gitignore-only change could alter lint scope without lint running. That path is now listed. Dropping the filter entirely remains a repo-policy call.
• This branch is based on ayeshurun/fabric-cli@main, which is 7 commits behind and 2 ahead of microsoft/fabric-cli@main. I deliberately did not rebase onto upstream/main: this PR targets the fork's main, which carries two fork-only commits, and rebasing would make the diff show those as deletions. The cost is that the branch does not inherit upstream's SHA-pinning commit (chore: Pin GitHub Actions to full-length commit SHAs microsoft/fabric-cli#271), which is why the pinning above had to be applied by hand to the job this PR touches. A 144-file reformat on a stale base will conflict badly. If any of this is wanted upstream, commits 1, 3 and 4 should be cherry-picked and the reformat re-run fresh on current main — do not port the reformat diff itself.
• Ordering matters if this is ever landed alongside other work. Every in-flight PR will conflict with the reformat. The reformat should land at a quiet moment, and open PRs should be rebased and re-run through tox -e format rather than resolved by hand.
• Whether to split this PR. The blame-ignore entry is only valid if the reformat lands as a PR containing nothing but the reformat. As one squash-merged PR, .git-blame-ignore-revs stays empty and git blame permanently attributes 144 files' worth of lines to the reformat. Landing it as two PRs — configuration first, then the reformat alone — costs one extra round trip and is the only way to get the blame benefit. Documented rather than decided unilaterally.
• .github/instructions/test.instructions.md carries the same bogus --playback flag plus a paragraph describing it as an intentional convention. Left alone — correcting it means rewriting stated design intent.

For reviewers

git config blame.ignoreRevsFile .git-blame-ignore-revs   # then read commit 2 as noise
git show ae4be9d 67c9675 fc7b387 fabfaf1 64a8a7b 5cab38e d997a21 5fdb8ac 0c5390e e7e58d2  # the review surface
tox -e lint                                                        # should pass (350 files)
python scripts/check_formatter_pins.py                             # pins agree
git ls-files -c -i --exclude-standard -- '*.py' '*.pyi'            # only sample_code/ may appear

---

Review iteration 5 (final) — 0c5390e

The pin check was both too strict and too permissive. All six of the reviewer's
script findings reproduced on disk.

It rejected valid configuration: a trailing comment on a requirements line
(black==26.5.1 # formatter), an indented deps = [ key, single-quoted TOML
strings, and a comment or blank line between - repo: and rev: are all legal
and all failed the check.

It accepted broken configuration: a loose duplicate (black>=24.0.0) or a
direct URL reference alongside a good pin was skipped rather than rejected, and
renaming id: black disabled the formatter entirely while the check still exited
0 — precisely the failure mode this PR exists to prevent.

Two prior attempts to harden the regexes failed the same way, so parsing now uses
real parsers: tomllib for tox.toml, yaml.safe_load for
.pre-commit-config.yaml. The version lives in the # frozen: comment, which YAML
discards, so it is read from the line carrying the rev the parser returned. PyYAML
is already a hard project dependency; tomli is added for Python 3.10. The
workflow now installs pyyaml explicitly instead of relying on it arriving via
pre-commit.

Also adds MANIFEST.in to paths: — it controls sdist contents, so a
MANIFEST.in-only change should not skip the build.

Verified with a 25-case suite on fresh git archive extracts, every mutation
guarded by an assert that the file bytes actually changed: 8 valid variations
exit 0, 17 broken ones exit 1.

Four iteration-5 claims were checked and are false, and no change was made for
them:

Claim	Reality
.git-blame-ignore-revs hardcodes 8e8472f8…	No SHA present; removed in 5cab38e
5fdb8ac says "squash merging exclusively…"	That text does not appear in the commit — fabricated quote
tests/** missing from paths:	Present, line 9
[env.format] lacks --filter-files	Present in both tox environments

Useful negative results from this iteration: **/*.py does not match
.ai-assets/foo.py (GitHub path filters use minimatch without {dot: true}), so
the explicit .ai-assets/** entry is load-bearing; and tox and pre-commit resolve
to the identical 350-file set.

The review loop is complete at 5 of 5 iterations.

---

Final change — removing pre-commit

e7e58d2, in response to the question "can we do the enforcement without adding
the pre-commit?" The answer is yes, and on the evidence from five review
iterations that is the better trade.

Why. tox -e lint in CI is what actually blocks a merge. pre-commit was a
shift-left convenience layered on top of it, and its cost was concentrated
rather than spread out: a third place to declare black/isort versions, a
# frozen: comment that could silently drift from the tag beside it, a pyyaml
dependency in CI, two extra workflow steps, and 55 of the pin checker's 317
lines. Every defect iteration 5 found — the renamed hook id, the stale
# frozen: version, the mutable tag, the duplicate repo block — lived in that
layer. None of them existed in the tox path.

It also created two independent definitions of "the set of files we format":
black's directory walk, which honours .gitignore, and pre-commit's explicit
list of tracked files, which does not. They agree at 350 files today. Keeping
them in agreement needed its own CI step.

The one thing pre-commit was really protecting. Removing it would have
silently re-opened the original bug. black skips gitignored paths while walking
directories, so a file that is tracked and gitignored is never checked and
tox -e lint still reports success — that is exactly how models.py and
static_test_data.py shipped unformatted. pre-commit run --all-files covered
that hole incidentally, by passing tracked filenames explicitly.

Rather than keep a tool for its side effect, the workflow now asks git directly:

git ls-files -c -i --exclude-standard -- '*.py' '*.pyi'

This is stronger than the cross-check it replaces: it names the failing files
instead of inferring the problem from a formatting diff, needs no black, no pins
and no second config file, and fails closed if git errors. sample_code/ is
excluded by prefix — those fixtures are byte-compared against VCR cassettes and
are already force-excluded in pyproject.toml — so adding a legitimate fixture
will not break CI while a genuinely hidden module still will. That one path is
now duplicated between pyproject.toml and the workflow; the comment says so.

Verified, running the step's shell extracted verbatim from the YAML:

Case	Expected	Result
Current tree	exit 0	exit 0
.gitignore negations removed	exit 1, names both files	exit 1, named models.py and static_test_data.py
New tracked .py hidden in an unrelated directory	exit 1	exit 1, named it
Run outside a git repository	non-zero (fail closed)	exit 128

Knock-on changes. check_formatter_pins.py drops to two files, 317 → 242
lines, and no longer imports yaml, so the explicit pip install pyyaml is
gone. Re-validated on fresh extracts against 21 adversarial cases — 7 valid
variations exit 0, 14 broken ones exit 1 — including a lint-vs-format
divergence inside tox.toml, which the error message now names precisely.
force-exclude stays in pyproject.toml, but the comment now gives the reason
that survives: it is what makes exclusions apply to explicitly-passed paths,
which is how editor format-on-save invokes black. CONTRIBUTING.md points
contributors at format-on-save and states plainly that there is no committed git
hook.

What is genuinely lost. Unformatted code can now reach a push and fail in
CI rather than being caught locally at commit time. That is a real regression in
feedback latency, and it is the deliberate trade: .vscode/settings.json
already gives format-on-save with importStrategy: fromEnvironment for both
tools, so anyone using the repo's recommended setup still gets local
correction — and tox -e format is one command away for anyone who is not.

If you would rather keep the hook, say so and I will restore it — but restoring
it unpinned and unchecked is worse than either extreme, because a stale local
black then fights CI in a loop.