Enable depguard to keep exactly one MySQL driver linked

[morgo]

Spirit links exactly one MySQL driver, github.com/block/mysql. This adds depguard to make that an enforced invariant rather than a convention.

The reason it needs an enforcer: nothing about linking a second MySQL driver fails to compile, and both of the ways it breaks are silent.

The registry half — this one already bit a consumer. A tls= DSN value is a name, and a name only resolves inside the TLS registry of the driver package that registered it. Registries are per-package globals; nothing about them travels in the DSN. pkg/dbconn registers rds into block/mysql (conn.go) and hands that name back in every enhanced DSN — including to downstream consumers that reuse EnhanceDSNWithTLS. A pool dialing through any other MySQL driver cannot resolve the name and fails to open against an RDS host at all.

That is host-shaped breakage: invisible to a suite that never points at an *.rds.amazonaws.com address, which is exactly why it sat unnoticed in SchemaBot's storage pool until the driver rename surfaced it (block/schemabot#1320).

The error half. Two drivers define two field-identical but distinct *mysql.MySQLError types, and errors.AsType against one returns false for the other — so the classifiers in pkg/dbconn, pkg/checkpoint and pkg/throttler keep compiling and silently stop recognizing deadlocks, lock-wait timeouts and unknown-system-variable.

Not a cleanup

The tree has no upstream import today. This is purely a regression guard.

Verification

• golangci-lint run → 0 issues
• go build ./... clean
• Confirmed the rule is not inert, since an enabled-but-never-firing linter is worth nothing: temporarily denying github.com/block/mysql instead flags 102 of the 103 importers (the default max-same-issues=3 cap hides the rest until you pass --max-same-issues=0, which is worth knowing if you go looking). Restored and diffed byte-identical afterwards.

🤖 Generated with Claude Code

[morgo]

🤖 Heads-up on a fix that came in via the sibling change (squareup/strata#425), where a reviewer bot raised a P2 that applies here too.

The depguard rule alone does not enforce the invariant. It is an AST-level check, so it only sees imports written in this repo's own files. A transitive dependency that imports upstream go-sql-driver links a second driver just as effectively and depguard says nothing — and that is the more likely way this regresses. Not hypothetical: one internal consumer of this library reaches upstream today purely transitively, through three separate dependencies, with no first-party import anywhere.

So this PR now also adds scripts/check-single-mysql-driver.sh, which asks the resolved package graph, wired as a step in the lint workflow. go list -deps -test is the authoritative question; go mod why is not, since it reports the shortest path and hides the real transitive cause.

One thing worth reading the second commit for. This check went green in CI on the first try, and that was luck. printf '%s\n' "$deps" | grep -qx PATTERN inverts its result when the match lands early in a long list: grep -q exits on first match, printf takes SIGPIPE writing the rest, and under set -o pipefail the pipeline reports that failure rather than grep's success. Spirit's 406-package list fits the pipe buffer so nothing looked wrong; strata's ~1240-package list did not, and it failed there. The same pattern guarded the banned driver, where an early match would have read as "not present" — a silent pass, the one outcome the script exists to prevent.

Everything now greps a file. Verified both directions: clean graph passes at 406 packages, and a first-party package importing only go-mysql/errors (invisible to depguard — confirmed at 0 issues) fails and names it. lint is green.

[Kiran01bm]

🤖 Review findings - created by Kiran's code review agent - for spirit/pull/1222, 1c38177.

Reviewed the full PR, with a focused pass on the delta 608d013a → 1c38177e ("Grep the package list from a file, not through a pipe"). No review was posted at the earlier heads, so everything still open is included here rather than only the delta.

Verdict: 8 findings — 2 blocking (a build-tag blind spot that both gates share, and private repo names on a public surface), 5 non-blocking, 1 suggestions bucket. The core premise is sound and I proved it: on a tree with a transitive banned import the script fails while golangci-lint run prints 0 issues. The delta's SIGPIPE fix is real hardening, though its stated justification does not hold at spirit's graph size.

Blocking

1. The gate runs go list with no -tags, and golangci-lint does not cover for it — so a banned import behind a build tag passes both gates. :61 resolves only the default-tag graph, and neither .golangci.yaml nor linter.yml sets build tags, so depguard is blind to the same files. Demonstrated: with a //go:build singleversion file in pkg/migration importing the banned driver, the script printed OK: 403 packages, exactly one MySQL driver linked (exit 0) and golangci-lint run was clean, while go list -deps -test -tags singleversion ./... listed it. singleversion and semisync are built for real by compose/compose.yml:52 and compose/semisync.yml:137 via two CI workflows, so a second driver would be linked into binaries CI actually runs.

2. The header names two private Block repositories, and the delta adds a third reference. :28 names an internal Go service platform and an internal RDS connector package, and asserts their current dependency posture; the new comment at :50 names a third ("the sibling _____ change"). All three resolve to "visibility":"private" under an authenticated token and 404 publicly, while block/spirit is public, so the comment ships in every clone and blob view. None of it is load-bearing — "a much larger sibling graph" says the same thing.

Non-blocking

3. count=$(grep -c . "$deps_file") at :67 still aborts silently, and the delta rewrote this exact line without fixing it. grep -c exits 1 on zero matches and the assignment takes that status, so set -e kills the script before the <100 guard on :68 runs. Verified against a module with zero .go files: total output was the one checking the package graph for… line, then exit 1 — the carefully-worded "The check is not meaningfully running. Fix it rather than trusting the pass." never prints, in exactly the case it was written for. || true, or wc -l < "$deps_file".

4. BANNED hardcodes one path while :96 claims "exactly one MySQL driver linked". :40 bans only go-sql-driver/mysql, but github.com/go-mysql-org/go-mysql/driver is a full database/sql driver doing sql.Register("mysql", driver{}) in an init(), and its module is already a direct require at go.mod:9 with eight of its packages linked. One import line links a second driver; script and depguard both stay green. schemabot's equivalent rule needs two deny entries for this reason.

5. The delta's justification is wrong for spirit, and its closing claim is wrong for the file. The SIGPIPE inversion is real — I reproduced PIPESTATUS=(141 0) and a silent pass — but the cliff is the 64 KB pipe buffer, which is 65536 on Linux CI as well as locally, and spirit's list is 403 lines / 12,798 bytes (19.5%). Injecting the banned entry at line 1 of the real list gave 0 inversions in 300 runs, so "it would have failed in the worst direction" is not true at this graph size (it needs ~2,060 packages), and the "invisible locally" framing blames the environment when the variable is graph size. Separately, "No pipes, no SIGPIPE" at :52 is contradicted by the four-stage pipeline 37 lines below at :89.

6. depguard is silently bypassed for generated files, and spirit has real ones in the linted module. golangci-lint v2 defaults linters.exclusions.generated: lax and .golangci.yaml sets no override, so any file whose first line carries a Code generated … DO NOT EDIT. header skips all linters including depguard — pkg/parser/parser.go and pkg/parser/hintparser.go qualify and are regenerated by parser-regen.yml. A blanket generated: disable surfaces 15 pre-existing issues, so the carve-out needs to be targeted.

7. The load-bearing rationale misattributes the breakage, in two files at once. The header at :20-23 and the identical desc at .golangci.yaml:37 name pkg/dbconn, pkg/checkpoint and pkg/throttler for deadlocks, lock-wait timeouts and unknown-system-variable. Only dbconn matches, and only for 1205/1213 (dbconn.go:216); checkpoint.go:41 guards 1146/1054 and throttler/aurora.go:183 guards access-denied codes. Error 1193 lives in the two packages the sentence omits — migration/check/configuration.go:76 and move/check/configuration.go:69.

8. CI-only gate with no local entry point, and two AGENTS.md surfaces go stale on merge. make lint and the .githooks pre-push both run golangci-lint in Docker only, which by finding 1's own logic cannot see the transitive case; the repo's convention wraps developer-facing scripts as make targets (setup-hooks: @./scripts/setup-git-hooks.sh), and this one costs 0.4 s warm. AGENTS.md still describes linter.yml as golangci-lint only, and ### Database connections documents none of the one-driver rule — so the invariant is machine-enforced in three places and human-documented in none.

General suggestions

Both mktemp calls now precede the single trap at :59, so a failure of the second leaks the first — mktemp -d plus one trap closes it. grep -qx at :76/:82 wants -F (the . in github.com is a wildcard). :38 does not resolve symlinks, so invoking through one blames go list for the wrong thing. The step has no if: always(), so one unrelated lint nit suppresses the driver check entirely. The remediation at :92 covers only the first-party case, leaving the transitive operator with a package list and no next step. And the rationale now exists in five places — YAML comment, depguard desc, this 30-line header, the workflow comment, and the pkg/dbconn godoc — two of which already carry the identical wrong sentence from finding 7; conn.go:211-219 is the right canonical home.

The one thing that could have broken, verified

The whole commit rests on depguard being AST-level and unable to see a transitive pull. I did not take it on faith: I built a local module importing the banned driver, wired it in with go mod edit -require -replace, and had pkg/dbconn import only that shim, with no first-party mention of the banned path. The script failed with Importers: example.com/…; golangci-lint run on the identical tree printed 0 issues. The premise holds exactly as written.

Verified correct

• The delta closes the SIGPIPE hazard on every branch that decides pass/fail; the three decision greps at :67/:76/:82 are now plain file reads.
• I tried to break the remaining Importers: pipeline and could not: with a genuine transitive-only import, -deps keeps the importer list non-empty, so grep -v matches and the full failure message — including both remediation lines — prints.
• Gate fires on blank, named and type-referencing imports, in _test.go files, in external _test packages, and on subpackages.
• The block/mysql-absent reverse-rot guard and the broken-go.mod path both fail closed with their intended messages.
• Temp-file hygiene is sound: zero leaks across 12+ runs on the pass path, both fail paths, the set -e abort, and a mid-run SIGINT.
• Identical behaviour from the repo root, from an absolute path with a different cwd, and under bash <path>; exec bit set (100755).
• Dependencies' own test binaries are excluded, which is the right semantics for a "what links into our binaries" check; the nested pkg/parser/goyacc module is outside the graph but requires only modernc.org/* + bigfft.
• CI fully green.

This review was generated by Claude Code (claude-opus-5).

[aparajon]

🤖 Adversarial correctness review — 1c38177e (3 files, +123/−0)

Right pair of instruments, and the second one is the part I would not have thought to ask for. depguard is AST-level, so it reasons only about first-party imports; measured on this branch:

go list -deps -test  →  403 packages in the resolved graph
first-party          →  109  (github.com/block/spirit/...)

The other ~294 are exactly where a transitive second driver arrives, and the script is the only thing that asks about them. I verified both instruments bite rather than trusting their names — planting _ "github.com/go-sql-driver/mysql" in pkg/dbconn:

script    FAIL: … is linked alongside github.com/block/mysql.
          Importers:
            github.com/block/spirit/pkg/dbconn
            github.com/block/spirit/pkg/dbconn [.../pkg/dbconn.test]   ← test variant too
          exit=1

depguard  pkg/dbconn/zz_canary.go:3:8: import 'github.com/go-sql-driver/mysql' is not
          allowed from list 'main': import github.com/block/mysql instead … (depguard)

Clean run is ~0.9s, so this is free in the lint job. Choosing go list -deps -test over go mod why is the difference between asking what is linked and what is recorded, and the header is right that the wrong version of this check is the easy one to write.

Three findings. One I'd fix before merge; the other two are low.

#	Sev	Where	What
1	med	scripts/check-single-mysql-driver.sh:28	The header's parenthetical names an internal monorepo and one of its private service repos. block/spirit is public and carries no such reference today — merging publishes them
2	low	.golangci.yaml:37	The desc's package list and its error-kind list don't intersect as stated: two of the three named packages classify none of the three named codes, and the code it names most specifically lives in two packages it doesn't name
3	low	scripts/check-single-mysql-driver.sh	Both instruments are blind to build-tag-gated files — 1,348 lines across four test files — and nothing reports the gap

---

1 — the header publishes two private repo names (med)

Line 28's parenthetical names a Block-internal monorepo and, inside it, one of its private service repos. I'm deliberately not repeating either name here, since this comment lands on a public repo too.

Checked rather than assumed:

• block/spirit is public (visibility: PUBLIC).
• The monorepo is private; the service repo is private.
• Grepping this repo's origin/main for that organization's path prefix returns zero files. This PR introduces the first such reference, into a public artifact.

The other two names in that same parenthetical are fine — one is a published OSS project, the other a public module path — so this is about two of the four.

The claim they support is the most valuable sentence in the header, because it turns "a transitive dep could do this" into "a transitive dep does do this, three hops deep, right now." Keep the claim; de-identify the evidence — "a sibling internal monorepo, through three separate transitive dependencies" carries the same weight. The same sentence appears in the sibling change in the internal repo, where it is fine because that repo is not public; the copy across siblings is what makes this easy to miss.

2 — the deny message's packages and its error codes describe different sets (low)

The desc is genuinely good, and explaining the mechanism rather than just the ban is what makes a compile-time canary survive a future "simplify this" pass. But it says the classifiers in pkg/dbconn, pkg/checkpoint and pkg/throttler stop recognizing deadlocks, lock-wait timeouts and unknown-system-variable, and only the first of those three packages classifies any of those three codes.

What the named packages actually classify, and what happens when errors.AsType starts returning false:

Named package	Codes it classifies	Consequence
pkg/dbconn	1205 lock-wait, 1213 deadlock (canRetryError, IsLockContentionError)	retries stop happening — silent, and the message is right here
pkg/checkpoint	1146 no-such-table, 1054 bad-field (IsIncompatible)	an incompatible cross-version checkpoint reads as a transient read error, so the run fails asking for a retry that can never succeed
pkg/throttler	access-denied codes (isPrivilegeDeniedError)	the wording of one probe-failure log line

And where the omitted ones are:

Not named	Codes	Consequence
pkg/change:1568,1690	IsLockContentionError	the other genuinely silent one — the buffered subscription stops adapting to contention its own flush fan-out causes
pkg/migration/check:76, pkg/move/check:69	1193 unknown-system-variable	if !ok || Number != 1193 { return err } — !ok takes the error branch, so the config check refuses to run against MySQL < 8.0.20
pkg/datasync/runner.go:794	1146	a fresh sync's legitimately-absent target table stops reading as expected, so datasync refuses to start

Two things fall out. pkg/throttler is the least consequential site in the repo — a log message — and it made the list of three, while pkg/change, whose degradation is the clearest instance of the silence the message is warning about, didn't. And unknown-system-variable is the one code named with an error number's worth of specificity, yet it lives only in the two check/configuration.go files, where the guard's !ok || polarity means the failure is a startup refusal, not silence — so "silently stop recognizing … unknown-system-variable" is wrong on both halves.

This matters more than a docs nit because the desc is the remediation an engineer reads at the moment they've been blocked, and it's the artifact arguing against deleting the rule later. Swapping pkg/throttler for pkg/change and either dropping unknown-system-variable or moving it to its own clause ("and the configuration checks refuse to run at all") keeps the length and makes every clause true.

3 — a build-tag-gated file evades both instruments (low)

Neither instrument passes build tags: .golangci.yaml has no run: section, so golangci-lint builds the default configuration, and the script calls go list -deps -test ./... with no -tags. So the two are consistently tag-blind — which is coherent, but leaves a hole neither reports. Verified by planting the same canary behind a tag:

//go:build singleversion
package migration
import _ "github.com/go-sql-driver/mysql"

script    OK: 403 packages, exactly one MySQL driver linked      exit=0
depguard  0 issues.                                             exit=0

Both green. The same import in an untagged _test.go file is caught by both, so the evasion axis is the tag, not the test file. Behind tags today: pkg/migration/resume_test.go and singleversion_test.go (singleversion), cutover_semisync_test.go (semisync), pkg/parser/reserved_words_test.go (reserved_words_test) — 1,348 lines.

Low, because those are test files and nothing in .github/workflows/ or the Makefile passes any of those tags, so a driver landing there wouldn't reach a CI binary. But it is a gap in the check's own stated scope rather than a boundary it drew: passing -test is a deliberate choice to cover test variants, and this covers some test files and not others. It also differs from the sibling internal change, which enumerates its tags in run.build-tags and feeds them to the script — worth knowing when reading the two together.

Cheapest fix in keeping with the rest of the script: loop the graph query over the tags (for tags in "" singleversion semisync reserved_words_test), and add run.build-tags so depguard sees them too. If that isn't worth it, one line in the header saying tag-gated files are out of scope stops the next reader from assuming otherwise — this script is unusually careful about not silently checking less than intended, and this is the one place it does.

---

Verified — the TLS registry claim, the SIGPIPE reasoning, the guards, and the workflow

The registry half of the desc is exactly right, and it is a restatement of behavior already documented on main. pkg/dbconn/conn.go:19 defines DriverName = "block-mysql"; initRDSTLS (conn.go:224) registers rdsTLSConfigName = "rds" into block/mysql; EnhanceDSNWithTLS is exported at conn.go:531 and hands tls=rds out to consumers. The doc comment above initRDSTLS already spells out the failure — a consumer opening a tls=rds DSN with upstream go-sql-driver fails in ParseDSN with "unknown config name: rds", before any dial, on every RDS host. So the deny message isn't asserting something new; it is putting a documented, host-shaped failure in front of the person about to reintroduce it.

The set -o pipefail reasoning is correct and the fix removes the class rather than working around it. Under printf "$deps" | grep -q, grep -q exits on first match, printf takes SIGPIPE, and the pipeline reports failure even though the match succeeded — which for the banned driver inverts in the worst direction, since an early match would read as "not present" and pass silently. Going to a temp file removes it. Keeping stderr separate and consulting go list's status on its own is the same instinct: a partially-printed graph from a failed module load can't be mistaken for a clean one.

Both negative guards are real, not decorative. The -lt 100 floor fires against a 403-package graph with room to spare, and the grep -qx "github.com/block/mysql" guard is the one I'd have asked for — it catches the reverse rot, where the driver gets replaced and the check keeps passing while asserting nothing. The Importers: diagnostic resolves the actual importer including its .test variant, which is what makes a transitive hit triageable rather than just red. One nit inside it: it is the only place in the script that discards its own errors (2>/dev/null, status unchecked), so a go list failure there prints an empty list under a FAIL: already raised — harmless, and the failure is still correct.

depguard covers test files here, so the two instruments overlap where it matters and diverge where it matters. No run.tests: false is set and v2 lints tests by default — my canary in an untagged _test.go was flagged, and the script independently reported the .test variant. A test reaching for upstream is caught by depguard on the same footing as production code; a transitive dep reaching for it on a test-only path is caught by the script's -test. Neither half is redundant.

Workflow placement is right. The step runs in the existing lint job after golangci-lint, so it inherits actions/setup-go@v7.0.0 pinned to 1.26.6 — the same version as go.mod — and needs no new permissions beyond contents: read. 19/19 checks green, which also demonstrates the graph loads in CI, the thing most likely to make a go list-based check fail there.

One difference from the sibling worth recording. The header's go mod why aside notes that go.mod/go.sum can carry a module nothing links. In the sibling repo that is concrete — its go.sum has go-sql-driver hash lines while its graph is clean. Here it is hypothetical: go.mod has no require and go.sum has zero go-sql-driver lines. The reasoning still holds, since the point is the substrate choice rather than the current fossil, but the two repos aren't in the same starting state.

Local checks. Script passes clean on the branch (403 packages, ~0.9s). golangci-lint run --enable-only=depguard clean across the unmodified branch. Canary and the go.mod/go.sum changes go mod tidy made for it fully reverted; git status --porcelain empty at 1c38177e.

This review was generated by Claude Code (claude-opus-5).

[aparajon]

🤖 Approving — the pair of instruments is right and I verified both bite. Findings are in my review comment; the medium one (two private repo names in a public artifact) is worth fixing before merge, the other two are low.

This stamp was left by Claude Code (claude-opus-5).

[morgo]

🤖 All three addressed in dc49c58. Each one held up when I checked it.

1 — private repo names in a public artifact (medium). Fixed, and it was in one more place than the review found. Confirmed the visibilities rather than assuming: block/spirit is PUBLIC, squareup/gap and squareup/ods-rds-connector are both PRIVATE, and cashapp/blip is PUBLIC — so the split was exactly the two you named. Worth adding that block/schemabot, referenced elsewhere in the same header, is also public, so that one stays.

The place the review missed: the same two names were in my own comment on this PR, which is public and was already published. I have edited it to match the header. GitHub keeps edit history, so the original is still reachable via the "edited" marker — flagging that rather than treating the edit as a clean removal.

The claim itself stays, de-identified to "one internal consumer of this library links upstream today through three separate transitive dependencies". Your read that this is the header's most valuable sentence is why I kept it rather than cutting it.

2 — the deny message's packages and codes describe different sets (low). Correct, and I did not apply the suggested swap verbatim. pkg/change is a consumer of the classifier, not a classifier — subscription_buffered.go:1568,1690 call dbconn.IsLockContentionError rather than doing their own errors.AsType — so adding it to a list introduced as "the classifiers in ..." would have traded one inaccuracy for another. It is now described as what it is: dbconn stops recognizing 1205/1213, and pkg/change's buffered subscription stops backing off through dbconn's helper.

Verified the rest before rewriting: pkg/checkpoint/checkpoint.go:41 gates IsIncompatible on 1146/1054; pkg/throttler/aurora.go:183 is a single site behind isPrivilegeDeniedError — a log line, as you said. And the 1193 polarity is exactly as described, at both pkg/migration/check/configuration.go:76 and pkg/move/check/configuration.go: !ok || myErr.Number != erUnknownSystemVariable sends !ok down the error branch, so that check refuses to run rather than going quiet. It now gets its own clause as the loud failure, which is the opposite direction from everything else in the message.

3 — tag-gated files evade both instruments (low). Reproduced your canary and fixed the script half. Rather than hardcoding a tag list, it derives tags from the repo's own //go:build lines, so a new tag-gated suite is covered when it lands:

before  OK: 403 packages, exactly one MySQL driver linked          exit=0
after   FAIL: github.com/go-sql-driver/mysql is linked alongside github.com/block/mysql.
        Importers:
          github.com/block/spirit/pkg/migration [github.com/block/spirit/pkg/migration.test]
                                                                   exit=1

Two things I checked before enabling every tag at once: no constraint in this repo is negated (all three are bare single tags), so combining them cannot exclude a file the default build would include — and the derivation now hard-fails if a negated constraint ever appears, since that is precisely when the assumption stops holding.

I did not add run.build-tags, so depguard stays tag-blind. Turning it on surfaces 4 pre-existing issues in those files (2 modernize, 2 noctx), and fixing unrelated test files does not belong in a change about driver enforcement. Your own reason is why that is acceptable — nothing in .github/workflows/ or the Makefile passes those tags, so a driver landing there never reaches a CI binary. The header now states the asymmetry instead of leaving the next reader to infer it.

Also fixed, from your note inside the Verified section: the Importers: diagnostic was the one place discarding its own errors. It reports the failure now — via a file, not a pipe, because grep -v '^$' exits 1 when every line is blank and would have reported failure for a go list that worked fine. Same class as the SIGPIPE trap, opposite direction.