Repository navigation
fix: correct the defects a Go style review found - #95
Merged
Merged
Conversation
Two reviews against the Google Go Style Guide, by Codex and Claude Fable, found real defects alongside the style points: - oauth2.NewClient, the README's integration, wrapped this package's cache in its own, which refreshes 10s before expiry, so the 30s skew never applied. ReuseTokenSourceWithSkew is now oauth2's cache with the longer window, which NewClient reuses as is. It caches copies of the tokens, since oauth2 writes its window into each one and caches sharing a source's token raced on that. A non-positive skew is still exactly oauth2.ReuseTokenSource. - A huge Retry-After overflowed to a negative wait, read as retry now. - An ID past int64 in the environment saturated instead of being reported as malformed. - WithApplicationTokenExpiration wrote to its captured value when applied, racing when one option was shared. - --json or --agent after a malformed flag was never read, so the failure came out as prose. - version and help exited 0 when stdout failed; three CLI errors used %v, hiding their cause from errors.Is. - Identifier admits ~int, so NewApplicationTokenSource(12345, key) compiles. gofmt is now enforced by golangci-lint, CI runs the tests with -race, and doc comments that named missing types, overpromised option order or misdescribed oauth2's refresh are corrected.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #95 +/- ##
==========================================
+ Coverage 97.68% 97.72% +0.03%
==========================================
Files 9 9
Lines 908 923 +15
==========================================
+ Hits 887 902 +15
Misses 21 21 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
PR1 deleted the wiring tests that were WithExpirySkew's only coverage, and left three branches of scanOutputFlags untested, which together dropped project coverage below main's. A behaviour test now shows the skew is the window the JWT cache refreshes on, and four CLI cases pin the scan: --agent after a bad flag, an explicit --json=true, a --json past a positional argument, and a --json value that is not a bool.
jferrl
added a commit
that referenced
this pull request
Oct 31, 2026
# Conflicts: # cmd/githubauth/main_test.go
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
Two independent reviews of the repository against the Google Go Style Guide, by Codex (
gpt-6.1-sol,ultra) and Claude Fable, found ten real defects alongside the style points. The one that matters most:oauth2.NewClient, the integration the README recommends, put its own cache in front of this package's, so tokens were refreshed 10s before expiry instead of the documented 30s. This PR fixes the ten defects and the doc comments that were wrong. The refactor and test cleanup the reviews asked for come in a second PR.What this does
oauth2.NewClient(ctx, src)with a 30s skewRetry-After: 9223372036854775807-1s, retried at onceGITHUB_APP_INSTALLATION_ID=9223372036854775808token --bogus --jsonversionwith a broken stdoutNewApplicationTokenSource(12345, key)Changes
ReuseTokenSourceWithSkewreturnsoauth2.ReuseTokenSourceWithExpiry, whichNewClientrecognises. It caches copies of the tokens: oauth2 writes its window into each token it holds, and caches sharing one source's token raced on that. A zero or negative skew is still exactlyoauth2.ReuseTokenSource.WithApplicationTokenExpirationnormalises its value once, so sharing one option across goroutines no longer races.--json/--agent, skipping flag values. Three errors wrap with%w;versionandhelpreport write failures.Identifieradmits~int..golangci.ymlenables gofmt (main.go was not formatted, from fix: close the credential leaks two security reviews found #93), and CI runsgo test -race.alga claim, overpromised option order, or said oauth2 refreshes only at expiry are corrected; the webhook constants and errors get their own comments.Rationale
Building on oauth2's cache, not keeping a custom one, is what lets
NewClientreuse it; oauth2 only skips wrapping its own type. Every fix has a test that fails with the fix undone. An adversarial Codex pass over this PR found three more issues, fixed: a zero skew double-cached an existing oauth2 cache,--json=was read as on, and theWithContextdoc overpromised.Migration Notes
None.
ReuseTokenSourceWithSkewkeeps its signature; only its unexported return type changed.