Conversation
Signed-off-by: Babak K. Shandiz <babakks@github.com>
Signed-off-by: Babak K. Shandiz <babakks@github.com>
Add Reload, which re-reads the gh config files from disk and refreshes the config returned by Read in place. Keeping the cached *Config pointer stable lets every existing holder observe the new values, which keeps auth host and token resolution (which reads through Read) consistent with callers that reload under a lock. Reload is a package-level var, like Read, so consumers and tests can replace it. Lock deepCopy so the in-place entries swap cannot race a concurrent read. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f5c79efc-51d8-4913-8961-701522797f9e
babakks
force-pushed
the
babakks/add-refresh-token-support
branch
from
September 14, 2026 23:38
566cb7d to
2ce7921
Compare
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The Unix-socket dial hook ignores request cancellation despite adopting the context-aware API.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Adds in-place configuration reloads so existing *Config holders observe refreshed disk values safely.
Changes:
- Adds synchronized
config.Reloadbehavior and tests. - Replaces deprecated reflection and HTTP transport APIs.
- Adds locking during configuration copying.
| File | Description |
|---|---|
pkg/config/config.go |
Adds synchronized in-place reload support. |
pkg/config/config_test.go |
Tests cached configuration refresh behavior. |
pkg/api/http_client.go |
Migrates Unix-socket transport dial hooks. |
pkg/x/markdown/accessibility_test.go |
Replaces deprecated reflection constants. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+239
to
241
| dial := func(_ context.Context, network, addr string) (net.Conn, error) { | ||
| return net.Dial("unix", socketPath) | ||
| } |
williammartin
approved these changes
Oct 2, 2026
williammartin
left a comment
williammartin
left a comment
Member
There was a problem hiding this comment.
Reviewed in the context of the cli/cli refreshable-token stack. The in-place reload behavior supports the lock, reload, re-read, and persist flow used to avoid spending a stale rotating refresh token.
This branch has not been deployed
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.
Description
Lets
ghre-read its configuration from disk in place, which the refreshable-token stack incli/clineeds to load the latest stored credential before spending a single-use refresh token.The main addition is
config.Reload: it re-reads the gh config files and swaps the entries of the cached*Configunder a write lock, so every existing holder of that pointer observes the new values without being handed a new pointer. Likeconfig.Read,Reloadis a package-level var, so consumers and tests can replace it. Unwritten in-memory changes are discarded by the refresh, so callers persist first if they need them.deepCopynow takes the read lock so an in-place entries swap cannot race a concurrent read.Two small drive-by fixes replace deprecated APIs:
reflect.Ptrbecomesreflect.Pointer, and the unix-socket transport usesDialContext/DialTLSContextinstead of the deprecatedDial/DialTLS.How did you test this change?
Unit tests in
pkg/configcoverReload: it refreshes values in place, keeps the same*Configpointer, and discards unwritten changes. Existing tests pass.