P2 [CRF-1] AllOAuth2TokenEndpointAuthMethods doc claims discovery metadata derives from it, but discovery does not read this function; the drift the comment says cannot happen is present today. (Mafu-san P2, Gon P2, Knov P2, Netero P3, Mafuuu P3, Leorio P3, Razor P3, Meruem P3, Ryosuke P3, Kurapika Nit, Kite Nit, Zoro Nit)

Both Valid() and the discovery metadata are derived from it, so what registration accepts and what /.well-known advertises cannot drift apart.

Only the first half is true. Valid() at line 285 now calls slices.Contains(AllOAuth2TokenEndpointAuthMethods(), m) and accepts {basic, post, none}. The discovery handler at coderd/oauth2provider/metadata.go:39 hardcodes []codersdk.OAuth2TokenEndpointAuthMethod{codersdk.OAuth2TokenEndpointAuthMethodClientSecretBasic, codersdk.OAuth2TokenEndpointAuthMethodClientSecretPost} and never calls this function. A POST /oauth2/register with token_endpoint_auth_method=none succeeds; a GET /.well-known/oauth-authorization-server on the same server does not advertise none. A spec-following client that reads discovery first concludes public clients are unsupported even though registration would accept one.

Worse, the stated shape conflicts with the current advertise-vs-accept split: AllOAuth2TokenEndpointAuthMethods() returns the accept-set including none, but discovery deliberately does not advertise none until public clients are wired end-to-end. Making these "the single source" forces one of two wrong outcomes: advertise none before the token endpoint honors it, or drop none from the accept-set.

Fix in this PR is one of two things, no third option: (1) wire TokenEndpointAuthMethodsSupported: codersdk.AllOAuth2TokenEndpointAuthMethods() in metadata.go so the doc becomes true, or (2) rewrite the doc to what today actually is (e.g. Valid() derives from it. Discovery metadata will derive from it once the follow-up PR wires it in; until then, /.well-known does not advertise "none" even though registration accepts it.)

🤖

P3 [CRF-2] RFC 7592 UpdateClientConfiguration silently converts a public client to confidential when the request omits token_endpoint_auth_method. (Hisoka P3, Kurapika Note, Razor Note)

The handler runs Validate(), then ApplyDefaults() at line 288, then writes ClientType: string(req.DetermineClientType()) at line 324 and TokenEndpointAuthMethod: sql.NullString{String: string(req.TokenEndpointAuthMethod), Valid: true} at line 328. ApplyDefaults maps an omitted method to client_secret_basic, so a public client (registered with token_endpoint_auth_method: "none") that PUTs anything without re-sending the method lands here with method "client_secret_basic" and client_type "confidential". Hisoka reproduced this on the PR head:

registered: method="none" client_type="public"
after PUT:  method="client_secret_basic" client_type="confidential"

client_type is ActionTrack in enterprise/audit/table.go:320, so the flip writes an audit entry claiming the client asked to change its own type. Class-wise this is the same "hardcoded confidential swallowed a public request" shape the PR just fixed at CreateDynamicClientRegistration (registration.go:104); the fix has not reached the sibling handler. Once the follow-up wires IsPublic() into token endpoint auth, this silent flip flips the client's auth model too: a public client that ever PUTs anything without re-sending "none" gets locked into needing a secret it was designed not to hold.

DetermineClientType's own doc already warns this shape ("a caller that also compares the request's auth method against a stored one must apply defaults first, or an omitted field compares as "" and looks like a change the client did not request") but points at Validate; the actual write is here.

Fix in this handler: when the request omits token_endpoint_auth_method, inherit the existing app's method before deriving client_type and re-serializing, or reject PUTs that would change client_type.

🤖

P3 [CRF-3] OAuth2ClientType doc says the token endpoint reads it; today no code does. (Mafu-san P3, Leorio P3)

Type comment ends with:

It is derived from the requested token_endpoint_auth_method and stored on the app, and the token endpoint reads it to decide whether a client secret is required.

coderd/oauth2provider/tokens.go never touches ClientType or IsPublic; it unconditionally requires req.ClientSecret != "" at line 96. grep -rn '\.IsPublic()' --include='*.go' under coderd/oauth2provider/ returns zero.

The PR body correctly discloses the deferral ("app registration itself do not yet skip secret issuance for a public client; that follows in the next PR in the stack"), so the reasoning is right; the artifact reads as if the hookup already exists. A developer opening IsPublic or OAuth2ClientType for the follow-up PR greps for the caller the doc names, finds nothing, and doubts either the tools or their reading.

Say the truth: A follow-up PR wires the token endpoint to read this when deciding whether to require a client secret.

🤖

P3 [CRF-4] The "do not delete" pin names TestCreateDynamicClientRegistration_ClientType, which does not exist in this PR. (Leorio P3, Ryosuke P3, Gon P2, Bisky Nit, Hisoka Nit, Pariston Nit, Mafuuu Nit, Razor Nit, Meruem Nit, Kite Nit, Zoro Nit)

the tests that pin these spellings to the wire values do, so do not delete them as redundant:
TestOAuth2ClientRegistrationRequest_DetermineClientType and
TestCreateDynamicClientRegistration_ClientType.

grep -rn TestCreateDynamicClientRegistration_ClientType --include='*.go' returns only this comment. The closest neighbor, TestCreateDynamicClientRegistration_DCREnabled at coderd/oauth2provider/registration_test.go:29, does not touch client_type. The actual DB-side wire-spelling guard is TestOAuth2ProviderAppIsPublic in coderd/database/modelmethods_internal_test.go:229 (this PR).

The block explicitly warns "do not delete them as redundant": leaving a load-bearing citation to a test that was never written invites a future editor to delete the constants ("the guard doesn't exist") or wastes a shift searching for a phantom.

Drop-in:

// ... the tests that pin these spellings to the wire values do, so do not
// delete them as redundant:
// TestOAuth2ClientRegistrationRequest_DetermineClientType (codersdk) and
// TestOAuth2ProviderAppIsPublic (coderd/database).

Or add the DB-round-trip test the current name implies (register with each auth method, read the row back, assert the stored client_type string).

🤖

P3 [CRF-5] Comment calls client_type "the nullable column," but the column is NOT NULL after migration 000565. (Gon P2, Knov Nit, Meruem Nit)

coderd/database/dump.sql:2655 declares client_type text DEFAULT 'confidential'::text NOT NULL, and migration 000565_oauth2_client_type_constraint.up.sql:19 ran ALTER COLUMN client_type SET NOT NULL. The sqlc-generated field on OAuth2ProviderApp is ClientType string, not sql.NullString. The reason to declare these constants as string (rather than codersdk.OAuth2ClientType) is that the model's field is string, so a typed comparison would require a cast at every callsite. The "nullable column" framing is not just wrong about the schema, it misleads the next reader about what invariant IsPublic is enforcing: it guards against an unrecognized Go-side value, not a NULL from the DB.

Rewrite: Values stored in oauth2_provider_apps.client_type, as plain strings for comparison against the sqlc-generated string column.

🤖

Nit [CRF-10] Table-driven cases key on tt.clientType; empty and whitespace cases render as anonymous or ambiguous sub-test names. (Kite)

The struct has no name field; t.Run(tt.clientType, ...) uses the input as the sub-test name. For {clientType: ""} this produces an anonymous sub-test in go test -v output, and the whitespace cases " public" and "public " produce sub-test paths that do not visibly distinguish which side the space is on without inspecting the source. Add a name field with human-readable case names (Empty, LeadingSpace, TrailingSpace, MixedCasePublic, AllCapsPublic, ...) so a failure line points at what broke without needing to count into the table.

🤖

Nit [CRF-11] Commit subject feat(codersdk,coderd/database): add OAuth2ClientType and derive it from auth method has two AGENTS.md issues. (Leorio)

• Scope must be a real filesystem path containing every changed file, or omitted for cross-cutting changes (AGENTS.md). This commit also touches coderd/oauth2provider/apps.go, coderd/oauth2provider/registration.go, and site/src/api/typesGenerated.ts; the declared scope covers neither coderd/oauth2provider/ nor site/. Omit the scope, since the change is cross-cutting.
• 84 chars, over 72.

Suggested rewrites, either one:

• feat: derive OAuth2 client type from token_endpoint_auth_method (62)
• feat: add OAuth2ClientType, derive from auth method (52)

The rest of the commit body is a real chart note. Leave that alone.

🤖

Nit [CRF-12] // authMethod is the requested token_endpoint_auth_method. restates the field name and type. (Gon)

The field on the next line is authMethod codersdk.OAuth2TokenEndpointAuthMethod. The comment adds no invariant, no why-not-what, no trap: the code already teaches a reader everything the comment does. Delete the line. The neighbouring applyDefaults comment carries a real why-not-what (matches the real request path) and should stay.

🤖

Note [CRF-13] DetermineClientType is called from Validate() before ApplyDefaults() and from registration after; the "same result before and after defaults" invariant is documented but not test-pinned for the public path. (Meruem)

codersdk/oauth2_validation.go:21 calls req.DetermineClientType() inside Validate(), which the handlers in registration.go:66,281 run before ApplyDefaults(). The stored-column call at :104,324 runs after. Correctness relies on ClientTypeFor producing the same class for the empty auth method and for the defaulted client_secret_basic; today both fall through to confidential and DetermineClientType's doc calls this out explicitly.

This file covers the omitted-and-defaulted case and the omitted-and-not-defaulted case, but only for the confidential path; there is no case that runs ApplyDefaults() first with TokenEndpointAuthMethod = "none" to pin that none stays public across defaults. If ApplyDefaults ever grows a rule that rewrites none (a normalization step, for example), the validation call and the storage call would silently disagree, and the failure mode is a redirect URI validated under one rule set and stored under the other.

Add a case: {name: "NoneStaysPublicAfterApplyDefaults", applyDefaults: true, authMethod: OAuth2TokenEndpointAuthMethodNone, expectedType: "public"}.

🤖

Note [CRF-14] OAuth2ProviderApp.IsPublic() and codersdk.OAuth2ClientType.Valid() have no production caller in this PR. (Netero, Razor)

Per the PR description, IsPublic() is consumed by the token endpoint / registration in the next PR in the stack (ENG-3029). OAuth2ClientType.Valid() follows the local convention of sibling untested enum Valid() methods in the same file. Flagged here only so a downstream reviewer does not re-discover them; not a defect on its own given the linked follow-up.

🤖