chore: apply go fix modernizations and guard drift in pre-commit#517
Conversation
Run `go fix` across the module and adopt the modern stdlib idioms it
suggests: maps.Copy, slices.Contains/Backward, strings+bytes.CutPrefix,
strings.SplitSeq, min/max, range-over-int, reflect.Type.Fields,
sync.WaitGroup.Go, and new(expr) in place of hand-rolled pointer helpers.
Go 1.26's new(expr) makes the intPtr/boolPtr/stringPtr/... helpers
redundant, so all 19 are deleted along with the `//go:fix inline`
directives go fix injected to mark them.
`go fix` needed two passes to converge (it inlines a helper's callers only
on the run after it marks the helper), and it skips build-tagged files
unless -tags is passed -- so tests/{e2e,integration,contract} were fixed
too, and both facts are captured in the new Makefile targets.
All changes are behavior-preserving. The only wire-visible candidates were
five `,omitempty` tags dropped from struct-typed JSON fields (time.Time and
two Gemini decode-only types); omitempty is a documented no-op on structs,
verified against goccy/go-json, which this repo uses.
Add `make fix` (apply) and `make fix-check` (report, non-mutating), and
wire fix-check into pre-commit. The hook checks rather than auto-applies:
go fix rewrites source in place and can leave an orphaned helper that
trips the `unused` linter, so an auto-applying hook would mutate the
commit and still fail it.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds ChangesGo fix modernization
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Makefile`:
- Around line 125-128: The lint-fix target is missing the same build-tag
handling used by lint, so it skips tag-gated files and can drift from the rest
of the lint flow. Update the Makefile’s lint-fix recipe to pass BUILD_TAGS to
golangci-lint run --fix, matching the lint target behavior so files behind e2e,
integration, and contract tags are included consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: af1fa4e5-f648-4738-b181-f209ba8b1cbb
📒 Files selected for processing (95)
.pre-commit-config.yamlMakefileconfig/cache.goconfig/cache_validation_test.goconfig/user_path_env.goext/registry_test.gointernal/admin/dashboard_config_contract_test.gointernal/admin/handler_live.gointernal/admin/handler_test.gointernal/anthropicapi/stream_test.gointernal/auditlog/auditlog.gointernal/auditlog/middleware_test.gointernal/authkeys/service.gointernal/conversationstore/store.gointernal/core/errors_test.gointernal/core/json_fields.gointernal/core/responses_field_parity_test.gointernal/core/types.gointernal/core/usage_json.gointernal/core/user_path_test.gointernal/failover/resolver.gointernal/failover/resolver_test.gointernal/failover/suggest.gointernal/gateway/batch_orchestrator.gointernal/gateway/model_helpers.gointernal/guardrails/llm_based_altering.gointernal/guardrails/responses_message_apply.gointernal/live/broker_test.gointernal/llmclient/client_test.gointernal/modeldata/merge.gointernal/modeldata/merge_test.gointernal/modeldata/types.gointernal/pricingoverrides/service_test.gointernal/pricingoverrides/store_sqlite_test.gointernal/providers/bedrock/bedrock.gointernal/providers/factory.gointernal/providers/gemini/gemini.gointernal/providers/gemini/gemini_test.gointernal/providers/gemini/native.gointernal/providers/registry_metadata.gointernal/providers/registry_metadata_override_test.gointernal/providers/resolve_bench_test.gointernal/providers/responses_converter.gointernal/providers/vllm/vllm.gointernal/providers/xai/xai.gointernal/ratelimit/limiter.gointernal/ratelimit/service_test.gointernal/ratelimit/store_sqlite_test.gointernal/ratelimit/types_test.gointernal/ratelimit/usage_tap_test.gointernal/realtime/registry_test.gointernal/responsecache/handle_request_test.gointernal/responsecache/semantic_test.gointernal/responsecache/sse_validation_test.gointernal/responsecache/vecstore_cleanup.gointernal/responsestore/store.gointernal/server/audio_service_test.gointernal/server/handlers_test.gointernal/server/http_test.gointernal/server/native_conversation_service.gointernal/server/native_file_service.gointernal/server/native_response_service.gointernal/server/realtime_webrtc_service.gointernal/server/response_input_items.gointernal/usage/audio_test.gointernal/usage/cost_test.gointernal/usage/dashboard_cost_validation_test.gointernal/usage/reader_fold_test.gointernal/usage/recalculate_pricing_issue435_test.gointernal/usage/savings_test.gointernal/usage/stream_observer_test.gointernal/usage/throughput_test.gointernal/virtualmodels/balancer_test.gointernal/virtualmodels/config_overlay_test.gointernal/workflows/types_test.gorun/lifecycle_test.gotests/contract/golden_helpers_test.gotests/contract/replay_harness_test.gotests/e2e/admin_test.gotests/e2e/auditlog_test.gotests/e2e/auth_test.gotests/e2e/chat_test.gotests/e2e/failover_test.gotests/e2e/helpers_test.gotests/e2e/main_test.gotests/e2e/mock_provider.gotests/e2e/ratelimit_test.gotests/e2e/responses_test.gotests/integration/admin_test.gotests/integration/auditlog_test.gotests/integration/helpers_test.gotests/integration/main_test.gotests/integration/setup_test.gotests/integration/usage_test.gotests/integration/workflows_guardrails_test.go
💤 Files with no reviewable changes (6)
- internal/responsecache/sse_validation_test.go
- internal/guardrails/llm_based_altering.go
- internal/core/user_path_test.go
- internal/workflows/types_test.go
- internal/virtualmodels/config_overlay_test.go
- internal/llmclient/client_test.go
lint-fix ran without --build-tags and without ./tests/..., so its autofix pass never visited the swagger-gated files under cmd/ and internal/server, nor any of the e2e/integration/contract files -- which live entirely under tests/. The two targets could therefore drift: `make lint` fails on a file `make lint-fix` cannot even see. Passing BUILD_TAGS alone is not enough. The e2e/integration/contract tags gate files only under tests/, so without ./tests/... in the package list golangci-lint still never loads them. Verified with an unused helper in a //go:build e2e file: tags-only reports 0 hits, tags + ./tests/... reports it. The recipes now differ only by --fix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
What
Ran
go fix ./...across the module and adopted the modern stdlib idioms it suggests, then added a pre-commit guard so the tree stays modernized.Net −112 lines (
448 insertions,560 deletions) across 95 files. OnlyMakefileand.pre-commit-config.yamlare non-Go.new(expr)replacing pointer helpersmaps.Copyslices.Contains/slices.Backwardmin/maxstrings.SplitSeqstrings.CutPrefix/bytes.CutPrefixreflect.Type.Fieldssync.WaitGroup.GoGo 1.26's
new(expr)makes the hand-rolledintPtr/boolPtr/stringPtr/… helpers redundant, so all 19 are deleted, along with the//go:fix inlinedirectivesgo fixinjected to mark them.User-visible impact
None. Every change is behavior-preserving.
The only wire-visible candidates were five
,omitemptytags dropped from struct-typed JSON fields — threetime.Time(auditlog.AttemptSnapshot.StartedAt, andStoredAt/ExpiresAtin the conversation/response stores) and two Gemini decode-only types.omitemptyis a documented no-op on struct values, so the fields were always emitted. Verified empirically againstgoccy/go-json(which this repo uses in place ofencoding/json): zero and non-zero values marshal byte-identically with and without the tag.The two
//go:build-gated Gemini types (geminiGenerateContentResponse,geminiCandidate) are never marshaled — decode only — so tags are irrelevant there.Two
go fixgotchas worth knowingBoth are now captured as comments in
Makefileand.pre-commit-config.yaml:go fixmarks a helper//go:fix inline, it inlines that helper's callers only on the next run — leaving the helper orphaned and tripping theunusedlinter. A singlego fix ./...leaves the tree lint-failing.go fix ./...never seestests/{e2e,integration,contract}orswagger. Those had ~100 KB of unapplied fixes; this PR fixes them too by passing-tags.Tooling
make fix— apply modernizations (documents the two-pass caveat).make fix-check— report drift viago fix -diff, non-mutating, exits non-zero on drift (~0.7 s).go-fix-checkpre-commit hook runsmake fix-check.BUILD_TAGSMakefile variable now thatlint,fix, andfix-checkall share it.make lintexpands to the identical command as before.The hook checks rather than auto-applies, deliberately:
go fixrewrites source in place and (per gotcha 1) can leave an orphaned helper that failsmake lint. An auto-applying hook would mutate the commit and still fail it.Verified the hook is real by injecting a
for k, v := rangecopy loop into a//go:build e2efile: plaingo fix -diff ./...exits 0 (misses it),make fix-checkexits 2 (catches it). The-tagsare load-bearing.Testing
All 13 pre-commit hooks pass on this commit:
make test-race— 60/60 packages, no data racesmake lint— 0 issues (--build-tags=swagger,e2e,integration,contract)make perf-check— hot-path alloc/byte guards within thresholdsmake fix-check— tree is ago fixfixed pointgo mod tidy— cleantests/{e2e,integration,contract}compile under their build tagsThe 3 pre-existing
go vetrepeats json tagwarnings ininternal/coreare unchanged by this PR (confirmed against baseline).🤖 Generated with Claude Code
Summary by CodeRabbit
fixandfix-checkdeveloper commands for Go modernization checks and updates.