From 36e1df259b314ae3c89c558627f52fb3c01a5bdb Mon Sep 17 00:00:00 2001 From: Alessandro Gorla Date: Thu, 29 Oct 2026 12:44:36 +0100 Subject: [PATCH 1/8] chore(git): sync SPI for describe_identity Refs Paca-AI/paca#661 Co-Authored-By: Claude Opus 5.5 --- .../platform/gitprovider/spi_types.go | 19 +++++++++++++++++-- .../platform/gitprovider/spi_wiretypes.go | 4 ++-- .../testdata/wire-shapes.golden.json | 9 +++++++++ 3 files changed, 28 insertions(+), 4 deletions(-) diff --git a/services/api/internal/platform/gitprovider/spi_types.go b/services/api/internal/platform/gitprovider/spi_types.go index bb58730ed..eee8252ea 100644 --- a/services/api/internal/platform/gitprovider/spi_types.go +++ b/services/api/internal/platform/gitprovider/spi_types.go @@ -82,7 +82,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"` @@ -222,6 +224,11 @@ type Capabilities struct { Reactions bool `json:"reactions"` WebhookManagement bool `json:"webhook_management"` IdentityDeletion bool `json:"identity_deletion"` + // IdentityDescription is true when the provider can say who an existing + // identity is (handle, display name, expiry) without a setup flow. Core + // uses it to fill in identities it holds without a handle, such as those + // created by a plugin's data migration. + IdentityDescription bool `json:"identity_description"` // Branches is true when branches can be looked up and created through the // provider (core uses it to link and create a task's branch). Branches bool `json:"branches"` @@ -245,7 +252,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 { @@ -375,6 +383,13 @@ type DeleteIdentityRequest struct { Identity IdentityRef `json:"identity"` } +// DescribeIdentityRequest asks who an existing identity is (capability +// IdentityDescription). Identity.Handle may be empty: that is the case the +// operation exists for. +type DescribeIdentityRequest struct { + Identity IdentityRef `json:"identity"` +} + // ── Repositories ───────────────────────────────────────────────────────────── // Repository is a repository an identity can see. diff --git a/services/api/internal/platform/gitprovider/spi_wiretypes.go b/services/api/internal/platform/gitprovider/spi_wiretypes.go index 0ac62b3c9..805bc93e7 100644 --- a/services/api/internal/platform/gitprovider/spi_wiretypes.go +++ b/services/api/internal/platform/gitprovider/spi_wiretypes.go @@ -16,8 +16,8 @@ func WireTypes() map[string]reflect.Type { "Info": t(Info{}), "Capabilities": t(Capabilities{}), "Terms": t(Terms{}), "BeginSetupRequest": t(BeginSetupRequest{}), "SetupStep": t(SetupStep{}), "FormField": t(FormField{}), "CompleteSetupRequest": t(CompleteSetupRequest{}), "Identity": t(Identity{}), "IdentityRef": t(IdentityRef{}), - "DeleteIdentityRequest": t(DeleteIdentityRequest{}), - "BeginAccessRequest": t(BeginAccessRequest{}), "CompleteAccessRequest": t(CompleteAccessRequest{}), "AccessGrant": t(AccessGrant{}), + "DeleteIdentityRequest": t(DeleteIdentityRequest{}), "DescribeIdentityRequest": t(DescribeIdentityRequest{}), + "BeginAccessRequest": t(BeginAccessRequest{}), "CompleteAccessRequest": t(CompleteAccessRequest{}), "AccessGrant": t(AccessGrant{}), "RepoRef": t(RepoRef{}), "Target": t(Target{}), "SubjectRef": t(SubjectRef{}), "Repository": t(Repository{}), "ListReposRequest": t(ListReposRequest{}), "RepositoryPage": t(RepositoryPage{}), "ResolveRepoRequest": t(ResolveRepoRequest{}), "ResolvedRepository": t(ResolvedRepository{}), diff --git a/services/api/internal/platform/gitprovider/testdata/wire-shapes.golden.json b/services/api/internal/platform/gitprovider/testdata/wire-shapes.golden.json index b8b96a53e..161e6f9ff 100644 --- a/services/api/internal/platform/gitprovider/testdata/wire-shapes.golden.json +++ b/services/api/internal/platform/gitprovider/testdata/wire-shapes.golden.json @@ -122,6 +122,7 @@ "hidden_markers": "bool", "idempotent_writes": "bool", "identity_deletion": "bool", + "identity_description": "bool", "identity_kinds": [ "string" ], @@ -377,6 +378,13 @@ "provider_ref": "bytes" } }, + "DescribeIdentityRequest": { + "identity": { + "handle": "string", + "id": "string", + "provider_ref": "bytes" + } + }, "Diff": { "text": "string", "truncated": "bool" @@ -609,6 +617,7 @@ "hidden_markers": "bool", "idempotent_writes": "bool", "identity_deletion": "bool", + "identity_description": "bool", "identity_kinds": [ "string" ], From 49c514f9633bc2558412047f4f2474eb31676388 Mon Sep 17 00:00:00 2001 From: Alessandro Gorla Date: Thu, 29 Oct 2026 12:53:58 +0100 Subject: [PATCH 2/8] feat(git): add describe_identity client op Refs Paca-AI/paca#661 Co-Authored-By: Claude Opus 5.5 --- .../internal/platform/gitprovider/client.go | 5 +++++ .../platform/gitprovider/client_test.go | 20 +++++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/services/api/internal/platform/gitprovider/client.go b/services/api/internal/platform/gitprovider/client.go index a42524905..d038e8a2f 100644 --- a/services/api/internal/platform/gitprovider/client.go +++ b/services/api/internal/platform/gitprovider/client.go @@ -124,6 +124,11 @@ func (c *Client) RemoveWebhook(ctx context.Context, req RemoveWebhookRequest) er return err } +// DescribeIdentity says who an existing identity is (capability identity_description). +func (c *Client) DescribeIdentity(ctx context.Context, req DescribeIdentityRequest) (Identity, error) { + return call[DescribeIdentityRequest, Identity](ctx, c, "describe_identity", req) +} + // DeleteIdentity asks the provider to forget an identity remotely. func (c *Client) DeleteIdentity(ctx context.Context, req DeleteIdentityRequest) error { _, err := call[DeleteIdentityRequest, Empty](ctx, c, "delete_identity", req) diff --git a/services/api/internal/platform/gitprovider/client_test.go b/services/api/internal/platform/gitprovider/client_test.go index b74798067..c3a530474 100644 --- a/services/api/internal/platform/gitprovider/client_test.go +++ b/services/api/internal/platform/gitprovider/client_test.go @@ -122,3 +122,23 @@ func TestClient_AccessGrantOperations(t *testing.T) { t.Errorf("a provider without grants answers unsupported: %v", err) } } + +func TestClient_DescribeIdentity(t *testing.T) { + d := &fakeDoer{status: 200, resp: `{"ok":true,"result":{"ref":{"id":"i1","handle":"bot"},"display_name":"Bot"}}`} + c := NewClient(d, "com.paca.gitlab") + id, err := c.DescribeIdentity(context.Background(), DescribeIdentityRequest{Identity: IdentityRef{ID: "i1"}}) + if err != nil || id.Ref.ID != "i1" || id.Ref.Handle != "bot" || id.DisplayName != "Bot" { + t.Fatalf("describe: %+v %v", id, err) + } + if d.gotURL != "com.paca.gitlab POST /_git/describe_identity" { + t.Errorf("called %q", d.gotURL) + } + var sent DescribeIdentityRequest + if json.Unmarshal(d.gotReq, &sent) != nil || sent.Identity.ID != "i1" { + t.Errorf("request body %s", d.gotReq) + } + d.status, d.resp = 404, `{"ok":false,"error":{"code":"not_found","message":"gone"}}` + if _, err := c.DescribeIdentity(context.Background(), DescribeIdentityRequest{}); !IsCode(err, CodeNotFound) { + t.Errorf("unknown identity is not_found: %v", err) + } +} From ef36364b335dcab73d47f5eebfc036a57170781f Mon Sep 17 00:00:00 2001 From: Alessandro Gorla Date: Thu, 29 Oct 2026 12:53:58 +0100 Subject: [PATCH 3/8] feat(git): name connections with no handle from the provider Identities created by a plugin's data migration have an empty handle. When connections or shared identities are listed, ask providers that declare identity_description who they are and store the answer. Refs Paca-AI/paca#661 Co-Authored-By: Claude Opus 5.5 --- .../api/internal/domain/git/repository.go | 3 + .../repository/postgres/git_repository.go | 19 ++++ .../postgres/git_repository_test.go | 29 +++++ .../api/internal/service/git/describe_test.go | 103 ++++++++++++++++++ services/api/internal/service/git/service.go | 1 + .../api/internal/service/git/service_test.go | 25 +++++ services/api/internal/service/git/setup.go | 65 +++++++++++ 7 files changed, 245 insertions(+) create mode 100644 services/api/internal/service/git/describe_test.go diff --git a/services/api/internal/domain/git/repository.go b/services/api/internal/domain/git/repository.go index e10856f80..9484aa923 100644 --- a/services/api/internal/domain/git/repository.go +++ b/services/api/internal/domain/git/repository.go @@ -21,6 +21,9 @@ type Store interface { SetAgentIdentity(ctx context.Context, agentID uuid.UUID, providerID string, identityID uuid.UUID) error FindAgentIdentity(ctx context.Context, agentID uuid.UUID, providerID string) (*Identity, error) DeleteIdentity(ctx context.Context, id uuid.UUID) error + // UpdateIdentityProfile fills in who an identity is on the platform. An + // empty displayName and a nil expiresAt leave the stored values as they are. + UpdateIdentityProfile(ctx context.Context, id uuid.UUID, handle, displayName string, expiresAt *time.Time) error // DeleteAgentIdentity removes an agent's mapping (not the identity itself). DeleteAgentIdentity(ctx context.Context, agentID uuid.UUID, providerID string) error diff --git a/services/api/internal/repository/postgres/git_repository.go b/services/api/internal/repository/postgres/git_repository.go index 2998bb0eb..6f4191bb4 100644 --- a/services/api/internal/repository/postgres/git_repository.go +++ b/services/api/internal/repository/postgres/git_repository.go @@ -149,6 +149,25 @@ func (s *GitRepository) CreateIdentity(ctx context.Context, i *gitdom.Identity) return nil } +// UpdateIdentityProfile stores the handle, and when given the display name and +// expiry, the platform reported for an identity. +func (s *GitRepository) UpdateIdentityProfile(ctx context.Context, id uuid.UUID, handle, displayName string, expiresAt *time.Time) error { + res, err := s.db.ExecContext(ctx, ` + UPDATE git_identities + SET handle = $2, + display_name = CASE WHEN $3 <> '' THEN $3 ELSE display_name END, + expires_at = COALESCE($4, expires_at), + updated_at = NOW() + WHERE id = $1`, id.String(), handle, displayName, expiresAt) + if err != nil { + return fmt.Errorf("git repo: update identity profile: %w", err) + } + if n, _ := res.RowsAffected(); n == 0 { + return gitdom.ErrNotFound + } + return nil +} + func (s *GitRepository) getIdentity(ctx context.Context, where string, args ...any) (*gitdom.Identity, error) { var rec gitIdentityRecord if err := s.db.GetContext(ctx, &rec, `SELECT `+gitIdentityCols+` FROM git_identities WHERE `+where, args...); err != nil { diff --git a/services/api/internal/repository/postgres/git_repository_test.go b/services/api/internal/repository/postgres/git_repository_test.go index 517892205..2077a1d0a 100644 --- a/services/api/internal/repository/postgres/git_repository_test.go +++ b/services/api/internal/repository/postgres/git_repository_test.go @@ -861,3 +861,32 @@ func TestGitStore_DeletingAConnectionLeavesItsRepositoriesToBeRebound(t *testing t.Fatalf("unknown repository: %v", err) } } + +func TestGitStore_UpdateIdentityProfile(t *testing.T) { + s, _ := newGitFx(t) + ctx := t.Context() + exp := time.Now().Add(48 * time.Hour).UTC().Truncate(time.Microsecond) + i := sharedIdentity("gitlab.com") + i.Handle, i.DisplayName = "", "GitLab connection" + if err := s.CreateIdentity(ctx, i); err != nil { + t.Fatal(err) + } + if err := s.UpdateIdentityProfile(ctx, i.ID, "paca-bot", "Paca Bot", &exp); err != nil { + t.Fatalf("update: %v", err) + } + got, err := s.FindIdentity(ctx, i.ID) + if err != nil || got.Handle != "paca-bot" || got.DisplayName != "Paca Bot" || got.ExpiresAt == nil || !got.ExpiresAt.Equal(exp) { + t.Fatalf("after update: %+v %v", got, err) + } + // An empty name and no expiry keep what is stored. + if err := s.UpdateIdentityProfile(ctx, i.ID, "renamed", "", nil); err != nil { + t.Fatal(err) + } + got, _ = s.FindIdentity(ctx, i.ID) + if got.Handle != "renamed" || got.DisplayName != "Paca Bot" || got.ExpiresAt == nil || !got.ExpiresAt.Equal(exp) { + t.Errorf("empty values overwrote: %+v", got) + } + if err := s.UpdateIdentityProfile(ctx, uuid.New(), "x", "", nil); !errors.Is(err, gitdom.ErrNotFound) { + t.Errorf("unknown identity: %v", err) + } +} diff --git a/services/api/internal/service/git/describe_test.go b/services/api/internal/service/git/describe_test.go new file mode 100644 index 000000000..f750c26af --- /dev/null +++ b/services/api/internal/service/git/describe_test.go @@ -0,0 +1,103 @@ +package gitsvc + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/google/uuid" + + gitdom "github.com/Paca-AI/api/internal/domain/git" + "github.com/Paca-AI/api/internal/platform/gitprovider" +) + +func describingSetup(t *testing.T, capable bool) (*Service, *fakeStore, *fakeProv) { + t.Helper() + svc, st, pr := setup(t) + pr.info.ID = "p" + pr.info.Capabilities.IdentityDescription = capable + return svc, st, pr +} + +func TestDescribeNameless_FillsHandleAndPersists(t *testing.T) { + svc, st, pr := describingSetup(t, true) + exp := time.Now().Add(24 * time.Hour).UTC() + id := st.addIdentity("", "tok-1") + pr.describe = func(r gitprovider.DescribeIdentityRequest) (gitprovider.Identity, error) { + return gitprovider.Identity{Ref: gitprovider.IdentityRef{ID: r.Identity.ID, Handle: "paca-bot"}, DisplayName: "Paca Bot", ExpiresAt: exp}, nil + } + for name, list := range map[string]func() ([]*gitdom.Identity, error){ + "identities": func() ([]*gitdom.Identity, error) { return svc.ListIdentities(context.Background()) }, + "connections": func() ([]*gitdom.Identity, error) { return svc.ListConnections(context.Background(), uuid.New()) }, + } { + id.Handle, id.DisplayName, id.ExpiresAt = "", "", nil + pr.describeLog = nil + got, err := list() + if err != nil || len(got) != 1 { + t.Fatalf("%s: %v %v", name, got, err) + } + if got[0].Handle != "paca-bot" || got[0].DisplayName != "Paca Bot" || got[0].ExpiresAt == nil || !got[0].ExpiresAt.Equal(exp) { + t.Errorf("%s: response not filled: %+v", name, got[0]) + } + if len(pr.describeLog) != 1 || pr.describeLog[0].Identity.ID != "tok-1" { + t.Errorf("%s: describe calls = %+v", name, pr.describeLog) + } + } + if st.profileUpdates != 2 { + t.Errorf("profile saved %d times, want 2", st.profileUpdates) + } + // Once named, a later listing does not ask again. + pr.describeLog = nil + if _, err := svc.ListIdentities(context.Background()); err != nil || len(pr.describeLog) != 0 { + t.Errorf("named identity described again: %v %d", err, len(pr.describeLog)) + } +} + +func TestDescribeNameless_ProviderErrorKeepsRequestOK(t *testing.T) { + svc, st, pr := describingSetup(t, true) + id := st.addIdentity("", "tok-1") + pr.describe = func(gitprovider.DescribeIdentityRequest) (gitprovider.Identity, error) { + return gitprovider.Identity{}, errors.New("platform down") + } + got, err := svc.ListIdentities(context.Background()) + if err != nil || len(got) != 1 || id.Handle != "" || st.profileUpdates != 0 { + t.Fatalf("got=%v err=%v handle=%q saves=%d", got, err, id.Handle, st.profileUpdates) + } +} + +func TestDescribeNameless_SkipsWhenNotNeeded(t *testing.T) { + t.Run("handle present", func(t *testing.T) { + svc, st, pr := describingSetup(t, true) + st.addIdentity("already", "tok-1") + if _, err := svc.ListIdentities(context.Background()); err != nil || len(pr.describeLog) != 0 { + t.Fatalf("err=%v calls=%d", err, len(pr.describeLog)) + } + }) + t.Run("provider without the capability", func(t *testing.T) { + svc, st, pr := describingSetup(t, false) + st.addIdentity("", "tok-1") + if _, err := svc.ListConnections(context.Background(), uuid.New()); err != nil || len(pr.describeLog) != 0 { + t.Fatalf("err=%v calls=%d", err, len(pr.describeLog)) + } + }) +} + +func TestDescribeNameless_IgnoresUnusableAnswers(t *testing.T) { + for name, answer := range map[string]gitprovider.Identity{ + "other identity": {Ref: gitprovider.IdentityRef{ID: "someone-else", Handle: "intruder"}, DisplayName: "X"}, + "empty handle": {Ref: gitprovider.IdentityRef{ID: "tok-1"}, DisplayName: "X"}, + } { + t.Run(name, func(t *testing.T) { + svc, st, pr := describingSetup(t, true) + id := st.addIdentity("", "tok-1") + pr.describe = func(gitprovider.DescribeIdentityRequest) (gitprovider.Identity, error) { return answer, nil } + if _, err := svc.ListIdentities(context.Background()); err != nil { + t.Fatal(err) + } + if id.Handle != "" || id.DisplayName != "" || st.profileUpdates != 0 { + t.Errorf("stored an unusable answer: %+v saves=%d", id, st.profileUpdates) + } + }) + } +} diff --git a/services/api/internal/service/git/service.go b/services/api/internal/service/git/service.go index 23d938d6e..a49f8f15a 100644 --- a/services/api/internal/service/git/service.go +++ b/services/api/internal/service/git/service.go @@ -34,6 +34,7 @@ type ProviderAPI interface { BeginIdentitySetup(ctx context.Context, req gitprovider.BeginSetupRequest) (gitprovider.SetupStep, error) CompleteIdentitySetup(ctx context.Context, req gitprovider.CompleteSetupRequest) (gitprovider.Identity, error) DeleteIdentity(ctx context.Context, req gitprovider.DeleteIdentityRequest) error + DescribeIdentity(ctx context.Context, req gitprovider.DescribeIdentityRequest) (gitprovider.Identity, error) BeginAccessGrant(ctx context.Context, req gitprovider.BeginAccessRequest) (gitprovider.SetupStep, error) CompleteAccessGrant(ctx context.Context, req gitprovider.CompleteAccessRequest) (gitprovider.AccessGrant, error) EnsureWebhook(ctx context.Context, req gitprovider.EnsureWebhookRequest) (gitprovider.WebhookRef, error) diff --git a/services/api/internal/service/git/service_test.go b/services/api/internal/service/git/service_test.go index ae88b8ed9..3de0b1abb 100644 --- a/services/api/internal/service/git/service_test.go +++ b/services/api/internal/service/git/service_test.go @@ -34,6 +34,8 @@ type fakeStore struct { sessions map[string]*gitdom.SetupSession access map[string]*gitdom.AccessSession bound map[string]uuid.UUID + + profileUpdates int } func newFakeStore() *fakeStore { @@ -100,6 +102,21 @@ func (f *fakeStore) DeleteAgentIdentity(_ context.Context, a uuid.UUID, _ string delete(f.agentIDs, a) return nil } +func (f *fakeStore) UpdateIdentityProfile(_ context.Context, id uuid.UUID, handle, name string, exp *time.Time) error { + i, ok := f.byID[id] + if !ok { + return gitdom.ErrNotFound + } + f.profileUpdates++ + i.Handle = handle + if name != "" { + i.DisplayName = name + } + if exp != nil { + i.ExpiresAt = exp + } + return nil +} func (f *fakeStore) DeleteIdentity(_ context.Context, id uuid.UUID) error { i, ok := f.byID[id] if !ok { @@ -338,6 +355,14 @@ type fakeProv struct { ensureErr error removed []gitprovider.RemoveWebhookRequest describes int + + describe func(gitprovider.DescribeIdentityRequest) (gitprovider.Identity, error) + describeLog []gitprovider.DescribeIdentityRequest +} + +func (p *fakeProv) DescribeIdentity(_ context.Context, r gitprovider.DescribeIdentityRequest) (gitprovider.Identity, error) { + p.describeLog = append(p.describeLog, r) + return p.describe(r) } func (p *fakeProv) EnsureWebhook(_ context.Context, r gitprovider.EnsureWebhookRequest) (gitprovider.WebhookRef, error) { diff --git a/services/api/internal/service/git/setup.go b/services/api/internal/service/git/setup.go index 141e76f19..641e2f44a 100644 --- a/services/api/internal/service/git/setup.go +++ b/services/api/internal/service/git/setup.go @@ -5,6 +5,7 @@ import ( "crypto/rand" "encoding/hex" "errors" + "log/slog" "net/url" "strings" "time" @@ -299,6 +300,7 @@ func (s *Service) ListIdentities(ctx context.Context) ([]*gitdom.Identity, error } out = append(out, ids...) } + s.describeNameless(ctx, out) return out, nil } @@ -321,6 +323,7 @@ func (s *Service) ListConnections(ctx context.Context, projectID uuid.UUID) ([]* all = append(all, id.ID) } } + s.describeNameless(ctx, out) counts, err := s.store.CountRepositories(ctx, all) if err != nil { return nil, err @@ -331,6 +334,68 @@ func (s *Service) ListConnections(ctx context.Context, projectID uuid.UUID) ([]* return out, nil } +// describeTimeout bounds one describe_identity call: listings must not hang on +// a slow platform. +const describeTimeout = 5 * time.Second + +// describeNameless asks the provider who each identity without a handle is, +// and stores the answer. Identities made by a plugin's data migration start +// that way (the migration cannot call the platform), so they could be neither +// named in Settings nor matched by an @mention. The call happens at most once +// per identity: after it the stored handle is no longer empty. It is best +// effort: any failure is logged and the caller carries on with the old values. +func (s *Service) describeNameless(ctx context.Context, ids []*gitdom.Identity) { + canDescribe := map[string]bool{} // provider id → declares identity_description + for _, id := range ids { + if id.Handle != "" { + continue + } + ok, seen := canDescribe[id.ProviderID] + if !seen { + info, err := s.providers.Describe(ctx, id.ProviderID) + ok = err == nil && info.Capabilities.IdentityDescription + canDescribe[id.ProviderID] = ok + } + if ok { + s.describeIdentity(ctx, id) + } + } +} + +func (s *Service) describeIdentity(ctx context.Context, id *gitdom.Identity) { + prov, err := s.providers.Provider(ctx, id.ProviderID) + if err != nil { + return + } + cctx, cancel := context.WithTimeout(ctx, describeTimeout) + defer cancel() + got, err := prov.DescribeIdentity(cctx, gitprovider.DescribeIdentityRequest{Identity: identityRef(id)}) + if err != nil { + // The error is the provider's own message, never a credential. + slog.Warn("git: describe identity failed", "provider", id.ProviderID, "identity", id.ID, "err", err) + return + } + // An answer for another identity, or without a handle, must not be stored. + if got.Ref.ID != id.ProviderIdentityID || got.Ref.Handle == "" { + return + } + var exp *time.Time + if !got.ExpiresAt.IsZero() { + exp = &got.ExpiresAt + } + if err := s.store.UpdateIdentityProfile(ctx, id.ID, got.Ref.Handle, got.DisplayName, exp); err != nil { + slog.Warn("git: save described identity failed", "provider", id.ProviderID, "identity", id.ID, "err", err) + return + } + id.Handle = got.Ref.Handle + if got.DisplayName != "" { + id.DisplayName = got.DisplayName + } + if exp != nil { + id.ExpiresAt = exp + } +} + func (s *Service) enabledProviders(ctx context.Context) (map[string]bool, error) { infos, err := s.providers.List(ctx) if err != nil { From 85951859fc1a47c4f4643e348e7f4fe027196aaa Mon Sep 17 00:00:00 2001 From: Alessandro Gorla Date: Thu, 29 Oct 2026 12:53:58 +0100 Subject: [PATCH 4/8] docs(git): document describe_identity Refs Paca-AI/paca#661 Co-Authored-By: Claude Opus 5.5 --- docs/ai-agent/git-agent-workflow.md | 2 +- docs/plugins/git-provider-spi.md | 11 +++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/docs/ai-agent/git-agent-workflow.md b/docs/ai-agent/git-agent-workflow.md index 4b84566f6..433e11b25 100644 --- a/docs/ai-agent/git-agent-workflow.md +++ b/docs/ai-agent/git-agent-workflow.md @@ -176,7 +176,7 @@ Declared in `Describe()`. Core's UI, tools and router read them instead of hard- | `webhook_scope` | `app`, `group`, `repo` | Whether core asks the provider to `EnsureWebhook` per linked repository. | | `triggers` | subset of `mention, label, review_request, assignment, ci_failure, own_cr_feedback, auto_review` | Which trigger switches are offered on an agent. | | `status_kind` | `check_run`, `commit_status`, `comment` | How run status is shown. | -| `branches`, `resolve_threads`, `file_read`, `code_search`, `labels`, `reviewer_requests`, `assignees`, `merge`, `rerun_checks`, `reactions`, `webhook_management`, `identity_deletion`, `access_grants` | bool | The matching optional operation exists; tools and screens that need it appear only then. | +| `branches`, `resolve_threads`, `file_read`, `code_search`, `labels`, `reviewer_requests`, `assignees`, `merge`, `rerun_checks`, `reactions`, `webhook_management`, `identity_deletion`, `identity_description`, `access_grants` | bool | The matching optional operation exists; tools and screens that need it appear only then. | | `batch_review`, `idempotent_writes`, `hidden_markers`, `fork_change_requests`, `draft_change_requests`, `nested_paths` | bool | Behaviour of required operations. | | `ci_config_globs` | paths | Files agents must not modify (§12.3). | diff --git a/docs/plugins/git-provider-spi.md b/docs/plugins/git-provider-spi.md index 9d5afd621..a24210e79 100644 --- a/docs/plugins/git-provider-spi.md +++ b/docs/plugins/git-provider-spi.md @@ -39,6 +39,7 @@ func main() { plugin.Run(&myPlugin{}) } | `Merger`, `CheckRerunner`, `Reactor` | `merge`, `rerun_checks`, `reactions` | | `WebhookManager` | `webhook_management` (repo/group-scoped webhooks) | | `IdentityDeleter` | `identity_deletion` | +| `IdentityDescriber` (`DescribeIdentity`) | `identity_description` (see below) | | `BranchManager` (`GetBranch`, `CreateBranch`) | `branches` | | `AccessGranter` | `access_grants` (see below) | @@ -74,6 +75,16 @@ The SDK ships a conformance suite (`plugin-sdk-go/gitprovider/conformance`) and Trigger configuration, routing, actor policy, run lifecycle, debounce, comment/thread ownership, status comments, audit, MCP tools, workspace protocol, credential-helper plumbing, task linking and every screen except your identity setup component. If you find yourself writing one of those, the SPI is missing something — raise it. +## Identity description (optional) + +`DescribeIdentity(ctx, DescribeIdentityRequest{Identity})` says who an existing identity is, without a setup flow. It exists for identities core holds with no handle: a plugin's data migration (for example from an older plugin version or a community plugin) creates `git_identities` rows without calling the platform, so it cannot know the token's user. Such a row would stay nameless in Settings → Git and `@mentions` would not match it. + +* **When core calls it**: while listing connections (a project's Settings → Git) or the workspace's shared identities, for each identity whose stored handle is empty and whose provider declares `identity_description`. Once per identity, because the answer is stored; an identity that already has a handle is never described. +* **What you return**: an `Identity` whose `Ref.ID` equals the request's, with `Ref.Handle` set (the login users `@mention`), `DisplayName`, and `ExpiresAt` for credentials that expire. `Next` is ignored. Unknown identity: `not_found`; credentials the platform no longer accepts: `forbidden`. +* **What core stores**: the handle, and the display name and expiry when non-empty. An answer with another `Ref.ID` or an empty handle is discarded. +* **Failure**: best effort. Core bounds the call (5 s), logs the error without secrets and serves the listing with the old values; it tries again on the next listing. +* **For migrations**: leave `handle` empty (and a generic `display_name`) in the SQL and implement `DescribeIdentity` by calling the platform's "current user" endpoint with the stored credential. + ## Access grants (optional) Some platforms only let an identity see the repositories of accounts it has been granted access to, and a *user* grants that on the platform. GitHub is the example: a GitHub App sees only the accounts and repositories it is installed on. Because each project can use a repository owned by a different user or organization, this cannot happen once, when the identity is created. From 658af6869f5bf9d2c99b62be59f79cda9aeef138 Mon Sep 17 00:00:00 2001 From: Alessandro Gorla Date: Thu, 29 Oct 2026 12:59:46 +0100 Subject: [PATCH 5/8] fix(git): back off after a failed describe_identity A revoked token or a platform outage made every listing wait for the call again. Remember the failure in memory and skip the identity for 15 minutes. Refs Paca-AI/paca#661 Co-Authored-By: Claude Opus 5.5 --- docs/plugins/git-provider-spi.md | 2 +- .../api/internal/domain/git/repository.go | 3 ++ .../api/internal/service/git/describe_test.go | 32 +++++++++++++++++++ services/api/internal/service/git/service.go | 4 ++- services/api/internal/service/git/setup.go | 17 ++++++++++ 5 files changed, 56 insertions(+), 2 deletions(-) diff --git a/docs/plugins/git-provider-spi.md b/docs/plugins/git-provider-spi.md index a24210e79..940a346b7 100644 --- a/docs/plugins/git-provider-spi.md +++ b/docs/plugins/git-provider-spi.md @@ -82,7 +82,7 @@ Trigger configuration, routing, actor policy, run lifecycle, debounce, comment/t * **When core calls it**: while listing connections (a project's Settings → Git) or the workspace's shared identities, for each identity whose stored handle is empty and whose provider declares `identity_description`. Once per identity, because the answer is stored; an identity that already has a handle is never described. * **What you return**: an `Identity` whose `Ref.ID` equals the request's, with `Ref.Handle` set (the login users `@mention`), `DisplayName`, and `ExpiresAt` for credentials that expire. `Next` is ignored. Unknown identity: `not_found`; credentials the platform no longer accepts: `forbidden`. * **What core stores**: the handle, and the display name and expiry when non-empty. An answer with another `Ref.ID` or an empty handle is discarded. -* **Failure**: best effort. Core bounds the call (5 s), logs the error without secrets and serves the listing with the old values; it tries again on the next listing. +* **Failure**: best effort. Core bounds the call (5 s), logs the error without secrets and serves the listing with the old values; after a failure core does not ask about that identity again for 15 minutes (in memory, reset on restart), then retries. * **For migrations**: leave `handle` empty (and a generic `display_name`) in the SQL and implement `DescribeIdentity` by calling the platform's "current user" endpoint with the stored credential. ## Access grants (optional) diff --git a/services/api/internal/domain/git/repository.go b/services/api/internal/domain/git/repository.go index 9484aa923..9352e3ec6 100644 --- a/services/api/internal/domain/git/repository.go +++ b/services/api/internal/domain/git/repository.go @@ -23,6 +23,9 @@ type Store interface { DeleteIdentity(ctx context.Context, id uuid.UUID) error // UpdateIdentityProfile fills in who an identity is on the platform. An // empty displayName and a nil expiresAt leave the stored values as they are. + // It only touches an identity that has no handle yet, so a handle set in + // the meantime (a reconnect) is never overwritten; an identity that is gone + // or already named is a silent no-op. UpdateIdentityProfile(ctx context.Context, id uuid.UUID, handle, displayName string, expiresAt *time.Time) error // DeleteAgentIdentity removes an agent's mapping (not the identity itself). DeleteAgentIdentity(ctx context.Context, agentID uuid.UUID, providerID string) error diff --git a/services/api/internal/service/git/describe_test.go b/services/api/internal/service/git/describe_test.go index f750c26af..f2934303b 100644 --- a/services/api/internal/service/git/describe_test.go +++ b/services/api/internal/service/git/describe_test.go @@ -101,3 +101,35 @@ func TestDescribeNameless_IgnoresUnusableAnswers(t *testing.T) { }) } } + +func TestDescribeNameless_BacksOffAfterFailure(t *testing.T) { + svc, st, pr := describingSetup(t, true) + now := time.Now() + svc.now = func() time.Time { return now } + id := st.addIdentity("", "tok-1") + fail := true + pr.describe = func(r gitprovider.DescribeIdentityRequest) (gitprovider.Identity, error) { + if fail { + return gitprovider.Identity{}, errors.New("revoked") + } + return gitprovider.Identity{Ref: gitprovider.IdentityRef{ID: r.Identity.ID, Handle: "bot"}}, nil + } + list := func() { _, _ = svc.ListIdentities(context.Background()) } + + list() + list() + if len(pr.describeLog) != 1 { + t.Fatalf("calls within the backoff = %d, want 1", len(pr.describeLog)) + } + now = now.Add(describeBackoff + time.Second) + list() + if len(pr.describeLog) != 2 { + t.Fatalf("calls after the backoff = %d, want 2", len(pr.describeLog)) + } + fail = false + now = now.Add(describeBackoff + time.Second) + list() + if id.Handle != "bot" || len(svc.describeFailed) != 0 { + t.Errorf("success: handle=%q failures=%v", id.Handle, svc.describeFailed) + } +} diff --git a/services/api/internal/service/git/service.go b/services/api/internal/service/git/service.go index a49f8f15a..f6d29bddf 100644 --- a/services/api/internal/service/git/service.go +++ b/services/api/internal/service/git/service.go @@ -64,11 +64,13 @@ type Service struct { cache map[credKey]cachedCred reruns map[uuid.UUID][]time.Time // check re-runs per repository, last hour ci map[ciKey]cachedCI // CI summaries of change-request heads + + describeFailed map[uuid.UUID]time.Time // identity → last failed describe, see describeBackoff } // New returns a Service. func New(store gitdom.Store, providers Providers) *Service { - return &Service{store: store, providers: providers, now: time.Now, cache: map[credKey]cachedCred{}, reruns: map[uuid.UUID][]time.Time{}} + return &Service{store: store, providers: providers, now: time.Now, cache: map[credKey]cachedCred{}, reruns: map[uuid.UUID][]time.Time{}, describeFailed: map[uuid.UUID]time.Time{}} } type credKey struct { diff --git a/services/api/internal/service/git/setup.go b/services/api/internal/service/git/setup.go index 641e2f44a..a342e4e84 100644 --- a/services/api/internal/service/git/setup.go +++ b/services/api/internal/service/git/setup.go @@ -338,6 +338,11 @@ func (s *Service) ListConnections(ctx context.Context, projectID uuid.UUID) ([]* // a slow platform. const describeTimeout = 5 * time.Second +// describeBackoff is how long after a failed describe the same identity is not +// asked again (a revoked token or a platform outage would otherwise slow every +// listing). It is kept in memory only, so a restart retries. +const describeBackoff = 15 * time.Minute + // describeNameless asks the provider who each identity without a handle is, // and stores the answer. Identities made by a plugin's data migration start // that way (the migration cannot call the platform), so they could be neither @@ -363,6 +368,12 @@ func (s *Service) describeNameless(ctx context.Context, ids []*gitdom.Identity) } func (s *Service) describeIdentity(ctx context.Context, id *gitdom.Identity) { + s.mu.Lock() + failedAt, failed := s.describeFailed[id.ID] + s.mu.Unlock() + if failed && s.now().Sub(failedAt) < describeBackoff { + return + } prov, err := s.providers.Provider(ctx, id.ProviderID) if err != nil { return @@ -373,8 +384,14 @@ func (s *Service) describeIdentity(ctx context.Context, id *gitdom.Identity) { if err != nil { // The error is the provider's own message, never a credential. slog.Warn("git: describe identity failed", "provider", id.ProviderID, "identity", id.ID, "err", err) + s.mu.Lock() + s.describeFailed[id.ID] = s.now() + s.mu.Unlock() return } + s.mu.Lock() + delete(s.describeFailed, id.ID) + s.mu.Unlock() // An answer for another identity, or without a handle, must not be stored. if got.Ref.ID != id.ProviderIdentityID || got.Ref.Handle == "" { return From 8a77ef9cf64a6af08dd793d650ded2b522a5b6db Mon Sep 17 00:00:00 2001 From: Alessandro Gorla Date: Thu, 29 Oct 2026 12:59:46 +0100 Subject: [PATCH 6/8] fix(git): never overwrite a handle in UpdateIdentityProfile Only update rows whose handle is still empty, so a reconnect that named the identity meanwhile wins over a lazy describe. Zero rows is a no-op. Refs Paca-AI/paca#661 Co-Authored-By: Claude Opus 5.5 --- .../repository/postgres/git_repository.go | 11 +++++----- .../postgres/git_repository_test.go | 22 +++++++++++++++---- 2 files changed, 23 insertions(+), 10 deletions(-) diff --git a/services/api/internal/repository/postgres/git_repository.go b/services/api/internal/repository/postgres/git_repository.go index 6f4191bb4..ed13cb9d7 100644 --- a/services/api/internal/repository/postgres/git_repository.go +++ b/services/api/internal/repository/postgres/git_repository.go @@ -150,21 +150,20 @@ func (s *GitRepository) CreateIdentity(ctx context.Context, i *gitdom.Identity) } // UpdateIdentityProfile stores the handle, and when given the display name and -// expiry, the platform reported for an identity. +// expiry, the platform reported for an identity. The handle = ” guard keeps a +// handle set meanwhile (a reconnect) from being overwritten; zero rows +// (deleted or already named) is not an error. func (s *GitRepository) UpdateIdentityProfile(ctx context.Context, id uuid.UUID, handle, displayName string, expiresAt *time.Time) error { - res, err := s.db.ExecContext(ctx, ` + _, err := s.db.ExecContext(ctx, ` UPDATE git_identities SET handle = $2, display_name = CASE WHEN $3 <> '' THEN $3 ELSE display_name END, expires_at = COALESCE($4, expires_at), updated_at = NOW() - WHERE id = $1`, id.String(), handle, displayName, expiresAt) + WHERE id = $1 AND handle = ''`, id.String(), handle, displayName, expiresAt) if err != nil { return fmt.Errorf("git repo: update identity profile: %w", err) } - if n, _ := res.RowsAffected(); n == 0 { - return gitdom.ErrNotFound - } return nil } diff --git a/services/api/internal/repository/postgres/git_repository_test.go b/services/api/internal/repository/postgres/git_repository_test.go index 2077a1d0a..4091c28d4 100644 --- a/services/api/internal/repository/postgres/git_repository_test.go +++ b/services/api/internal/repository/postgres/git_repository_test.go @@ -878,15 +878,29 @@ func TestGitStore_UpdateIdentityProfile(t *testing.T) { if err != nil || got.Handle != "paca-bot" || got.DisplayName != "Paca Bot" || got.ExpiresAt == nil || !got.ExpiresAt.Equal(exp) { t.Fatalf("after update: %+v %v", got, err) } - // An empty name and no expiry keep what is stored. - if err := s.UpdateIdentityProfile(ctx, i.ID, "renamed", "", nil); err != nil { + // A row that already has a handle is left unchanged. + if err := s.UpdateIdentityProfile(ctx, i.ID, "renamed", "Other", nil); err != nil { t.Fatal(err) } got, _ = s.FindIdentity(ctx, i.ID) - if got.Handle != "renamed" || got.DisplayName != "Paca Bot" || got.ExpiresAt == nil || !got.ExpiresAt.Equal(exp) { + if got.Handle != "paca-bot" || got.DisplayName != "Paca Bot" || got.ExpiresAt == nil || !got.ExpiresAt.Equal(exp) { + t.Errorf("a named identity was overwritten: %+v", got) + } + // An empty name and no expiry keep what is stored. + j := sharedIdentity("gitlab.com") + j.ProviderIdentityID, j.Handle, j.DisplayName = "tok-2", "", "GitLab connection" + if err := s.CreateIdentity(ctx, j); err != nil { + t.Fatal(err) + } + if err := s.UpdateIdentityProfile(ctx, j.ID, "bot2", "", nil); err != nil { + t.Fatal(err) + } + got, _ = s.FindIdentity(ctx, j.ID) + if got.Handle != "bot2" || got.DisplayName != "GitLab connection" || got.ExpiresAt != nil { t.Errorf("empty values overwrote: %+v", got) } - if err := s.UpdateIdentityProfile(ctx, uuid.New(), "x", "", nil); !errors.Is(err, gitdom.ErrNotFound) { + // An unknown identity is a no-op. + if err := s.UpdateIdentityProfile(ctx, uuid.New(), "x", "", nil); err != nil { t.Errorf("unknown identity: %v", err) } } From 8a3154691904456a8f6fe9dd36b5058ae22f2589 Mon Sep 17 00:00:00 2001 From: Alessandro Gorla Date: Mon, 2 Nov 2026 11:08:31 +0100 Subject: [PATCH 7/8] fix(git): bound one listing's describe calls and mask their errors Each nameless identity had its own 5 second timeout, so several unreachable ones could hold a listing for 5 seconds each, one after the other. Share the timeout across the listing and leave the rest for the next one, without a backoff since they were never asked. The logged error comes from the plugin, so mask it like any provider message. Refs Paca-AI/paca#661 Co-Authored-By: Claude Opus 5.5 --- .../api/internal/service/git/describe_test.go | 18 ++++++++++++ services/api/internal/service/git/setup.go | 28 +++++++++++-------- 2 files changed, 35 insertions(+), 11 deletions(-) diff --git a/services/api/internal/service/git/describe_test.go b/services/api/internal/service/git/describe_test.go index f2934303b..b083a1793 100644 --- a/services/api/internal/service/git/describe_test.go +++ b/services/api/internal/service/git/describe_test.go @@ -66,6 +66,24 @@ func TestDescribeNameless_ProviderErrorKeepsRequestOK(t *testing.T) { } } +func TestDescribeNameless_StopsWhenTheBudgetRunsOut(t *testing.T) { + svc, st, pr := describingSetup(t, true) + st.addIdentity("", "tok-1") + st.addIdentity("", "tok-2") + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + pr.describe = func(gitprovider.DescribeIdentityRequest) (gitprovider.Identity, error) { + cancel() // the budget is gone once the first call returns + return gitprovider.Identity{}, errors.New("timed out") + } + if _, err := svc.ListIdentities(ctx); err != nil { + t.Fatal(err) + } + if len(pr.describeLog) != 1 || len(svc.describeFailed) != 1 { + t.Fatalf("calls=%d backoffs=%d, want 1 of each", len(pr.describeLog), len(svc.describeFailed)) + } +} + func TestDescribeNameless_SkipsWhenNotNeeded(t *testing.T) { t.Run("handle present", func(t *testing.T) { svc, st, pr := describingSetup(t, true) diff --git a/services/api/internal/service/git/setup.go b/services/api/internal/service/git/setup.go index a342e4e84..58272d2aa 100644 --- a/services/api/internal/service/git/setup.go +++ b/services/api/internal/service/git/setup.go @@ -334,8 +334,9 @@ func (s *Service) ListConnections(ctx context.Context, projectID uuid.UUID) ([]* return out, nil } -// describeTimeout bounds one describe_identity call: listings must not hang on -// a slow platform. +// describeTimeout bounds all the describe_identity calls of one listing, however +// many identities it has to name: listings must not hang on a slow platform. +// Whatever it leaves unnamed is asked on the next listing. const describeTimeout = 5 * time.Second // describeBackoff is how long after a failed describe the same identity is not @@ -350,40 +351,45 @@ const describeBackoff = 15 * time.Minute // per identity: after it the stored handle is no longer empty. It is best // effort: any failure is logged and the caller carries on with the old values. func (s *Service) describeNameless(ctx context.Context, ids []*gitdom.Identity) { + budget, cancel := context.WithTimeout(ctx, describeTimeout) + defer cancel() canDescribe := map[string]bool{} // provider id → declares identity_description for _, id := range ids { if id.Handle != "" { continue } + if budget.Err() != nil { + return // not a failure of this identity, so no backoff either + } ok, seen := canDescribe[id.ProviderID] if !seen { - info, err := s.providers.Describe(ctx, id.ProviderID) + info, err := s.providers.Describe(budget, id.ProviderID) ok = err == nil && info.Capabilities.IdentityDescription canDescribe[id.ProviderID] = ok } if ok { - s.describeIdentity(ctx, id) + s.describeIdentity(ctx, budget, id) } } } -func (s *Service) describeIdentity(ctx context.Context, id *gitdom.Identity) { +// describeIdentity asks within budget but saves with ctx, so an answer that +// arrives just in time is not lost. +func (s *Service) describeIdentity(ctx, budget context.Context, id *gitdom.Identity) { s.mu.Lock() failedAt, failed := s.describeFailed[id.ID] s.mu.Unlock() if failed && s.now().Sub(failedAt) < describeBackoff { return } - prov, err := s.providers.Provider(ctx, id.ProviderID) + prov, err := s.providers.Provider(budget, id.ProviderID) if err != nil { return } - cctx, cancel := context.WithTimeout(ctx, describeTimeout) - defer cancel() - got, err := prov.DescribeIdentity(cctx, gitprovider.DescribeIdentityRequest{Identity: identityRef(id)}) + got, err := prov.DescribeIdentity(budget, gitprovider.DescribeIdentityRequest{Identity: identityRef(id)}) if err != nil { - // The error is the provider's own message, never a credential. - slog.Warn("git: describe identity failed", "provider", id.ProviderID, "identity", id.ID, "err", err) + // The text comes from the plugin, so it is masked like any provider message. + slog.Warn("git: describe identity failed", "provider", id.ProviderID, "identity", id.ID, "err", RedactSecrets(err.Error())) s.mu.Lock() s.describeFailed[id.ID] = s.now() s.mu.Unlock() From 315a51eb56fbdfbc33f21f45b8ced76b882c3667 Mon Sep 17 00:00:00 2001 From: Alessandro Gorla Date: Tue, 3 Nov 2026 10:18:25 +0100 Subject: [PATCH 8/8] fix(git): back off after unusable describe answers, not after caller cancel An ok answer with another ref or an empty handle now counts as a failure (logged, identity paused for 15 minutes) instead of clearing the backoff. A request cancelled by the client no longer pauses the identity; running out of the shared budget still does. Tests cover a real shared deadline over several identities and that the logged error is masked. Co-Authored-By: Claude Opus 5.5 --- docs/plugins/git-provider-spi.md | 2 +- .../api/internal/service/git/describe_test.go | 72 +++++++++++++++++-- .../api/internal/service/git/service_test.go | 6 +- services/api/internal/service/git/setup.go | 26 ++++--- 4 files changed, 91 insertions(+), 15 deletions(-) diff --git a/docs/plugins/git-provider-spi.md b/docs/plugins/git-provider-spi.md index 940a346b7..82135b209 100644 --- a/docs/plugins/git-provider-spi.md +++ b/docs/plugins/git-provider-spi.md @@ -82,7 +82,7 @@ Trigger configuration, routing, actor policy, run lifecycle, debounce, comment/t * **When core calls it**: while listing connections (a project's Settings → Git) or the workspace's shared identities, for each identity whose stored handle is empty and whose provider declares `identity_description`. Once per identity, because the answer is stored; an identity that already has a handle is never described. * **What you return**: an `Identity` whose `Ref.ID` equals the request's, with `Ref.Handle` set (the login users `@mention`), `DisplayName`, and `ExpiresAt` for credentials that expire. `Next` is ignored. Unknown identity: `not_found`; credentials the platform no longer accepts: `forbidden`. * **What core stores**: the handle, and the display name and expiry when non-empty. An answer with another `Ref.ID` or an empty handle is discarded. -* **Failure**: best effort. Core bounds the call (5 s), logs the error without secrets and serves the listing with the old values; after a failure core does not ask about that identity again for 15 minutes (in memory, reset on restart), then retries. +* **Failure**: best effort. Core bounds all the calls of one listing to 5 s in total (not per call), logs the error without secrets and serves the listing with the old values. After a failure, or an answer it cannot use (another `Ref.ID`, empty handle), core does not ask about that identity again for 15 minutes (in memory, reset on restart), then retries. A request the client cancels is not counted as a failure. * **For migrations**: leave `handle` empty (and a generic `display_name`) in the SQL and implement `DescribeIdentity` by calling the platform's "current user" endpoint with the stored credential. ## Access grants (optional) diff --git a/services/api/internal/service/git/describe_test.go b/services/api/internal/service/git/describe_test.go index b083a1793..338fb4160 100644 --- a/services/api/internal/service/git/describe_test.go +++ b/services/api/internal/service/git/describe_test.go @@ -1,8 +1,11 @@ package gitsvc import ( + "bytes" "context" "errors" + "log/slog" + "strings" "testing" "time" @@ -66,21 +69,73 @@ func TestDescribeNameless_ProviderErrorKeepsRequestOK(t *testing.T) { } } -func TestDescribeNameless_StopsWhenTheBudgetRunsOut(t *testing.T) { +func TestDescribeNameless_CallerCancelDoesNotBackOff(t *testing.T) { svc, st, pr := describingSetup(t, true) st.addIdentity("", "tok-1") st.addIdentity("", "tok-2") ctx, cancel := context.WithCancel(context.Background()) defer cancel() pr.describe = func(gitprovider.DescribeIdentityRequest) (gitprovider.Identity, error) { - cancel() // the budget is gone once the first call returns - return gitprovider.Identity{}, errors.New("timed out") + cancel() // the client closes the request during the first call + return gitprovider.Identity{}, errors.New("context canceled") } if _, err := svc.ListIdentities(ctx); err != nil { t.Fatal(err) } - if len(pr.describeLog) != 1 || len(svc.describeFailed) != 1 { - t.Fatalf("calls=%d backoffs=%d, want 1 of each", len(pr.describeLog), len(svc.describeFailed)) + if len(pr.describeLog) != 1 || len(svc.describeFailed) != 0 { + t.Fatalf("calls=%d backoffs=%d, want 1 call and no backoff", len(pr.describeLog), len(svc.describeFailed)) + } +} + +// One 5 s budget covers the whole listing: with a timeout per call, three +// identities that never answer would each be asked. +func TestDescribeNameless_BudgetIsSharedAcrossIdentities(t *testing.T) { + old := describeTimeout + describeTimeout = 50 * time.Millisecond + defer func() { describeTimeout = old }() + svc, st, pr := describingSetup(t, true) + for _, tok := range []string{"tok-1", "tok-2", "tok-3"} { + st.addIdentity("", tok) + } + pr.describeCtx = func(ctx context.Context, _ gitprovider.DescribeIdentityRequest) (gitprovider.Identity, error) { + <-ctx.Done() // a platform that never answers + return gitprovider.Identity{}, ctx.Err() + } + start := time.Now() + if _, err := svc.ListIdentities(context.Background()); err != nil { + t.Fatal(err) + } + if len(pr.describeLog) != 1 { + t.Fatalf("calls = %d, want 1: the budget ran out in the first", len(pr.describeLog)) + } + if el := time.Since(start); el > 5*time.Second { + t.Errorf("listing took %v", el) + } + // The call that ran out of budget is paused, the ones never asked are not. + if len(svc.describeFailed) != 1 { + t.Errorf("backoffs = %d, want 1", len(svc.describeFailed)) + } +} + +func TestDescribeNameless_LogMasksSecrets(t *testing.T) { + var buf bytes.Buffer + old := slog.Default() + slog.SetDefault(slog.New(slog.NewTextHandler(&buf, nil))) + defer slog.SetDefault(old) + svc, st, pr := describingSetup(t, true) + st.addIdentity("", "tok-1") + pr.describe = func(gitprovider.DescribeIdentityRequest) (gitprovider.Identity, error) { + return gitprovider.Identity{}, errors.New("401 for glpat-abcdefghij1234567890") + } + if _, err := svc.ListIdentities(context.Background()); err != nil { + t.Fatal(err) + } + out := buf.String() + if !strings.Contains(out, "describe identity failed") { + t.Fatalf("no log line: %q", out) + } + if strings.Contains(out, "glpat-abcdefghij1234567890") { + t.Errorf("token leaked in the log: %q", out) } } @@ -116,6 +171,13 @@ func TestDescribeNameless_IgnoresUnusableAnswers(t *testing.T) { if id.Handle != "" || id.DisplayName != "" || st.profileUpdates != 0 { t.Errorf("stored an unusable answer: %+v saves=%d", id, st.profileUpdates) } + // It counts as a failure: the next listing does not ask again. + if len(pr.describeLog) != 1 || len(svc.describeFailed) != 1 { + t.Fatalf("calls=%d backoffs=%d, want 1 of each", len(pr.describeLog), len(svc.describeFailed)) + } + if _, err := svc.ListIdentities(context.Background()); err != nil || len(pr.describeLog) != 1 { + t.Errorf("asked again within the backoff: err=%v calls=%d", err, len(pr.describeLog)) + } }) } } diff --git a/services/api/internal/service/git/service_test.go b/services/api/internal/service/git/service_test.go index 3de0b1abb..9d50aedf6 100644 --- a/services/api/internal/service/git/service_test.go +++ b/services/api/internal/service/git/service_test.go @@ -358,10 +358,14 @@ type fakeProv struct { describe func(gitprovider.DescribeIdentityRequest) (gitprovider.Identity, error) describeLog []gitprovider.DescribeIdentityRequest + describeCtx func(context.Context, gitprovider.DescribeIdentityRequest) (gitprovider.Identity, error) } -func (p *fakeProv) DescribeIdentity(_ context.Context, r gitprovider.DescribeIdentityRequest) (gitprovider.Identity, error) { +func (p *fakeProv) DescribeIdentity(ctx context.Context, r gitprovider.DescribeIdentityRequest) (gitprovider.Identity, error) { p.describeLog = append(p.describeLog, r) + if p.describeCtx != nil { + return p.describeCtx(ctx, r) + } return p.describe(r) } diff --git a/services/api/internal/service/git/setup.go b/services/api/internal/service/git/setup.go index 58272d2aa..e262625bb 100644 --- a/services/api/internal/service/git/setup.go +++ b/services/api/internal/service/git/setup.go @@ -337,7 +337,7 @@ func (s *Service) ListConnections(ctx context.Context, projectID uuid.UUID) ([]* // describeTimeout bounds all the describe_identity calls of one listing, however // many identities it has to name: listings must not hang on a slow platform. // Whatever it leaves unnamed is asked on the next listing. -const describeTimeout = 5 * time.Second +var describeTimeout = 5 * time.Second // a variable only so tests can shorten it // describeBackoff is how long after a failed describe the same identity is not // asked again (a revoked token or a platform outage would otherwise slow every @@ -388,20 +388,24 @@ func (s *Service) describeIdentity(ctx, budget context.Context, id *gitdom.Ident } got, err := prov.DescribeIdentity(budget, gitprovider.DescribeIdentityRequest{Identity: identityRef(id)}) if err != nil { + if ctx.Err() != nil { + return // the caller gave up (client closed the request): the provider is not at fault + } // The text comes from the plugin, so it is masked like any provider message. slog.Warn("git: describe identity failed", "provider", id.ProviderID, "identity", id.ID, "err", RedactSecrets(err.Error())) - s.mu.Lock() - s.describeFailed[id.ID] = s.now() - s.mu.Unlock() + s.backOffDescribe(id.ID) return } - s.mu.Lock() - delete(s.describeFailed, id.ID) - s.mu.Unlock() - // An answer for another identity, or without a handle, must not be stored. + // An answer for another identity, or without a handle, is as useless as an + // error: it is not stored and the identity is not asked again right away. if got.Ref.ID != id.ProviderIdentityID || got.Ref.Handle == "" { + slog.Warn("git: describe identity gave an unusable answer", "provider", id.ProviderID, "identity", id.ID) + s.backOffDescribe(id.ID) return } + s.mu.Lock() + delete(s.describeFailed, id.ID) + s.mu.Unlock() var exp *time.Time if !got.ExpiresAt.IsZero() { exp = &got.ExpiresAt @@ -419,6 +423,12 @@ func (s *Service) describeIdentity(ctx, budget context.Context, id *gitdom.Ident } } +func (s *Service) backOffDescribe(id uuid.UUID) { + s.mu.Lock() + s.describeFailed[id] = s.now() + s.mu.Unlock() +} + func (s *Service) enabledProviders(ctx context.Context) (map[string]bool, error) { infos, err := s.providers.List(ctx) if err != nil {