From 56f2415c4dcacf0e22630ae8fd46c6b353dd59f6 Mon Sep 17 00:00:00 2001 From: pikann22 Date: Wed, 28 Oct 2026 07:57:15 +0000 Subject: [PATCH 1/2] feat: enhance capabilities structure with KindProfile and profile method for identity kinds --- gitprovider/conformance/conformance.go | 46 +++++++++++++++++++- gitprovider/gitprovider_test.go | 15 +++++++ gitprovider/testdata/wire-shapes.golden.json | 16 +++++++ gitprovider/types.go | 33 +++++++++++++- 4 files changed, 107 insertions(+), 3 deletions(-) diff --git a/gitprovider/conformance/conformance.go b/gitprovider/conformance/conformance.go index 1c751f7..96d1306 100644 --- a/gitprovider/conformance/conformance.go +++ b/gitprovider/conformance/conformance.go @@ -65,6 +65,9 @@ type WebhookCase struct { // Fixtures describe the backend's seed data. type Fixtures struct { Identity gp.IdentityRef + // IdentityKind is the kind of Identity, for providers that support several + // (their per-kind profiles differ). Empty: the first of IdentityKinds. + IdentityKind gp.IdentityKind // Repo is a repository the identity can see, holding the change request, // issue and thread below. Repo gp.RepoRef @@ -257,6 +260,32 @@ func testDescribe(t *testing.T, h Harness) { t.Error("CIConfigGlobs must not contain empty patterns") } } + declared := map[gp.IdentityKind]bool{} + for _, k := range c.IdentityKinds { + declared[k] = true + } + profiled := map[gp.IdentityKind]bool{} + for _, kp := range c.Kinds { + switch { + case !declared[kp.Kind]: + t.Errorf("Kinds names %q, which is not in IdentityKinds", kp.Kind) + case profiled[kp.Kind]: + t.Errorf("Kinds lists %q twice", kp.Kind) + } + profiled[kp.Kind] = true + if kp.CredentialModel != gp.CredentialEphemeral && kp.CredentialModel != gp.CredentialStatic { + t.Errorf("Kinds[%s].CredentialModel %q is invalid", kp.Kind, kp.CredentialModel) + } + if kp.WebhookScope != gp.WebhookApp && kp.WebhookScope != gp.WebhookGroup && kp.WebhookScope != gp.WebhookRepo { + t.Errorf("Kinds[%s].WebhookScope %q is invalid", kp.Kind, kp.WebhookScope) + } + if kp.AccessGrants && !c.AccessGrants { + t.Errorf("Kinds[%s] needs access grants but the provider does not declare the access_grants capability", kp.Kind) + } + if kp.WebhookScope != gp.WebhookApp && !c.WebhookManagement { + t.Logf("note: %s identities use %s-scoped webhooks without WebhookManagement: users must register webhooks by hand", kp.Kind, kp.WebhookScope) + } + } // A webhook the provider must manage itself implies it can manage it. if c.WebhookScope != gp.WebhookApp && !c.WebhookManagement { t.Log("note: repo/group-scoped webhooks without WebhookManagement mean users must register webhooks by hand") @@ -643,6 +672,17 @@ func checkWant(t *testing.T, ev gp.Event, w Want, caps gp.Capabilities) { } } +// fixtureKind is the identity kind the fixtures' identity has. +func fixtureKind(f Fixtures, caps gp.Capabilities) gp.IdentityKind { + if f.IdentityKind != "" { + return f.IdentityKind + } + if len(caps.IdentityKinds) > 0 { + return caps.IdentityKinds[0] + } + return "" +} + // ── credentials ────────────────────────────────────────────────────────────── func testCredentials(t *testing.T, h Harness) { @@ -658,7 +698,7 @@ func testCredentials(t *testing.T, h Harness) { if c.Secret == "" || c.Username == "" { t.Errorf("credential needs username and secret: username=%q secret-empty=%v", c.Username, c.Secret == "") } - if caps.CredentialModel == gp.CredentialEphemeral { + if caps.Profile(fixtureKind(f, caps)).CredentialModel == gp.CredentialEphemeral { if c.ExpiresAt.IsZero() { t.Error("ephemeral credentials must carry ExpiresAt") } else if d := time.Until(c.ExpiresAt); d <= 0 || d > 65*time.Minute { @@ -1189,7 +1229,9 @@ func testOptionalWrites(t *testing.T, h Harness) { mustCode(t, f, err, gp.CodeValidation) }) } - if caps.WebhookManagement { + // Webhooks are only managed for kinds that deliver them per repository or + // group; an app-level webhook has nothing to register. + if caps.WebhookManagement && caps.Profile(fixtureKind(f, caps)).WebhookScope != gp.WebhookApp { t.Run("webhook-management", func(t *testing.T) { p := h.New(t) w := p.(gp.WebhookManager) diff --git a/gitprovider/gitprovider_test.go b/gitprovider/gitprovider_test.go index 98fd3d0..4ba18cd 100644 --- a/gitprovider/gitprovider_test.go +++ b/gitprovider/gitprovider_test.go @@ -112,3 +112,18 @@ func TestDispatch_UndeclaredCapabilityIsUnsupported(t *testing.T) { t.Fatalf("status %d; want 501 for an undeclared capability", status) } } + +func TestCapabilitiesProfile(t *testing.T) { + c := gp.Capabilities{ + IdentityKinds: []gp.IdentityKind{gp.IdentityApp, gp.IdentityToken}, CredentialModel: gp.CredentialEphemeral, + WebhookScope: gp.WebhookApp, AccessGrants: true, + Kinds: []gp.KindProfile{{Kind: gp.IdentityToken, CredentialModel: gp.CredentialStatic, WebhookScope: gp.WebhookRepo}}, + } + if got := c.Profile(gp.IdentityToken); got.CredentialModel != gp.CredentialStatic || got.WebhookScope != gp.WebhookRepo || got.AccessGrants { + t.Errorf("token profile = %+v", got) + } + // A kind without an entry takes the provider-wide values. + if got := c.Profile(gp.IdentityApp); got.CredentialModel != gp.CredentialEphemeral || got.WebhookScope != gp.WebhookApp || !got.AccessGrants { + t.Errorf("app profile = %+v", got) + } +} diff --git a/gitprovider/testdata/wire-shapes.golden.json b/gitprovider/testdata/wire-shapes.golden.json index 34cec50..b8b96a5 100644 --- a/gitprovider/testdata/wire-shapes.golden.json +++ b/gitprovider/testdata/wire-shapes.golden.json @@ -125,6 +125,14 @@ "identity_kinds": [ "string" ], + "kinds": [ + { + "access_grants": "bool", + "credential_model": "string", + "kind": "string", + "webhook_scope": "string" + } + ], "labels": "bool", "merge": "bool", "multiple_identities": "bool", @@ -604,6 +612,14 @@ "identity_kinds": [ "string" ], + "kinds": [ + { + "access_grants": "bool", + "credential_model": "string", + "kind": "string", + "webhook_scope": "string" + } + ], "labels": "bool", "merge": "bool", "multiple_identities": "bool", diff --git a/gitprovider/types.go b/gitprovider/types.go index 0fc9add..1a193fb 100644 --- a/gitprovider/types.go +++ b/gitprovider/types.go @@ -75,6 +75,20 @@ const ( WebhookRepo WebhookScope = "repo" // per repository ) +// KindProfile is what one identity kind implies on a provider that supports +// several (GitHub: an App is ephemeral with an app-level webhook and needs an +// installation per account; a personal access token is static with a webhook per +// repository). The provider-wide fields of [Capabilities] describe the default +// kind; Kinds refines them per kind. +type KindProfile struct { + Kind IdentityKind `json:"kind"` + CredentialModel CredentialModel `json:"credential_model"` + WebhookScope WebhookScope `json:"webhook_scope"` + // AccessGrants is true when identities of this kind must be granted access + // to each account or organization (see [Capabilities.AccessGrants]). + AccessGrants bool `json:"access_grants"` +} + // StatusKind is how a run's state is shown on the platform. type StatusKind string @@ -184,8 +198,14 @@ type Capabilities struct { StatusKind StatusKind `json:"status_kind"` Triggers []TriggerKind `json:"triggers"` // MultipleIdentities is true when more than one identity per host can be - // created (needed for per-agent identities). + // created (needed for per-agent identities, and for several connections of + // a workspace or project). MultipleIdentities bool `json:"multiple_identities"` + // Kinds refines CredentialModel, WebhookScope and AccessGrants per identity + // kind, and lists the kinds a user can choose between when setting one up + // (core then sends Params["kind"]). Empty: there is one way to connect, and + // every kind in IdentityKinds uses the provider-wide values. + Kinds []KindProfile `json:"kinds,omitempty"` // Optional operations (each backed by an optional interface). ResolveThreads bool `json:"resolve_threads"` @@ -221,6 +241,17 @@ type Capabilities struct { CIConfigGlobs []string `json:"ci_config_globs,omitempty"` } +// Profile returns the profile of an identity kind: its entry in Kinds, or the +// provider-wide values when it has none. +func (c Capabilities) Profile(kind IdentityKind) KindProfile { + for _, k := range c.Kinds { + if k.Kind == kind { + return k + } + } + return KindProfile{Kind: kind, CredentialModel: c.CredentialModel, WebhookScope: c.WebhookScope, AccessGrants: c.AccessGrants} +} + // SupportsTrigger reports whether the provider declared trigger k. func (c Capabilities) SupportsTrigger(k TriggerKind) bool { for _, t := range c.Triggers { From df161d3d2a91c4637601ef3cfe79f16793f9af51 Mon Sep 17 00:00:00 2001 From: pikann22 Date: Wed, 28 Oct 2026 08:05:11 +0000 Subject: [PATCH 2/2] feat: enhance capabilities and profile handling for identity kinds with improved validation and fallback logic --- gitprovider/conformance/conformance.go | 15 +++++++++++++++ gitprovider/gitprovider_test.go | 8 ++++++++ gitprovider/types.go | 7 +++++-- 3 files changed, 28 insertions(+), 2 deletions(-) diff --git a/gitprovider/conformance/conformance.go b/gitprovider/conformance/conformance.go index 96d1306..164c01c 100644 --- a/gitprovider/conformance/conformance.go +++ b/gitprovider/conformance/conformance.go @@ -286,6 +286,18 @@ func testDescribe(t *testing.T, h Harness) { t.Logf("note: %s identities use %s-scoped webhooks without WebhookManagement: users must register webhooks by hand", kp.Kind, kp.WebhookScope) } } + if k := h.Fixtures.IdentityKind; k != "" && !declared[k] { + t.Errorf("Fixtures.IdentityKind %q is not in IdentityKinds; Profile would silently fall back to the provider-wide values", k) + } + if c.WebhookManagement { + managed := c.WebhookScope != gp.WebhookApp + for _, kp := range c.Kinds { + managed = managed || kp.WebhookScope != gp.WebhookApp + } + if !managed { + t.Log("note: WebhookManagement is declared but every kind delivers app-level webhooks, so it is never exercised") + } + } // A webhook the provider must manage itself implies it can manage it. if c.WebhookScope != gp.WebhookApp && !c.WebhookManagement { t.Log("note: repo/group-scoped webhooks without WebhookManagement mean users must register webhooks by hand") @@ -1231,6 +1243,9 @@ func testOptionalWrites(t *testing.T, h Harness) { } // Webhooks are only managed for kinds that deliver them per repository or // group; an app-level webhook has nothing to register. + if caps.WebhookManagement && caps.Profile(fixtureKind(f, caps)).WebhookScope == gp.WebhookApp { + t.Logf("skipping webhook-management: the fixture identity (%q) uses app-level webhooks; run the suite with a fixture of a repo/group-scoped kind to cover it", fixtureKind(f, caps)) + } if caps.WebhookManagement && caps.Profile(fixtureKind(f, caps)).WebhookScope != gp.WebhookApp { t.Run("webhook-management", func(t *testing.T) { p := h.New(t) diff --git a/gitprovider/gitprovider_test.go b/gitprovider/gitprovider_test.go index 4ba18cd..91e30f6 100644 --- a/gitprovider/gitprovider_test.go +++ b/gitprovider/gitprovider_test.go @@ -126,4 +126,12 @@ func TestCapabilitiesProfile(t *testing.T) { if got := c.Profile(gp.IdentityApp); got.CredentialModel != gp.CredentialEphemeral || got.WebhookScope != gp.WebhookApp || !got.AccessGrants { t.Errorf("app profile = %+v", got) } + // An unknown kind also takes the provider-wide values. + if got := c.Profile(""); got.Kind != "" || got.CredentialModel != gp.CredentialEphemeral || !got.AccessGrants { + t.Errorf("empty-kind profile = %+v", got) + } + // A kind entry can switch AccessGrants off against a provider-wide true. + if got := c.Profile(gp.IdentityToken); got.AccessGrants { + t.Errorf("token profile must override AccessGrants to false: %+v", got) + } } diff --git a/gitprovider/types.go b/gitprovider/types.go index 1a193fb..5b71711 100644 --- a/gitprovider/types.go +++ b/gitprovider/types.go @@ -79,7 +79,9 @@ const ( // several (GitHub: an App is ephemeral with an app-level webhook and needs an // installation per account; a personal access token is static with a webhook per // repository). The provider-wide fields of [Capabilities] describe the default -// kind; Kinds refines them per kind. +// kind; Kinds refines them per kind. A kind listed here is also one a user can +// choose when setting up a connection, even if its profile matches the +// provider-wide values. type KindProfile struct { Kind IdentityKind `json:"kind"` CredentialModel CredentialModel `json:"credential_model"` @@ -242,7 +244,8 @@ type Capabilities struct { } // Profile returns the profile of an identity kind: its entry in Kinds, or the -// provider-wide values when it has none. +// provider-wide values when it has none. The fallback is intentional: a kind in +// IdentityKinds need not be listed in Kinds unless it differs or is user-selectable. func (c Capabilities) Profile(kind IdentityKind) KindProfile { for _, k := range c.Kinds { if k.Kind == kind {