Conversation
saucam
approved these changes
May 5, 2026
Contributor
There was a problem hiding this comment.
Code Review
This pull request modernizes the codebase by adopting Go 1.21+ features, such as the max built-in, the slices package for collection checks, and the any alias for interface{}. It also simplifies integer-based loops using the range keyword. However, a critical compilation error was introduced in the integration tests by attempting to use a non-existent Go method on sync.WaitGroup. Additionally, the containsString helper function should be removed as it is now a redundant wrapper around slices.Contains.
Mechanical output of go fix ./... on both the root module and pkg/authjwt.
Five categories of pure-form modernization, identical semantics, no API
changes:
1. interface{} → any (Go 1.18+)
internal/service/{audit,authcode}.go
pkg/authjwt/{claims,verifier_test}.go
2. for i := 0; i < N; i++ → for range N (Go 1.22+)
tests/integration/{attestation_oidc,deactivation,identity,refresh_token_race}_test.go
3. manual loop → slices.Contains (Go 1.21+)
internal/service/credential_policy.go::containsString
internal/service/oauth.go (3 sites: client_credentials / authorization_code
/ refresh_token grant-allowlist checks)
pkg/authjwt/claims.go::HasScope
4. if x < 0 { x = 0 } → max(x, 0) (Go 1.21+ builtins)
internal/handler/identity.go::listIdentitiesOp (offset clamp)
5. wg.Add(1) + go func(){defer wg.Done(); ...}() → wg.Go(func(){...}) (Go 1.25)
tests/integration/refresh_token_race_test.go::TestRefreshTokenConcurrentRotation
Net: 11 files, +41/-71. Adds "slices" import in three files; no other
import changes.
Test plan
- [x] go vet ./... — clean (root + pkg/authjwt)
- [x] go test ./... -count=1 -timeout=180s — green, ~9.8s
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Mechanical output of
go fix ./...on both the root module andpkg/authjwt. Five categories of pure-form modernization to Go 1.21–1.25 idioms. No API or semantic changes. Net code reduction: 11 files, +41/−71.What changed
interface{}→any(Go 1.18+)internal/service/{audit,authcode}.go,pkg/authjwt/{claims,verifier_test}.gofor i := 0; i < N; i++→for range N(Go 1.22+)tests/integration/{attestation_oidc,deactivation,identity,refresh_token_race}_test.goslices.Contains(Go 1.21+)"slices"import in 3 filesinternal/service/credential_policy.go::containsString,internal/service/oauth.go(3 sites:client_credentials/authorization_code/refresh_tokengrant-allowlist checks),pkg/authjwt/claims.go::HasScopeif x < 0 { x = 0 }→max(x, 0)(Go 1.21+ builtins)internal/handler/identity.go::listIdentitiesOpwg.Add(1) + go func() { defer wg.Done(); ... }()→wg.Go(func() { ... })(Go 1.25)WaitGroup.Gomethodtests/integration/refresh_token_race_test.go::TestRefreshTokenConcurrentRotationThe
slices.Containsswaps add"slices"to imports in three files — no other import changes.Test plan
go vet ./...— clean (root +pkg/authjwt)go test ./... -count=1 -timeout=180s— green, ~9.8sgo fixScope and ordering
pkg/authjwt/claims.go. Both edits are mechanical (go fixhere changesinterface{}→any+slices.Contains; build(deps): upgrade lestrrat-go/jwx v2 → v4 + Go 1.26 #114 changes the v4 token accessors). Whichever lands second needs a small rebase.GO_VER). Thewg.Gochange requires Go 1.25 which the repo's already on.Why this is worth doing
interface{}you see in a future PR review will now be there because someone wrote it post-fix, not because it predates the alias).slices.Containsis more grep-able than ad-hoc loops and harder to get subtly wrong.wg.Goremoves theAdd(1)/defer Done()pairing footgun in concurrent test code.for range Nmatches what the project already uses for newer code; consistency.