[rhuss]

Originally NVIDIA/OpenShell#3284 and NVIDIA/OpenShell#3235 by @rhuss.

Summary

A gateway's metadata.json carries oidc_scopes, and the Rust CLI feeds it into every flow it starts: commands/gateway.rs reads metadata.oidc_scopes and passes it to both the interactive path and the client-credentials path.

The Go SDK reads that field in resolveClientCredentialsConfig only. Login and DeviceLogin silently ignored it and always used the built-in defaults (openid profile email), so a gateway registered with custom scopes got them for machine-to-machine auth but not for user login.

This resolves gateway scopes in both interactive flows using the same rule the client-credentials path already applies: they fill only genuinely-unset scopes, so an explicit WithScopes still wins.

Behavior change: a gateway with oidc_scopes set in metadata.json now affects Login and DeviceLogin from the Go SDK, matching what the CLI has always done.

Behavior changes

• WithTimeout(0) (or a negative duration) now means no deadline for Login, DeviceLogin and ClientCredentials; it used to mean the 2 minute default. NewClientCredentialsAuth still bounds every background exchange, by the 2 minute default when the timeout is not positive.
• ClientCredentials now applies WithTimeout (2 minutes by default) when the context has no deadline; it used to ignore the option.
• WithScopes() with no arguments now sends only openid from Login and DeviceLogin, and no scope parameter from the client credentials flows; it used to fall back to openid profile email.
• Login and DeviceLogin always request openid, even when WithScopes leaves it out.
• A gateway's oidc_scopes now applies to Login and DeviceLogin, not only to the client credentials flows.

Related Issue

No separate issue. This is a localized bug fix bringing the Go SDK in line with existing CLI behavior for a metadata field the SDK already parses, in the same spirit as NVIDIA/OpenShell#3235, which this PR also contains (see below).

Changes

• oidc.go: resolveOIDCConfig resolves gwCfg.OIDCScopes into unset scopes, mirroring credentials.go. Login now normalizes openid after gateway resolution rather than before, so scopes from metadata.json are normalized too.
• device.go: the same resolution in DeviceLogin's gateway block, with the same reordering.
• Tests: table-driven wire-level coverage for both flows (gateway scopes applied, gateway scopes gain openid, explicit WithScopes wins, empty oidc_scopes falls back to defaults). Extracted setupScopeCapturingDeviceProvider so the device tests share one mock provider.

Testing

• mise run go:ci green: build, golangci-lint (0 issues), gofmt, full go test -race, proto-check, docs-check.
• mise run pre-commit green.
• 8 new subtests across TestLogin_GatewayScopes and TestDeviceLogin_GatewayScopes.

Checklist

• Conventional Commit message, DCO sign-off
• Tests added and passing
• No public API signature changed
• Behavior change documented above

Included: honor explicitly-set zero option values in oidc defaults

The first commit comes from NVIDIA/OpenShell#3235, which this PR stacked on in core. Its description:

oidc.loginConfig.applyDefaults() decided whether to apply a default by inspecting the field's value (len(c.scopes) == 0, c.timeout == 0) rather than whether the caller had set it. As a result, a caller who explicitly passed a zero value had it silently replaced:

• WithTimeout(0) became the 2-minute default.
• WithScopes() (explicit empty) became the default scopes, even though WithScopes already records scopesSet.

This makes applyDefaults consult the *Set sentinels instead, so defaults fill only genuinely-unset fields. Unset behavior and non-zero explicit values are unchanged.

Follow-up to a review comment on NVIDIA/OpenShell#3232.

• oidc/options.go: add a timeoutSet sentinel (set by WithTimeout); applyDefaults now checks !scopesSet / !timeoutSet instead of the zero value.
• oidc/credentials_auth.go: guard the client-credentials token exchange so a zero timeout means "no deadline" rather than an already-expired context (matching the existing guards in Login and DeviceFlow).
• Tests: explicit empty scopes and explicit zero timeout are honored; unset still defaults; a client-credentials exchange with WithTimeout(0) succeeds instead of failing on a born-expired context.

Behavior change: WithTimeout(0) now means "no timeout" instead of the 2-minute default.

---

Port note

Ported from NVIDIA/OpenShell#3284 (which contains the commits of NVIDIA/OpenShell#3235) after the Go SDK moved out of NVIDIA/OpenShell at 8ed8bf0c907fd06572d616d64f22ede915d0c10f. The commits are the original ones from the core PR at 19567199934498358846d47b355e4bcb1e3ed279, with the same authors and sign-offs; the only change is the module path (github.com/NVIDIA/OpenShell/sdk/go is now github.com/ai-openshell/sdk-go). The core PR changed only sdk/go, so nothing stays in core.

• a0b0ba8 fix(sdk/go): honor explicitly-set zero option values in oidc defaults
• 17d562d fix(sdk/go): always request the openid scope in interactive oidc flows
• 951cf8b fix(sdk/go): honor gateway-configured oidc scopes in interactive flows

The commits after those (24af026, 8e7ae81, 27b075b, 62ca0cb) are new here and address review feedback.

File paths above are relative to the old sdk/go directory (here: openshell/v1/oidc/), and the Testing section refers to core's tasks (mise run go:ci is mise run ci here).

[rhuss]

cc-review Summary

What Went Well

• applyDefaults in openshell/v1/oidc/options.go (lines 55-66) now keys on the scopesSet/timeoutSet flags instead of zero values, so an explicitly empty scope list or an explicit timeout is finally distinguishable from "unset".
• requireOpenIDScope (options.go:78-87) is a single normalization step that builds a fresh slice, puts openid first and drops duplicates, so neither caller slices nor defaultScopes get mutated. It runs after all scope sources are resolved in Login (oidc.go:58) and DeviceLogin (device.go:87), and is deliberately skipped for client credentials.
• Gateway oidc_scopes only fill genuinely unset scopes (oidc.go:150-153, device.go:78-81), so an explicit WithScopes always wins over on-disk gateway metadata.
• accessTokenForExchange (credentials_auth.go:76-81) gets rid of the old born-expired context for negative timeouts, which made every client credentials exchange fail immediately.
• The new tests check what is actually sent on the wire: TestLogin_AlwaysRequestsOpenIDScope and TestLogin_GatewayScopes parse the authorization URL, and setupScopeCapturingDeviceProvider in device_test.go fails if the device request never happened. Precedence and empty-gateway fallback are covered with hermetic withGatewayResolver and t.TempDir() setups.
• Scopes are encoded through url.Values in buildAuthURL and requestDeviceCode, so whitespace-split gateway scope strings can't inject extra parameters.

Findings

All findings are Minor and posted as inline comments.

Severity	File	Description	Source
Minor	openshell/v1/oidc/credentials_auth.go:74	Comment says "zero timeout" and "DeviceFlow", but code treats any non-positive value as no deadline	architecture
Minor	openshell/v1/oidc/credentials_auth.go:76	Detached exchange has no deadline with WithTimeout(0); a blocking secret provider wedges auth	gator
Minor	openshell/v1/oidc/credentials_test.go:363	"No scope sent for WithScopes()" is only checked on config, not on the wire	test-quality
Minor	openshell/v1/oidc/device.go:78	Gateway scope fill block now duplicated in three places	architecture
Minor	openshell/v1/oidc/device_test.go:121	TestDeviceLogin_MissingIssuer doc comment now sits on the new scope test	test-quality
Minor	openshell/v1/oidc/options.go:63	WithTimeout(0) silently changes from the 2 minute default to no deadline	production
Minor	openshell/v1/oidc/options.go:157	WithTimeout doc now covers all login flows, but ClientCredentials ignores it	architecture
Minor	openshell/v1/oidc/options.go:157	Behavior changes to WithTimeout/WithScopes aren't called out; docs/src/api/oidc.md is stale	goal-alignment

Follow-up Progress

• Mode: full review. This is the first review of this PR, so there were no earlier conversations to carry forward.
• New material findings: none. Nothing reached Critical or Important after calibration.
• Limitations: one external reviewer (CodeRabbit) failed to run, so reviewer coverage for this run is incomplete and no completed review baseline is recorded.

Review Details

• Findings posted: 9 (as 8 inline comments; two reviewers reported the same misplaced doc comment in device_test.go, so those were combined)
• Findings reviewed and not posted: 0
• Gate outcome: PASS
• Participating reviewers: architecture, goal-alignment, production, test-quality, gator

Coverage incomplete: the CodeRabbit reviewer failed, so this run does not record a completed review baseline.

[Gkrumbach07]

Thanks for the follow-ups. I re-checked the four new commits on top of current main and against a real Keycloak, and the timeout and scope rules now hold for all four entry points:

• ClientCredentials and NewClientCredentialsAuth send no scope parameter without scopes, send exactly what WithScopes or the gateway's oidc_scopes names, and Keycloak accepts every case.
• DeviceLogin sends openid plus the gateway's scopes, and an explicit WithScopes wins.
• WithTimeout stops a slow token endpoint at the configured bound. A context deadline wins over it, and WithTimeout(0) means no deadline, except in the background exchange of NewClientCredentialsAuth, which gets 2 minutes.

The code, the doc comments and docs/src/api/oidc.md say the same thing. Build, vet, race tests and golangci-lint v2.12.2 are clean, and the two new timeout tests fail or hang on the code at 951cf8b. Measurements are below.

Call, with the token endpoint held for 3 s	Elapsed	Result
ClientCredentials, WithTimeout(1s)	1.00 s	error
ClientCredentials, WithTimeout(0)	3.01 s	ok
ClientCredentials, 500 ms context deadline and WithTimeout(10s)	0.50 s	error
NewClientCredentialsAuth, WithTimeout(1s)	1.00 s	error
NewClientCredentialsAuth, WithTimeout(0)	3.01 s	ok, with a 2m0s deadline on the exchange

I'm fine with WithTimeout(0) meaning "no deadline" before 1.0: an explicit zero is honored instead of being replaced, the option documents it, and no caller in this repository passes 0. My one concern is where the note lands. hack/automation/tag-release.sh builds the release notes from the body of the pull request that merged the stable pin bump, and bump-pr-body.md doesn't list SDK changes since the last tag. So the "Behavior changes" section of this description won't appear in the v0.1.8 notes unless someone copies it there. Two cheap ways to carry it:

• Squash this PR as fix(oidc)!: ... with a BREAKING CHANGE: footer naming WithTimeout(0), WithScopes() and gateway oidc_scopes. That matches the feat!: convention #4 used and keeps the note in git log. The title's sdk/go scope is also left over from core.
• Add the note to the next stable bump PR's body, or open a small follow-up that gives the bump template an "SDK changes since the last tag" section.

Three small nonblocking points follow. Two are on lines this PR doesn't change, so they are here rather than inline.

openshell/v1/oidc/credentials.go:142 (nonblocking): Now that ClientCredentials applies WithTimeout, a timeout comes back as oidc: client credentials exchange failed: token request failed, the same text as a refused connection. I measured this against Keycloak behind a slow proxy: errors.Is(err, context.DeadlineExceeded) and errors.Is(err, ErrTimeout) are both false, for WithTimeout(1s) and for a 500 ms context deadline. That matters because WithTimeout is new behavior for this entry point, and a caller has no way to tell "raise the timeout" from "the provider is down". Wrapping the context error when it is set is enough, and it carries nothing secret: if ctxErr := ctx.Err(); ctxErr != nil { return nil, fmt.Errorf("%w: token request failed: %w", ErrClientCredentials, ctxErr) }. The secret provider error at line 100 has the same shape. Returning ErrTimeout instead would also work, but errors.go:28 scopes that error to the interactive flows.

docs/src/api/oidc.md:138 (nonblocking): The sentinel table describes ErrTimeout as "Authentication flow timed out". The new Timeouts section says ClientCredentials and NewClientCredentialsAuth time out too, so a reader would expect errors.Is(err, oidc.ErrTimeout) to match them, and it doesn't. Either say "Interactive flow (browser, keyboard or device code) timed out", as errors.go and doc.go do, or make the client credentials flows return it.

[rhuss]

Thanks for re-checking against Keycloak. On the notes in the review:

• credentials.go:142: Fixed in 36b3211. Once the context has ended, exchangeClientCredentials wraps the context error for the secret provider, the token request and the response read. errors.Is(err, context.DeadlineExceeded) now matches for both WithTimeout and a context deadline. The provider's own error still stays out of the message. TestClientCredentials_TokenRequestTimeoutWrapsContextError covers the token request, and the existing bound test now checks the secret provider path too.
• docs/src/api/oidc.md:138: Fixed in 36b3211. The table now says "Interactive flow (browser, keyboard or device code) timed out", and the Timeouts section explains how to tell a client credentials timeout apart from a provider failure.
• Release notes: I retitled the PR to fix(oidc)!: ... and will squash with a BREAKING CHANGE: footer naming WithTimeout(0), WithScopes() and gateway oidc_scopes.

[rhuss]

@Gkrumbach07 would be great if you could give it another approval so that it will then get auto-merged.