fix(fmt): the formatter gates walked the working tree, not the repo (cli#549)

[saadqbal]

Closes#549.

The bug

make fmt-check — a prerequisite of make check, the documented pre-push tier — ran both formatters over ., and . is the whole working tree, not the repo. Any untracked directory holding Go files was reported as drift while every tracked file was correctly formatted:

==> goimports (import grouping) needed on:
<untracked-scratch-dir>/internal/cli/data.go
==> run `make fmt` to fix

CI never saw it, because a fresh checkout has no untracked Go files. So this was a local-only false failure, which is the kind that teaches people to stop reading make check output. And the remedy it printed was the same bug in write mode: make fmt (gofmt -s -w .) rewrote files the repo does not track — in the case that surfaced this, another session's working copy.

Worth noting because it explains why only this gate broke: Go's ./... pattern skips dot-prefixed directories, so vet, test and deadcode never saw the scratch directory at all. gofmt . does not skip them.

The fix

Both invocations — check and write — take their file list from git ls-files '*.go': exactly the set a PR can contain.

The logic moved to scripts/format.sh rather than growing more backslash-continued shell in the Makefile, because the edge cases want comments and a place to be tested:

• xargs over a NUL stream from a printf builtin, not a bare argv expansion. Measured: 9000 tracked files is 2.0 MB of argv against a 1 MB ARG_MAX — the naive form dies with argument list too long, the batched form reports all 9000.
• tracked-but-deleted index entries are filtered out.git ls-files reports the index, so a mid-edit deletion would otherwise hard-error the gate on a tree that is merely half-finished.
• fails closed (exit 2) outside a git work tree, and on an empty file list. The empty case is not cosmetic: bare gofmt -l with no path arguments reads stdin, so an unguarded empty list checks nothing and exits 0 — the inert-verification class of backend#1729.
• stderr is deliberately not captured — go run's module-download progress would otherwise show up as phantom drift filenames on a cold cache.

build.yml's Lint job now calls make fmt-check instead of keeping its own inline copy, so the file set has one definition and local cannot drift from CI — the invariant the Makefile header exists to protect. That also drops the restated goimports@v0.48.0 pin, and GOIMPORTS_VERSION joins check-tool-pins.sh's TOOLS so it stays dropped on the next bump. (It was previously uncovered by that guard, which is why the second copy went unnoticed.)

Verification

Run on this branch with an untracked .claude/worktrees/… directory present containing deliberately misformatted Go:

check	result
make check with the scratch dir present	green — ==> fmt-check: 219 tracked Go file(s) clean
the old gofmt -s -l . on the same tree	flags the scratch file (the bug, reproduced)
tracked gofmt drift (non-simplified slice expr in internal/slug/slug.go)	still fails, exit 1
tracked goimports drift (mis-grouped import, same file)	still fails, exit 1
make fmt with the scratch dir present	scratch file byte-identical (sha unchanged)
fail-closed: no work tree / empty list / bad usage	exit 2 each, with a reason
deleted tracked file	passes at 218/219 rather than erroring
large list: 9000 files, 2.0 MB argv	all 9000 reported (naive form: argument list too long)
shellcheck --shell=bash scripts/format.sh (default severity, not just -error)	clean
actionlint .github/workflows/build.yml	clean

scripts/format.sh is added to the Installer (shell) job's shellcheck step, so that stays true.

Note

One thing this does not change: an untracked non-dot directory containing a Go package still fails go vet ./..., because Go legitimately treats it as part of the module. That is the toolchain's own view of the module rather than a formatter scoping bug, so it is out of scope here.

Context

Found while working on #548; kept out of that PR to keep it scoped to the offboard telemetry bug (backend#2314).

---

Note

Low Risk
Dev-tooling and CI formatter gates only; no runtime, auth, or data-path changes. The main risk is a false-green fmt check if the new script’s fail-closed paths regress, which the added harness is meant to catch.

Overview
make fmt / make fmt-check no longer walk . (the whole working tree). They now format only tracked *.go files, so untracked scratch dirs no longer fail make check locally or get rewritten by make fmt.

Logic lives in scripts/format.sh (check + write). It fails closed outside a git work tree or on an empty file list, batches via xargs, and does not swallow formatter failures through command substitution. Lint CI now runs make fmt-check instead of an inline copy, and GOIMPORTS_VERSION is enforced by check-tool-pins.sh.

A hermetic fmt-selftest (scripts/tests/format-verify.sh) is wired into make check/ci and the installer job so a formatter that never ran cannot report clean.

^{Reviewed by Cursor Bugbot for commit a53d66b. Bugbot is set up for automated code reviews on this repo. Configure here.}

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit bf70404. Configure here.}

[LukasWodka]

Verified against develop and the branch. This is the rare fix that closes the class rather than the instance, and three of the four things I checked were sharper than the description lets on.

The bug, reproduced. On develop all four invocations walk . — check and write:

Makefile:261 gofmt -s -w .
Makefile:262 goimports … -w .
Makefile:268 gofmt -s -l .
Makefile:275 goimports … -l .

So the remedy the gate printed really was the same bug in write mode. Your point that ./... skips dot-prefixed directories — which is why vet/test/deadcode never saw the scratch dir and only this gate broke — is correct and is the detail that makes the diagnosis complete rather than plausible.

My independent count matches yours exactly: 219 tracked .go files. I'd started to question the 9000 files / 2.0 MB argv justification, since this repo's real expansion is 6,813 bytes against a 1,048,576 ARG_MAX — 0.6% of the limit. Then I read your verification table properly: you list "219 tracked" and "large list: 9000 files" as separate rows, so the 9000 is an explicit stress case and not a claim about cli. My objection was unfounded and the table is why. Worth keeping that separation — it's the difference between a measurement and a projection.

The stdin trap is real, and it's the best thing in here.

$ echo "" | gofmt -l
$ echo $?
0

Bare gofmt -l with no path arguments reads stdin, so an unguarded empty file list checks nothing and exits 0 — a gate that reports success while verifying zero files. Guarding it with exit 2 rather than letting it pass is precisely the inert-verification class, and citing backend#1729 for it is the right frame.

And the part I'd have missed: you fixed the guard that failed to catch the duplicate. On develop:

• build.yml:157 kept its own inline gofmt -s -l .
• build.yml:170 kept a hardcodedgoimports@v0.48.0
• scripts/check-tool-pins.sh's TOOLS contained onlyGOVULNCHECK_VERSION

So GOIMPORTS_VERSION was genuinely uncovered, which is why a second restated pin sat there unnoticed. On this branch build.yml calls make fmt-check (one definition of the file set, so local cannot drift from CI) andGOIMPORTS_VERSION joins TOOLS. Fixing the drift and then extending the check that would have prevented it is the whole difference between a fix and a lesson.

Two smaller things, both right:

• tracked-but-deleted index entries filtered — git ls-files reports the index, so a mid-edit deletion would hard-error a gate on a merely half-finished tree. Passing at 218/219 rather than erroring is the correct call.
• stderr deliberately not captured, because go run's module-download progress would surface as phantom drift filenames on a cold cache. That's the kind of thing that gets "fixed" later by someone adding 2>&1 and reintroducing ghost failures — good that the reason is written down.

And the scope note is honest: an untracked non-dot directory containing a Go package still fails go vet ./..., because that's Go's own view of the module rather than a formatter scoping bug. Naming what this doesn't fix is what stops the next person assuming it did.

Small connection: this is the follow-through on the aside you left me on cli#547 — the make check fmt-check noise from a stale nested worktree. Good that it became a ticket and a fix rather than a footnote.

Not approving yet — CI is pending on this head. I'll approve next pass once green; nothing here needs anything from you.

[LukasWodka]

Requesting changes. Bugbot's High on scripts/format.sh:90 is real — I didn't take it on trust, I reproduced the mechanism. And it's the same class this PR exists to fix, in the PR's own new code.

The defect, verified three ways

1 · The flags.scripts/format.sh:38 is set -uo pipefail — no -e.

2 · The asymmetry between the two modes. Write mode calls it directly:

run_formatter "gofmt -s -w" gofmt -s -w >/dev/null # exit 2 kills the script ✓

Check mode captures it:

drift="$(run_formatter "gofmt -s" gofmt -s -l)"# exit 2 kills only the subshell ✗

3 · A minimal repro of exactly that shape:

set -uo pipefail
f() { echo"boom">&2;exit 2; }
out="$(f)"echo"PARENT STILL RUNNING; out='${out}'"exit 0

boom
PARENT STILL RUNNING; out=''
parent exit=0

So on a formatter failure — cold go run cache, network blip, a bad GOIMPORTS_VERSION pin, or a non-zero gofmt — drift comes back empty, [ -n "$drift" ] is false, fail stays 0, and the script prints

==> fmt-check: 219 tracked Go file(s) clean

and exits 0. The FAILED (exit N) line goes to stderr and is true, but nothing acts on it. Write mode is unaffected; only check mode — the gate — is.

Why I'm blocking rather than noting it

This is worse than the bug being fixed. The old gofmt -s -l . at least ran and its exit status reached the recipe. The new form can report a clean tree having verified nothing — and build.yml's Lint job now calls make fmt-check, so the false green reaches CI, not just local. That is precisely the inert-verification class you cite backend#1729 for, and the empty-list guard three lines up exists because you were already thinking about it:

bare gofmt -l with no path arguments reads stdin, so an unguarded empty list checks nothing and exits 0 — the inert-verification class of backend#1729

Same sentence, same file, different path.

What would fix it

Any shape that keeps the failure status in the parent. Cheapest is to stop swallowing it in a command substitution — capture to a temp file and check rc in the caller's own shell:

run_formatter() { # sets DRIFT, returns 0 on successlocal label="$1";shift
DRIFT="$(printf '%s\0'"${files[@]}"| xargs -0 "$@")"|| {
echo"==> ${label}: FAILED — see the output above">&2return 2
}
}
run_formatter "gofmt -s" gofmt -s -l ||exit 2

Whatever the shape, the thing worth pinning afterwards is the failure path, not just the drift path: a case where the formatter itself exits non-zero and fmt-check must exit non-zero too. Your verification table covers empty list, no work tree, bad usage, deleted file and the 9000-file stress — all good — but there's no row for "the formatter died." That's the row that would have caught this, which is the more useful way to see it than as a Bugbot finding.

Unchanged from my earlier read

Everything else stands and I'd approve it once this is fixed: the .-vs-repo diagnosis is right (verified all four invocations on develop), the ./...-skips-dot-dirs explanation is correct and completes it, the 219-file count matches my own, collapsing build.yml to make fmt-check removes the second definition, and adding GOIMPORTS_VERSION to check-tool-pins' TOOLS closes the coverage gap that let the duplicate pin hide. The scope note about untracked non-dot directories is honest.

Not resolving Bugbot's thread — it's theirs and it's correct.

[saadqbal]

You and Bugbot are both right, and I reproduced the mechanism before touching anything rather than taking either on trust. Fixed in eb24527.

The defect.run_formatter ended in exit 2. Write mode called it directly so that killed the script; check mode captured it in $( ), where an exit ends only the subshell. Your three-way verification matches what I get:

f() { echo boom >&2; exit 2; }
out="$(f)"; echo "PARENT STILL RUNNING; out=[${out}]"
-> boom / PARENT STILL RUNNING; out=[] / parent exit=0

So drift came back empty, fail stayed 0, and ==> fmt-check: 219 tracked Go file(s) clean printed with exit 0 on a formatter that never ran. In a script whose header claims to fail closed against exactly that, on the same PR that cites backend#1729. Yours is the better catch of the two — Bugbot named the line, you named why write mode was unaffected, which is what made the shape obvious rather than the symptom.

The fix.run_formatter writes the formatter's stdout to a temp file and returns the status; callers are run_formatter … || exit 2 in the parent shell. It never exits and is never wrapped in a command substitution — with a comment saying so, because the next person to "tidy" this back into a capture reintroduces the bug silently.

But a comment cannot hold that shut, so scripts/tests/format-verify.sh does. Eight properties, formatters stubbed on PATH and via $GO, hermetic, no network, ~6 s: the failure propagates in both modes, the untracked file never reaches a formatter while the tracked one does, tracked drift still exits 1, and the three fail-closed paths. It is wired into make check, make ci, and the Installer (shell) job — that job rather than Lint, since stubbed formatters need no Go toolchain. make check is 10.6 s warm with it, against the 60 s budget.

Proof it is not inert. Against a copy of format.sh with the old subshell shape restored:

FAIL: a failing formatter exited 0 (check mode swallowed it — the #550 defect)
format-verify: 1 FAILED, 7 passed

Against the fix: format-verify: 8 properties hold.

One admission worth recording: writing that harness cost me one round of the identical trap — rc="$(run_case …)" discarded the CASE_OUT the function had set, and case 3 failed for that reason rather than a real one. Call sites are direct now with a comment naming the mechanism. Two instances of one bug class in one sitting is a decent argument for the harness existing.

On the 9000 files / 2.0 MB row — your reading is right, it is a stress case, not a claim about this repo. cli's real expansion is ~7 KB against a 1 MB ARG_MAX, i.e. xargs buys nothing today and is there so the gate does not acquire a size ceiling later. Happy to say that explicitly in the script comment if you would rather it not read as load-bearing.

Re-verified the original acceptance criteria after the refactor: make check green with a .claude/worktrees/ scratch dir present holding misformatted Go (the old gofmt -s -l . still flags it on that same tree), tracked gofmt and goimports drift still exit 1, make fmt leaves the scratch file byte-identical, shellcheck clean on both scripts at default severity, actionlint clean.

🤖 Addressed by Claude Code

[LukasWodka]

The defect is fixed, and I proved it rather than reading the diff. I shimmed a failing gofmt onto PATH and ran both versions of the script:

=== eb24527c (this head) ===
simulated gofmt failure
==> gofmt -s: FAILED (exit 1) — see the output above
EXIT=2 ✅
=== bf704048 (the version I blocked) ===
simulated gofmt failure
==> gofmt -s: FAILED (exit 1) — see the output above
==> fmt-check: 219 tracked Go file(s) clean
EXIT=0 ← the false green

The old version printed the failure and then claimed 219 files clean. That's now gone: stdout to $out_file, run_formatterreturns the status, and every call site is || exit 2. Clearing my change-request as soon as CI is green.

And scripts/tests/format-verify.sh is more than I asked for. I suggested a verification row; you wrote a 157-line hermetic self-test, wired it into build.yml:91andmake fmt-selftest, shellcheck'd it, and framed the whole file by the class rather than the instance — "Every case here is a way the gate could report CLEAN while checking nothing." It fails closed if format.sh is missing ("refusing to report pass"), and it carries the lesson forward in a comment: "Call it DIRECTLY, never as $(run_case ...)" — the bug's own shape, written down where the next person will hit it. 8 properties, green.

One gap: the new test is coarse, not vacuous

I mutation-proved the test itself, and it catches the defect only when every propagation path is broken:

mutation	format-verify result
swallow only the gofmt -s check-mode call site	8 properties hold, exit 0 ❌
swallow both check-mode call sites	FAIL: a failing formatter exited 0 (check mode swallowed it — the #550 defect), exit 1 ✅

So the test is real — it does redden on the defect — but what it actually asserts is "at least one propagation path works," not "each one does." The explode stub makes both formatters fail, so whichever call site is still || exit 2 carries the script to a non-zero exit and the case reads that as success.

That matters because it's how the original bug arrived: one call-site shape being wrong. A future regression that breaks only the gofmt path, or only goimports, ships green through this test — the precise scenario the file exists to prevent.

Fix is small: make the stub selective, so one case fails only gofmt and another fails only the goimports invocation. Then each propagation path is pinned independently and the mutation I ran above reddens.

Not blocking on it — the shipped defect is genuinely fixed, the test is a real improvement over nothing, and I'd rather this land than sit. But it's worth doing while the file is fresh, because a self-test whose name promises per-formatter coverage and delivers or-coverage is the same shape of problem one level up.

Everything else from my earlier reads still holds: the .-vs-repo diagnosis, the ./...-skips-dot-dirs explanation, 219 tracked files matching my own count, build.yml collapsing to make fmt-check so the file set has one definition, and GOIMPORTS_VERSION joining check-tool-pins' TOOLS.

[LukasWodka]

Approving — clearing my change-request as I said I would.

Green on eb24527c: 25 pass, 1 skipping, MERGEABLE, Bugbot's thread resolved, zero unresolved threads. And the blocking defect is fixed and proven — my PATH shim with a failing gofmt gives exit 2 on this head where the version I blocked gave exit 0 after printing "219 tracked Go file(s) clean".

The one open item is explicitly non-blocking and stays a follow-up: format-verify.sh reddens only when both check-mode propagation paths are swallowed, not when one is — because the explode stub fails both formatters, so whichever call site still carries || exit 2 takes the script to a non-zero exit and the case reads that as success. A selective stub (fail only gofmt in one case, only the goimports invocation in another) pins each path independently. Worth doing while the file is fresh, since the original bug was exactly one call site being wrong.

Everything else stands as verified: the .-vs-repo diagnosis with all four invocations confirmed on develop, the ./...-skips-dot-dirs explanation, 219 tracked files matching my independent count, build.yml collapsed to make fmt-check so the file set has one definition, and GOIMPORTS_VERSION added to check-tool-pins' TOOLS — which closes the gap that let the duplicate pin hide in the first place.

[saadqbal]

You're right, and the mutation table is the part that settles it — I'd have defended that test on the strength of "it reddens on the defect," which was true and beside the point. Fixed in a53d66b.

What it was actually asserting. The explode stub failed both formatters, so whichever call site still had || exit 2 carried the script to a non-zero exit and the case read that as success. Or-coverage under a name promising per-formatter coverage — and the gap is aimed precisely at how the original bug arrived, one call-site shape being wrong.

Fix. The two formatters get independent stubs (write_stub, mode per tool: gofmt bare, goimports via $GO), and two new cases fail exactly one of them, so only that call site can produce the non-zero exit.

Mutation-proved in both directions, which is the check I owed the first version:

mutation	before a53d66b	after
swallow only the gofmt check-mode call site	10 properties hold, exit 0 ❌	FAIL: a failing gofmt alone exited 0 — that call site swallows failures, exit 1 ✅
swallow only the goimports check-mode call site	(same gap)	FAIL: a failing goimports alone exited 0 — that call site swallows failures, exit 1 ✅
unmutated	8 hold	format-verify: 10 properties hold ✅

Each failure names the path that broke, so a future regression points at its own call site instead of "something swallowed a failure".

For the record on the head you verified: CI went fully terminal on eb24527 with no failures — 26 pass, and Cursor Bugbot flipped from skipping to pass on that push, so it did review the reworked script rather than sitting it out. make check stays green and inside budget with the two extra cases; shellcheck clean at default severity.

Ready for another look when you have a moment.

🤖 Addressed by Claude Code

[LukasWodka]

Approving. a53d66bd implements the per-path pinning exactly as intended, and I re-ran the mutation matrix on the PR head rather than taking it on trust:

mutation on scripts/format.sh	suite
drop || exit 2 on run_formatter "gofmt -s" gofmt -s -l (:137)	🔴 FAIL: a failing gofmt alone exited 0 — that call site swallows failures
drop || exit 2 on run_formatter "goimports" … -l (:142)	🔴 FAIL: a failing goimports alone exited 0 — that call site swallows failures

Each check-mode call site now reddens on its own, which is the property the shared explode fixture could not express. Baseline is 10/10 green, and I confirmed each mutation actually applied to the line I meant (an inert mutation and real coverage look identical in a log).

Two things I liked beyond the ask:

• write_stub records argv into a shared seen.txt while the two stubs behave independently — so splitting the behaviour didn't cost the scoping property, which still passes.
• the *) echo "make_fixture: unknown behaviour" … exit 2 arm. A typo'd fixture name now fails the suite instead of silently constructing a fixture with unset modes.

One residual, non-blocking. The same or-coverage still holds in write mode: --write is only exercised with the shared explode fixture, so mutating :121or:122 alone leaves the suite 10/10 green (verified — both mutations pass). Lower stakes than check mode, since a swallowed write failure gets caught by the check gate afterwards rather than shipping. The fixtures you already added cover it; it's the loop bound:

forwhichin gofmt goimports;do
dir="$(make_fixture "explode-${which}")"formodein --check --write;do
run_case "$dir""$mode"; rc=$CASE_RC
[ "$rc"-eq 0 ] && bad "a failing ${which} alone exited 0 in ${mode}"|| ok "a failing ${which} ALONE fails ${mode} (rc=$rc)"done
rm -rf "$dir"done

(--write on a fresh fixture is independent of --check having run first, since the stubs never modify files.) Take it here or in a follow-up — your call, not a condition of this approval.

[LukasWodka]

/fr-pass