ci: scope format presubmit to changed files (#2232)

[sanjayrohith]

Fixes #2232

Problem

scripts/fix_format.sh runs Prettier, Pyink, dart format, swift-format, and ktfmt across the entire workspace on every PR. A contributor touching only Swift can be blocked by unrelated Dart formatting drift, as happened in this run. Their choices are to pull unrelated formatting churn into their PR or to wait for someone else to fix it. As the monorepo grows this gets worse.

Approach

The obvious fix — adding paths: filters to the presubmit-lint workflow — is a trap. format-check is a required check, and a required check that is filtered out never reports a conclusion, so PRs that skip it sit pending forever. So the job still always runs; it narrows its own work instead.

fix_format.sh gains a --changed mode that resolves the changed set from git and hands each formatter only its own files, skipping formatters whose languages the branch never touched. Default behaviour (whole repo) is unchanged, and post-submit on main still sweeps everything, so scoping can never let drift accumulate.

Changes

scripts/fix_format.sh

• New flags: --changed, --base <ref>, --plan, --help. Existing invocations (fix_format.sh, fix_format.sh --check) behave exactly as before.
• Changed-set discovery unions committed, staged, unstaged, and untracked files, and drops deletions. It diffs against the merge base, not the branch tip, so commits that landed on main after the branch point are not dragged in.
• Per-language selection mirrors each formatter's existing whole-repo scope, so --changed can never widen what a formatter touches:
  • Dart is still limited to samples/client/flutter and renderers/flutter.
  • Swift is still limited to Package.swift and swift/.
  • Python filters out /generated/ explicitly, because pyink's extend-exclude in pyproject.toml applies to directory walks but not to explicitly listed files. Without this, scoped runs would have formatted generated code that whole-repo runs skip.
  • ktfmt is a Gradle task rather than a file-level binary, so changed Kotlin sources are mapped to their owning Gradle module and only those modules run.
• --plan prints which formatters would run and over how many files, then exits. It needs no language toolchains.

.github/workflows/presubmit-lint.yml

• format-check computes its scope from github.event.pull_request.base.sha on PRs, and stays in whole-repo mode for pushes to main.
• The scope step drives toolchain installation off --plan output: Node, Python, and Dart setup are each skipped when that formatter has no work. A Swift-only PR now installs none of the three.
• fetch-depth: 0 so the merge base is reachable.
• The new unit tests run in the static-checks job.

scripts/test_fix_format.py — 21 tests covering the scoping logic.

CONTRIBUTING.md — documents --changed, --base, and --plan.

Verification

The issue's exact scenario, run against this repo with a single added Swift file:

$ ./scripts/fix_format.sh --plan --base upstream/main
--- formatting plan ---
mode: changed (base: upstream/main)
prettier: 0 file(s)
pyink: 0 file(s)
dart: 0 file(s)
swift: 1 file(s)
ktfmt: 0 module(s)

Feeding that through the workflow's gating logic:

prettier=false (SKIP setup)
pyink=false   (SKIP setup)
dart=false    (SKIP setup)

Dart formatting can no longer block a Swift-only PR, and the job stops installing three toolchains it will not use.

Scoped mode on this PR's own diff:

$ ./scripts/fix_format.sh --check --changed --base upstream/main
Running Prettier formatting for Node/Web assets...   All matched files use Prettier code style!
Running Pyink for Python files...                    1 file would be left unchanged.
Skipping Dart format: no matching files changed.
Skipping swift-format: no matching files changed.
Skipping ktfmt: no matching files changed.
Done.                                                # exit 0

Correct failure behaviour — a deliberately malformed TS file is caught, and only that file is checked:

[warn] renderers/web_core/src/__scope_probe.ts
[warn] Code style issues found in the above file. Run Prettier with --write to fix.
# exit 1

Backwards compatibility — the default whole-repo path is unchanged and still passes:

$ ./scripts/fix_format.sh --check
Running Prettier formatting for Node/Web assets...  All matched files use Prettier code style!
Running Pyink for Python files...                   320 files would be left unchanged.

Unit tests:

$ python3 -m unittest scripts.test_fix_format
Ran 21 tests in 0.7s
OK

They cover: Swift-only / TypeScript-only / Dart-only isolation, Package.swift at the root, generated-Python exclusion, Dart outside the two formatted roots, Kotlin→Gradle-module mapping (including modules without ktfmt), untracked files, deleted files, unioning committed and working-tree changes, merge-base isolation from unrelated main commits, whole-repo mode, and the two error paths.

Notes for reviewers

• Prettier file selection is an extension allowlist (PRETTIER_EXTENSIONS). Handing Prettier the raw changed set with --ignore-unknown was simpler, but then any changed file at all — a .sh, a .png — marks Prettier as needed and forces a Node install. The allowlist covers Prettier's built-in parsers; a Prettier plugin adding a new language would need a line here. The whole-repo post-submit run is the backstop if that is ever missed.
• mapfile is deliberately avoided so the script keeps running on the bash 3.2 that ships with macOS.
• Out of scope, happy to follow up: workspace-lint still runs yarn build:all + lint:all for every PR, and static-checks still checks license headers repo-wide. Both have the same "unrelated work blocks your PR" shape, but scoping them means reasoning about the Yarn workspace/wireit graph, which felt like a separate change rather than something to bundle here.

[gemini-code-assist]

Code Review

This pull request updates the fix_format.sh script to support formatting only changed files via the --changed and --base flags, and adds a --plan option to preview which formatters would run. It also includes a new Python test suite (test_fix_format.py) to verify this scoping logic and updates the contribution guidelines. The review feedback suggests optimizing the changed-file discovery by removing redundant git diff commands and improving robustness by replacing echo with printf when printing file paths.

[gemini-code-assist]

The second and third git diff commands are redundant. git diff ... "$diff_point" compares the working tree against the merge base, which already includes all committed changes on the branch, staged changes in the index, and unstaged changes in the working tree. Removing the redundant git diff calls improves performance by avoiding unnecessary Git operations.

Suggested change

	git diff --name-only --diff-filter=ACMRT "$diff_point" --
	git diff --name-only --diff-filter=ACMRT --
	git diff --cached --name-only --diff-filter=ACMRT --
	git diff --name-only --diff-filter=ACMRT "$diff_point" --

[gemini-code-assist]

Using echo to print arbitrary variables (like file paths) can lead to unexpected behavior if the path starts with a hyphen (e.g., -n or -e), as echo may interpret it as an option. It is safer and more robust to use printf '%s\n' instead.

Suggested change

	[[ -n "$path" && -f "$path" ]] && echo "$path"
	[[ -n "$path" && -f "$path" ]] && printf '%s\n' "$path"

[sanjayrohith]

Both applied, thanks.

Redundant git diff calls — agreed. git diff <merge_base> compares the merge base against the working tree, so it already covers committed, staged, and unstaged changes; the --cached and bare git diff calls added nothing. I kept git ls-files --others alongside it, since untracked files don't appear in that diff. The existing tests (including the committed-plus-dirty case) still pass, and I added one more that pins the staged-file case explicitly — that's the behaviour that would have silently regressed if the reasoning had been wrong.

echo → printf — applied. For accuracy though: the practical impact is nil here, since echo only misreads a path named exactly -n/-e/-E, and no formatter's extension filter would select such a path anyway. Correct habit regardless, and free.

Also in this push, unrelated to the review: the zizmor template-injection failure at presubmit-lint.yml:122. ${{ steps.scope.outputs.args }} was being interpolated into a run: body; it and the two github.* expressions now go through env: instead. Checked with zizmor 1.25.2 locally — reproduces on the previous revision, clean on this one.

[polina-c]

I am rejecting your PR - our policy is to have all files formatted in a right way.

If formatting rules change upstream, someone should reformat all the files.