chore: Reduce churn on future PRs

[rkuris]

Let's discuss how we can reduce churn on future golang PRs. Here's a claude analysis of a recent golang PR in firewood as a starting point.

PR #1814 Review Analysis

PR #1814 (feat(reconstruction): Add Go bindings and tests) required significant review effort — 44 reviewer comments across 36 threads, 40 author responses, all within a single day.

Metrics by Reviewer

Reviewer	Unique issues raised	Replies/follow-ups	Rounds to approval
Copilot	9	0	5 passes (never approves)
alarso16	11	4	3 (CHANGES_REQUESTED → APPROVED)
RodrigoVillar	14	1	5 (4× COMMENTED → APPROVED)
maru-ava	2	1	3 (COMMENTED only)
demosdemon	2	0	1 (straight to APPROVED)
rkuris (responses)	—	40	—

Issue Classification

Category	Count	Catchable by tooling?
Correctness / Bugs (finalizer panic, WaitGroup race, error-path leak)	6	Partially — go vet, -race
Concurrency / Lock ordering (deadlock risk, explain unlock/relock)	4	Partially — -race, go-deadlock
Code style / Idiomatic Go (r.NoError, errgroup, defer, struct ordering)	8	Partially — revive, team conventions
Error handling / Lint (errcheck, explicit _ = ...)	5	Yes — errcheck, golangci-lint
Magic numbers ("Why 32?", use constants)	4	Yes — mnd linter
Test design / Scope (unclear test purpose, missing cleanup, separate tests)	7	No — requires human judgment
Documentation / Comments (inaccurate comments, missing docs)	5	No — requires human judgment
Refactoring / DRY	3	No
API design (export decisions, wrapping vs embedding)	2	No

Of the 44 comments, approximately 19 (43%) were catchable by existing or easily-added tooling. The remaining 25 required human judgment.

Tooling Recommendations

A. Add mnd (magic number detector) to golangci-lint

Would catch 4 comments automatically. Add to .golangci.yaml:

- mnd

B. Run golangci-lint locally before pushing

CI already runs golangci-lint, but 5 errcheck/lint issues made it to review. A pre-push hook or habit of running cd ffi && golangci-lint run ./... before opening the PR would eliminate these.

C. Consider go-deadlock for debug builds

The lock-ordering issues (4 comments) are subtle. The go-deadlock library replaces sync.Mutex in debug builds and detects potential deadlocks at runtime. This could be wired into test builds.

D. Require t.Cleanup for resource handles

Copilot flagged missing cleanup. A team convention (or custom linter rule) requiring t.Cleanup(handle.Drop) after every handle creation would prevent test resource leaks.

E. Pre-submission checklist for Go FFI PRs

A lightweight checklist could help catch the most common issues before review:

• golangci-lint run ./... passes
• go test -race ./... passes
• No magic numbers in tests (use constants)
• All handles have t.Cleanup registered
• Mutex fields precede guarded fields in structs

F. Tune Copilot reviewer

Copilot raised 9 issues. Some were genuinely valuable (finalizer panic, WaitGroup race), but others duplicated what the linter would catch (errcheck on rng.Read). If Copilot re-reviews on every push, it adds noise — consider configuring it to only review the initial diff.

Already Addressed

• PR #1820 addresses the rand.Read errcheck issues.

[demosdemon]

Anecdote that doesn't really change anything: my stats on this are biased. I triggered copilot the first time and approved well after the conversation had covered many the things I may have mentioned.

[rkuris]

CI already runs golangci-lint, but 5 errcheck/lint issues made it to review. A pre-push hook or habit of running cd ffi && golangci-lint run ./... before opening the PR would eliminate these.

This one is a great one for possible discussion. I wonder if we always create everything in draft until it's been through the lint checks when it can be promoted. Is this even possible?