Nit [CRF-11] The 5-line comment above TokenEndpointAuthMethodsSupported duplicates the godoc on codersdk.AllOAuth2TokenEndpointAuthMethods and pads the rest. (Gon P2)

From Gon: "codersdk/oauth2.go:273-275 already documents the shared-source invariant on the called function: 'Both Valid() and the discovery metadata are derived from it, so what registration accepts and what /.well-known advertises cannot drift apart.' The first two lines here restate that. The remaining three lines carry one fact ('not gated on dcrEnabled because existing public clients still exchange tokens') in five lines of prose."

Trim to the why-not-what unique to this site:

// Not gated on dcrEnabled: existing public clients still need to
// exchange tokens when new registrations are turned off.

Related: CRF-3 asks you to revise the same block's rationale because the "existing ones exchanging tokens" claim is not currently true for the tree; if you take that fix the trim rolls in for free.

🤖

P3 [CRF-3] Discovery advertises "none" in token_endpoint_auth_methods_supported, but the token endpoint at tokens.go:96-101 still appends client_secret is required and cannot be empty for every authorization_code grant with an empty secret and never inspects client_type. (Knov P2, Kurapika P3, Meruem P3, Melody P3, Mafuuu Note, Mafu-san Note, Pariston Note, Kite Note, Luffy Note)

From Kurapika: "A conforming client that trusts .well-known/oauth-authorization-server, registers with token_endpoint_auth_method=none, gets no secret in the response (correct per RFC 7591 §3.2.1), and then attempts the token exchange will hit client_secret is required at parameter parsing, before authorizationCodeGrant even runs."

From Knov on the in-file comment's rationale ("disabling registration stops new public clients being created but does not stop existing ones exchanging tokens, so the method remains supported"): "No existing public client can exchange tokens today, so that rationale is aspirational, not a description of the tree."

The follow-up PR named in the description will make this coherent, but reviewing this PR standalone, discovery is telling a conforming client something the server will not do. DCR is off by default (GetOAuth2DCREnabled), but discovery is served unconditionally.

Options:

• Hold "none" out of AllOAuth2TokenEndpointAuthMethods() until the token endpoint accepts it. Registration then rejects "none" (consistent with the token endpoint's current behavior) and this becomes a coherent no-op until the follow-up.
• Introduce a separate AdvertisedTokenEndpointAuthMethods() filtered by what the token endpoint executes.
• Land the discovery advertisement in the same PR as the token-endpoint change so the pair agrees at merge.

Also: revise the comment at metadata.go:39-42. As written, it justifies the choice with a state the tree does not currently produce.

🤖

Note [CRF-8] require.Contains(..., None) does not pin the drift-prevention invariant the metadata comment claims. (Bisky Note)

From Bisky: "The metadata.go comment says TokenEndpointAuthMethodsSupported is derived from AllOAuth2TokenEndpointAuthMethods() 'so discovery cannot advertise a method registration rejects, or hide one it accepts.' The test asserts only Contains(..., None). Hardcode the advertised list to []{None} and drop client_secret_basic and client_secret_post: registration still accepts them via Valid(), discovery hides them, this test still passes."

require.ElementsMatch(t, codersdk.AllOAuth2TokenEndpointAuthMethods(), metadata.TokenEndpointAuthMethodsSupported) costs a line and pins the actual invariant.

🤖

Note [CRF-10] RegisterPublicClient is added to a public test-support package but has no in-tree caller, and its signature diverges from the surrounding helpers on the ctx parameter. (Netero Note, Chopper Note, Gon Note, Luffy Note, Mafu-san Note, Razor Note, Hisoka Nit, Zoro Note+Nit)

From Netero: "grep -rn RegisterPublicClient --include='*.go' returns only the definition. The PR description names a follow-up PR in the stack as the intended caller (token-endpoint PKCE-only exchange). Netero flags unused exports without evaluating stack intent; if the follow-up is dropped, this stays as a small dead export in a test-support package."

From Zoro on the signature: "Every other helper defined in this file that needs a context creates it inline from t (CreateTestOAuth2App line 61, EnableDCR line 107, AuthorizeOAuth2App via doAuthorizeRequest line 148, ExchangeCodeForToken line 229, FetchOAuth2Metadata line 347, PerformTokenExchangeExpectingError line 296). Taking ctx from the caller is not wrong on its own; it is only inconsistent with the pattern the file has already committed to."

From Mafu-san on the risk: "Adding a test helper without exercising it means a bug in the helper (wrong RedirectURIs field name, wrong request shape, wrong assertion) rides through this PR uncaught; a caller in the next PR would surface it late and blame the wrong change."

Options: (a) fold the helper introduction into the PR that first uses it, or (b) add one minimal exercise here (register through EnableDCR, assert resp.ClientSecret == ""), and align the signature to func(t *testing.T, ...)-first with ctx created inline like the neighbors.

🤖

Nit [CRF-13] // Generate client credentials. narrates the mechanism of the two lines that follow it. (Gon Nit)

From Gon: "The payload of the block is the second sentence ('Public clients authenticate with PKCE alone and never receive a secret'). Drop the leading section-header sentence and keep the rationale:

// Public clients skip secret generation and authenticate with PKCE
// alone (RFC 7591 §2, OAuth 2.1 §2.1).
```"

> 🤖

Note [CRF-15] now := dbtime.Now() and uuid.New() for the secret ID are captured outside the InTx closure. (Pariston Note)

From Pariston: "Not currently a bug: InTx(fn, nil) runs at the driver's default isolation and does not retry on serialization failure, so the captured values are used exactly once. Worth knowing because if anyone later passes a TxOptions with retry semantics, or wraps this in a retry loop, the same secret ID would be reused across attempts and the second attempt would fail on uniqueness. Moving the uuid.New() and dbtime.Now() calls inside the closure would remove that footgun for the cost of two lines."

🤖

Nit [CRF-7] The em-dash → comma cleanup is applied to the two //nolint:gocritic comments this change touched but not to the ten sibling instances in the same file and package. (Kite Nit)

From Kite: "Six identical // OAuth2 system context — RFC 7592 client configuration endpoint comments remain in registration.go at lines 227, 313, 338, 421, 445, 497, and four more in tokens.go at lines 261, 289, 451, 488. scripts/check_emdash.sh only scans added lines in the diff, so these siblings sit under the linter's radar but violate the same rule the change is enforcing on the lines it touched."

One-character change each. Doing them now stops the class re-appearing in a future diff against those lines.

🤖

P3 [CRF-5] The fmt.Sprintf → accessURL.JoinPath fix has no regression test. (Mafu-san P3, Chopper Nit, Meruem Nit, Ryosuke Note)

From Mafu-san: "None of the three new tests uses a trailing-slash accessURL... Revert accessURL.JoinPath(\"/oauth2/clients\", clientID.String()) to the old fmt.Sprintf and every test still passes. The fix is real (verified against the code) but the regression guard is not."

The PR description names this fix explicitly ("a latent bug where an access URL configured with a trailing slash would mint //oauth2/clients/{id} as the client's management endpoint"), so it is worth pinning. Add one subtest row that sets accessURL to https://example.com/ (trailing slash) and asserts the response's RegistrationClientURI contains exactly one slash between authority and /oauth2/clients/. Same pattern would fail against path.Join as well.

🤖

P3 [CRF-6] // Create client secret - parse the formatted secret to get components mislocates the operation. (Gon P2)

From Gon: "'Create client secret' is wrong at this location. The secret was already generated on line ~82 by generateClientCredentials(). The block that follows parses that string to pull the prefix out, then inserts a DB row for it. It creates a row, not a secret. 'parse the formatted secret to get components' is what the function name ParseFormattedSecret says."

A future maintainer reading this hunts for a secret-minting call that is not there. Delete or replace with one line naming what the parse is for (extracting Prefix for InsertOAuth2ProviderAppSecretParams.SecretPrefix).

🤖