Repository navigation
Refreshable tokens: build-gated debug overrides for testing token expiry - #14752
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Debug-only behavior needs HTTP-client logging regression tests and a tagged CI test invocation.
2 open issues
What changed in this PR
Moves refreshable-token testing overrides behind the ghdebug build tag, keeping them unavailable in regular builds.
Changes:
- Adds optional debug configuration and a
make debugtarget. - Marks debug binaries in version output.
- Updates guided scripts and expands refresh assertions.
Review effort: Balanced
williammartin
left a comment
There was a problem hiding this comment.
Hey @babakks,
Sorry, I have to leave as my day is over. This is a slightly sloppy review starting point 😅
I feel 3/10 strongly about the testing comments. However, I feel strongly about the rebuild decision because plain make should reliably produce a regular build.
The PR description is very helpful, especially the walkthrough and reviewer notes.
There was a problem hiding this comment.
Suggestion
Non blocking, and I feel 3/10 strongly about this. Could we run the ghdebug variant in CI so the tagged implementation cannot silently stop compiling?
$ git show origin/pr-14752:.github/workflows/go.yml | grep 'go test'
run: go test -race -tags=integration ./...The regular run should stay so we also cover the disabled implementation. What do you think?
There was a problem hiding this comment.
Good point, thanks! 🙏 I took a slightly different route that makes a tagged test run unnecessary.
In 8f43bec I moved the environment parsing out of config_enabled.go into an unexported configFromEnv in the untagged config.go. That way config_test.go covers every parsing and panic case in the regular test run, on every OS. What is left behind the tag is just the choice of what Config returns:
ghdebug:return configFromEnv()!ghdebug:return DebugConfig{}, whichTestConfigIgnoresEnvironmentalready asserts in the regular run
Since the release Config never references configFromEnv, the linker drops it from regular builds. I checked the resulting binaries for linux/amd64, windows/amd64, windows/arm64, darwin/amd64, and darwin/arm64: every regular build has no GH_INTERNAL_ strings and no parser symbols at all (only the Config stub), while every ghdebug build has all three variable names along with configFromEnv and lifetimeFromEnv.
So the only way the debug variant can break without the regular run noticing is by failing to compile, and d5d4d81 covers that with a go build -tags=ghdebug ./cmd/gh step on the Ubuntu runner. Running the whole suite again with the tag would buy us nothing extra, and since Go caches test results per tag set it would rerun every package on every OS.
3423139 to
d1792c3
Compare
Add an internal/debug package that exposes Config, which returns a DebugConfig holding internal debugging overrides as Option values, and Enabled, which reports whether gh was built with the ghdebug build tag. In ghdebug builds Config reads GH_INTERNAL_AT_EXPIRES_IN and GH_INTERNAL_RT_EXPIRES_IN as token lifetimes in seconds and GH_INTERNAL_DEBUG_REFRESH_TOKEN as a boolean, leaves unset overrides as None, and panics on invalid values. In regular builds every override is always None, so the overrides cannot be enabled in released binaries. The tag is prefixed with gh to avoid colliding with a generic debug tag. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ac178b5a-5b62-49bd-9e83-4796c7ddec46
Add "make debug", which builds bin/gh like "make" but with the ghdebug build tag so the internal debug overrides are available. It removes bin/gh first because script/build skips rebuilding when no source file has changed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ac178b5a-5b62-49bd-9e83-4796c7ddec46
Append "[debug build]" to the first line of gh version and gh --version when gh is built with the ghdebug build tag, so a debug binary is easy to tell apart from a regular one. Regular builds are unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ac178b5a-5b62-49bd-9e83-4796c7ddec46
Replace the ad hoc os.Getenv testing hacks with the build-gated internal/debug configuration. The access and refresh token lifetime overrides in internal/config and the verbose refresh logging switch in the factory now come from GH_INTERNAL_AT_EXPIRES_IN, GH_INTERNAL_RT_EXPIRES_IN, and GH_INTERNAL_DEBUG_REFRESH_TOKEN, and have no effect in builds without the ghdebug tag. Update the refreshable-token e2e scripts to use the new variable names, rename their own settings to GH_INTERNAL_E2E_BIN, GH_INTERNAL_E2E_HOST, and GH_INTERNAL_E2E_ALLOW_VSCODE, and refuse to run unless gh version reports a debug build, pointing at "make debug" otherwise. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ac178b5a-5b62-49bd-9e83-4796c7ddec46
Check that gh auth token prints the expiry warning on stderr whenever it prints a stored expiring token, and that the warning is absent when no stored expiring token is printed, such as with GH_TOKEN. Also run gh auth refresh both with and without --expiring and assert that each receives an expiring refreshable token. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ac178b5a-5b62-49bd-9e83-4796c7ddec46
script/build skipped rebuilding bin/gh whenever no source file was newer than the binary, regardless of build tags. A plain `make` straight after `make debug` therefore kept the debug binary. Compare the tags recorded in the existing binary's build info with GO_BUILDTAGS and rebuild when they differ. This also lets `make debug` drop its `rm -f bin/gh` workaround. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ac178b5a-5b62-49bd-9e83-4796c7ddec46
Move the environment parsing into an unexported configFromEnv in the untagged config.go. The ghdebug implementation of Config now just calls it, while the release implementation still returns an empty DebugConfig. This lets the regular test run cover all parsing and panic cases on every OS without the ghdebug tag. The tagged test only checks that Config reads the real environment. Release builds never reference configFromEnv, so the linker still drops the parser and the variable names from them. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ac178b5a-5b62-49bd-9e83-4796c7ddec46
The ghdebug implementation of internal/debug is not compiled by the regular test run or build, so it could silently stop compiling. Build ./cmd/gh with the tag on the Ubuntu runner to catch that. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ac178b5a-5b62-49bd-9e83-4796c7ddec46
d1792c3 to
a1d1667
Compare


Description
This PR sits on top of #14745 in the refreshable-token stack. Testing refresh behavior by hand is awkward because GitHub issues expiring access tokens with a lifetime of 8 hours and refresh tokens with a lifetime of 6 months, so there is no practical way to wait for a real expiry. Until now the stack relied on ad hoc
os.Getenvhacks (GH_AT_EXPIRES_IN,GH_RT_EXPIRES_IN, andGH_DEBUG_REFRESH_TOKEN) that were compiled into every build, including release binaries.This replaces those hacks with a small
internal/debugpackage.debug.Config()returns adebug.DebugConfigvalue whose fields areOptionvalues, one per override, read from the environment only when gh is built with theghdebugbuild tag. An unset override isNone, so an explicit zero orfalseis still distinguishable from no override. The tag is prefixed withghso it cannot collide with a genericdebugtag that a dependency or another tool might use. In a regular build the package compiles to a stub that always returns an emptyDebugConfigwith every overrideNone, so the overrides do not exist in released binaries and cannot be switched on by setting an environment variable. The parsing itself lives in an unexported, untaggedconfigFromEnv, so the regular test run covers it on every OS; only theghdebugimplementation ofConfigcalls it, and the linker drops it from regular builds.The overrides, all prefixed
GH_INTERNAL_to make it clear they are not a supported interface:GH_INTERNAL_AT_EXPIRES_IN: access token lifetime in seconds, measured from when the token was issued. With1, any stored access token is already treated as expired, so the next use triggers a refresh.GH_INTERNAL_RT_EXPIRES_IN: refresh token lifetime in seconds, measured the same way. When it is exceeded, gh behaves as if the server rejected the refresh token: it clears the stored credential and asks the user to rungh auth login.GH_INTERNAL_DEBUG_REFRESH_TOKEN: a boolean. By default the refresh exchange is logged as head lines only underGH_DEBUG=api; set it totrueto log the full refresh request and response.An invalid value panics in a debug build, so a typo cannot silently fall back to the real lifetime.
To make getting a debug binary easy, there is a new
make debugtarget. It does the same asmakebut passes theghdebugbuild tag, producingbin/ghwith the overrides enabled. The build script now also compares the build tags embedded in an existingbin/ghwithGO_BUILDTAGS, so switching betweenmakeandmake debugalways rebuilds, while repeating the same one still skips an up to date binary. CI also builds./cmd/ghwith-tags=ghdebugon the Ubuntu runner, so the debug variant cannot silently stop compiling.A debug binary identifies itself in its version output, so it is easy to tell which kind of binary you are running:
The two guided e2e scripts (
script/refreshable-token-e2e.shandscript/refreshable-token-gitcredential-e2e.sh) now use theGH_INTERNAL_names, checkgh versionfor the[debug build]marker and refuse to run against a regular binary, and point atmake debug. Their own settings are renamed to match:GH_BIN,GH_E2E_HOST, andGH_E2E_ALLOW_VSCODEbecomeGH_INTERNAL_E2E_BIN,GH_INTERNAL_E2E_HOST, andGH_INTERNAL_E2E_ALLOW_VSCODE.The main e2e script also checks more of the behavior from earlier PRs in the stack. Every
gh auth tokenstep that prints a stored expiring token now asserts that the expiry warning appears on stderr, and the steps that print a non-expiring token (such as one fromGH_TOKEN) assert that it does not. There is also a newgh auth refreshstep without--expiring, next to the existing one with it, and both assert that gh asked for and received an expiring refreshable token.Trying it out
Build a debug binary and log in with an expiring token:
Then force the access token to count as expired and make any API call with
GH_DEBUG=api:A refresh shows up in stderr as a request to the OAuth token endpoint, before the
api/userrequest:Running the same command without
GH_INTERNAL_AT_EXPIRES_INshows no such line, because the freshly refreshed token is still valid.bin/gh auth statusalso shows the new expiry time. AddGH_INTERNAL_DEBUG_REFRESH_TOKEN=trueto see the full refresh exchange instead of just its head lines.To see what happens when the refresh token itself has expired (this logs you out of the host in gh):
For a guided walkthrough of every scenario, run
script/refreshable-token-e2e.shorscript/refreshable-token-gitcredential-e2e.shaftermake debug.How did you test this change?
I ran
makeandmake debugand compared the two binaries.bin/gh versionprintedgh version 2.100.0-180-g76df8f52a (2026-10-19)for the regular build and the same line with a trailing[debug build]for the debug build, andgh --versionmatched in both cases.go version -m bin/ghreported-tags=ghdebugfor the debug build. I also ranmake debug,make,make,make debug,make debugin sequence: every switch between the two rebuilt with the right tags, and every repeat reportedbin/ghas up to date. I also built regular andghdebugbinaries for linux/amd64, windows/amd64, windows/arm64, darwin/amd64, and darwin/arm64: no regular build contains anyGH_INTERNAL_string or theconfigFromEnvandlifetimeFromEnvsymbols, while everyghdebugbuild contains all of them.I ran
script/refreshable-token-e2e.shagainst the debug binary and all scenarios passed, including the newgh auth tokenwarning checks and bothgh auth refreshsteps, with and without--expiring.I ran
script/refreshable-token-gitcredential-e2e.shagainst the debug binary in an Ubuntu 25.10 container with git 2.51.0, so the git 2.46+authtypecredential capability path was exercised, and all scenarios passed.Key points
GH_INTERNAL_IS_DEBUGvariable: a release binary has no code that readsGH_INTERNAL_*variables, so it could not respond to one without adding such code to release builds.GH_INTERNAL_AT_EXPIRES_IN=3600therefore means "pretend the token was issued with a one hour lifetime", which keeps the behavior consistent across repeated invocations.make debugwrites to the samebin/ghasmake, so the two builds replace each other rather than coexisting.Notes for reviewers
Start with
internal/debug/config.go(theDebugConfigtype and the untaggedconfigFromEnvparser) and its two build-tagged implementations (config_enabled.goandconfig_disabled.go, which pick whatConfigreturns and definedebug.Enabled), thenpkg/cmd/version/version.gofor the version marker, then the call sites ininternal/config/refreshable.go(accessTokenNeedsRefreshandsimulateRefreshTokenExpired) andpkg/cmd/factory/default.go(tokenRefresherHttpClientFunc). In the scripts, the debug check isrequire_gh, and apart from the renames the only new logic is theauth tokenwarning andauth refreshassertions inscript/refreshable-token-e2e.sh.Authorship and follow-up
Who wrote this:
Who answers review comments: