Conversation
Contributor
HanXHX
commented
May 14, 2026
HanXHX
commented
- Fix misleading comment on fetchOIDCCached concurrency (C1)
- Document that returned OIDCConfig pointer must not be mutated (C2)
- Skip server-side revocation in Logout when baseURL is empty, avoiding a noisy second warning after a config load failure (C3)
- Update GetValidToken docstring to document the RETYC_TOKEN fallback-to-disk behaviour, and emit a warning to stderr when the fall-through actually triggers (C4)
- Fix misleading comment on fetchOIDCCached concurrency (C1) - Document that returned OIDCConfig pointer must not be mutated (C2) - Skip server-side revocation in Logout when baseURL is empty, avoiding a noisy second warning after a config load failure (C3) - Update GetValidToken docstring to document the RETYC_TOKEN fallback-to-disk behaviour, and emit a warning to stderr when the fall-through actually triggers (C4)
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the auth/OIDC flow to address prior review comments by clarifying concurrency semantics and improving user-facing behavior around logout and token selection (env token vs disk token).
Changes:
- Clarifies
fetchOIDCCachedconcurrency expectations and documents that the returned*OIDCConfigmust not be mutated. - Avoids attempting server-side session revocation during
LogoutwhenbaseURLis empty. - Documents
RETYC_TOKENfallback-to-disk behavior inGetValidTokenand emits a warning when fallback is triggered.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/service/auth.go | Updates cached OIDC config documentation and adjusts logout revocation behavior when baseURL is empty. |
| internal/auth/oidc.go | Expands GetValidToken documentation and adds a stderr warning when RETYC_TOKEN is invalid and the code falls back to disk token logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
203
to
206
| tok, lerr := config.LoadToken() | ||
| if lerr == nil && tok.RefreshToken != "" { | ||
| if lerr == nil && tok.RefreshToken != "" && baseURL != "" { | ||
| oidcCfg, oerr := fetchOIDCCached(ctx, baseURL, httpClient) | ||
| if oerr != nil { |
Comment on lines
381
to
+385
| if !errors.Is(err, ErrNoRefreshToken) { | ||
| return nil, fmt.Errorf("RETYC_TOKEN refresh failed: %w", err) | ||
| } | ||
| // ErrNoRefreshToken (invalid_grant): fall through to disk token. | ||
| fmt.Fprintf(os.Stderr, "warning: RETYC_TOKEN is expired or revoked, falling back to stored disk token\n") | ||
| // Fall through to the disk token path below. |
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.