Entire

Now I have a complete picture. Here's my review:

Review: `soph/add-ci-and-lint`

Overall

Good work. The CI setup is solid, the golangci-lint config is thorough, and the vast majority of linter fixes are genuine improvements — not silencing. A few items need attention though.

CI & Lint Config

The .golangci.yaml is well thought out:

nolintlint requires both explanation and specificity — this is the right call and it forces all //nolint directives to justify themselves
testifylint with enable-all is good given you're adding testify
gosec exclusions (G104, G115, G204, G301, G304, G703) all have inline comments with valid reasons
wrapcheck and gosec excluded from _test.go makes sense
govet.disable: [fieldalignment, shadow] — both are common and reasonable

The workflow files and mise tasks look clean. The mise-tasks/lint/go script correctly skips golangci-lint in CI (where the GitHub Action handles it with inline annotations).

Genuine Improvements (not silencing)

for i := 0; i < N; i++ → for i := range N / for range b.N — idiomatic Go 1.22+
fmt.Errorf("static string") → errors.New(...) — correct, avoids unnecessary formatting
err == packp.ErrEmptyAdvRefs → errors.Is(err, ...) — proper error chain checking
http.NewRequest → http.NewRequestWithContext with t.Context() — fixes noctx
exec.Command → exec.CommandContext in tests — good practice
_ = caps.Set(...) in tests → require.NoError(t, caps.Set(...)) — these actually assert now instead of ignoring
Import grouping sorted alphabetically — standard Go convention
Named params on interface method signatures — improves readability
Exhaustive switch cases (e.g. ActionSkip, ActionBlock, PacketResponseEnd) — prevents silent bugs when new enum values are added

`//nolint` Directives — Verdict

All well-justified except one (see bugs below). Each has an explanation as required by the config. Highlights:

//nolint:wrapcheck // io.EOF must pass through — correct, wrapping io.EOF breaks the io.Reader contract
//nolint:nilnil // nil signals no auth method found — legitimate API design
//nolint:nilerr // missing config dir means no stored credentials — correct, os.UserHomeDir error is expected in some envs
//nolint:errcheck // best-effort refresh token storage — reasonable, access token is already saved

Issues

1. graphStore = nil removed — this is a functional regression

internal/strategy/bootstrap/bootstrap.go:458 — The line graphStore = nil was removed (likely flagged by wastedassign or ineffassign), but it's load-bearing. The comment above explains it allows GC to reclaim a ~4.6 GB allocation for the linux kernel repo before the explicit runtime.GC() on the next line. Without it, the Go runtime may keep graphStore alive on the stack until the function returns.

Fix: restore the line with a nolint directive:

2. Double error wrapping in pkg/gitsync/client.go → internalbridge/config.go

client.Probe wraps with "probe: %w", calls internalbridge.Probe which also wraps with "probe: %w". Result: probe: probe: <original>. Same issue with Run → "run: %w" in the bridge and then "sync: %w" or "plan: %w" in the client (those are different, so fine).

Fix: remove the wrapping in internalbridge/config.go (it's a thin pass-through) or use distinct prefixes.

3. //nolint:unparam // error return kept for future use (bootstrap.go:505, checkPackSizeAndSubdivide)

"For future use" is speculative. Either the error return is useful now or it should be removed. If there's a concrete plan to use it, reference the issue. If not, use // kept for API consistency or remove the return value.

Minor Nits

X-GitHub-Api-Version → X-Github-Api-Version (bootstrap.go:664): This is the canonicalheader linter normalizing to Go's http.CanonicalHeaderKey. HTTP headers are case-insensitive so it's functionally fine, but it diverges from GitHub's documented header. Worth noting in case someone searches for the exact string.
fmt.Sprintf → string concatenation in planner.go reason messages: Fine for performance (these are hot paths during planning), but make sure the team is OK with the readability tradeoff. The perfsprint linter drives this.
Adding stretchr/testify: The project was previously stdlib-only for testing. The usage here is limited to require.NoError to replace _ = err in tests. Reasonable, but worth calling out as a new dependency.
PlanReplicationRef switch (planner.go:340-344): Added case RefKindBranch: to the switch, but its body is identical to the default: case. If this is for exhaustive checking, consider using the same body or a fallthrough. Currently it's:

The branch and default cases are the same string. You could do case RefKindBranch: fallthrough or combine them.

Summary

The graphStore = nil removal is the only real bug — everything else ranges from solid to nitpick. The linting is genuinely fixing issues, not just adding //nolint to get green.