From df85013928b2bcdafd54b41631aab09c7f17b608 Mon Sep 17 00:00:00 2001 From: Paul Querna Date: Thu, 23 Jul 2026 06:24:32 +0000 Subject: [PATCH 1/2] feat(invitation): emit ResourceDoesNotExist marker on confirmed-absent delete The invitation ResourceDeleter already treats a provider 404 on CancelInvite as success (desired state achieved). Per SPEC-09a (REQ-VPQ-009A), a deleter that authoritatively confirms the correctly-addressed resource is already absent should return the typed c1.connector.v2.ResourceDoesNotExist{} marker (nil error) instead of an untyped success, so a delete retried after a crash is classified as REVOKED rather than an ambiguous NotFound. Only the confirmed-absence branch changes: the marker is emitted solely when no active cancellation succeeded yet the invitation is considered removed (a 404 for a well-formed invitation id against a configured org). An active cancellation remains ordinary success and carries no marker; every other provider/addressing error is unchanged. The user deleter is intentionally excluded: it surfaces 404 as an error (wrapGitHubError) and has no existing already-absent success branch, so adding the marker there would broaden behavior. No go.mod/SDK bump: the marker type is already vendored (baton-sdk v0.19.1); the resource.ResourceDoesNotExistAnnotations() helper from baton-sdk#1033 is unreleased, so the marker is constructed directly with a note to adopt the helper after a future SDK bump. Tests: new invitation_delete_test.go covers confirmed-absence -> marker, ordinary success -> no marker, other error unchanged, exact org/invitation-id addressing preserved, and malformed/wrong-type inputs. Co-authored-by: c1-squire-dev[bot] --- pkg/connector/invitation.go | 18 +++- pkg/connector/invitation_delete_test.go | 124 ++++++++++++++++++++++++ 2 files changed, 140 insertions(+), 2 deletions(-) create mode 100644 pkg/connector/invitation_delete_test.go diff --git a/pkg/connector/invitation.go b/pkg/connector/invitation.go index fe49b5c6..dd87f92e 100644 --- a/pkg/connector/invitation.go +++ b/pkg/connector/invitation.go @@ -346,14 +346,16 @@ func (i *invitationResourceType) Delete(ctx context.Context, resourceId *v2.Reso } var ( - isRemoved = false - resp *github.Response + isRemoved = false + wasCancelled = false + resp *github.Response ) for _, org := range orgs { resp, err = i.client.Organizations.CancelInvite(ctx, org, invitationID) if err == nil { isRemoved = true + wasCancelled = true continue } if isNotFoundError(resp) { @@ -374,6 +376,18 @@ func (i *invitationResourceType) Delete(ctx context.Context, resourceId *v2.Reso var annotations annotations.Annotations annotations.WithRateLimiting(restApiRateLimit) + if !wasCancelled { + // No active cancellation succeeded, yet the invitation is considered + // removed: the provider authoritatively reported it already absent + // (404) for a well-formed invitation id against a configured org. + // Emit the typed already-absent marker so a delete retried after a + // crash is classified as success rather than an ambiguous NotFound + // (SPEC-09a; baton-sdk#1033). An active cancellation is ordinary + // success and intentionally carries no marker. A future baton-sdk + // bump can replace this direct construction with + // resource.ResourceDoesNotExistAnnotations(). + annotations.Append(&v2.ResourceDoesNotExist{}) + } return annotations, nil } diff --git a/pkg/connector/invitation_delete_test.go b/pkg/connector/invitation_delete_test.go new file mode 100644 index 00000000..1ad5c185 --- /dev/null +++ b/pkg/connector/invitation_delete_test.go @@ -0,0 +1,124 @@ +package connector + +import ( + "context" + "net/http" + "testing" + + v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" + "github.com/google/go-github/v69/github" + "github.com/migueleliasweb/go-github-mock/src/mock" + "github.com/stretchr/testify/require" +) + +// TestInvitationDelete exercises invitationResourceType.Delete against the +// go-github-mock router (Pattern B: inline mock.NewMockedHTTPClient + +// github.NewClient). CancelInvite issues DELETE +// /orgs/{org}/invitations/{invitation_id}; getOrgs returns the configured +// orgs without any API call, so only the CancelInvite endpoint needs a +// handler. The already-absent marker (&v2.ResourceDoesNotExist{}) is asserted +// via annotations.Annotations.Contains. +func TestInvitationDelete(t *testing.T) { + ctx := context.Background() + + newBuilder := func(httpClient *http.Client) *invitationResourceType { + gh := github.NewClient(httpClient) + return InvitationBuilder(InvitationBuilderParams{ + client: gh, + orgCache: newOrgNameCache(gh), + orgs: []string{invitationTestOrgLogin}, + }) + } + + invitationResID := func(resource string) *v2.ResourceId { + return &v2.ResourceId{ + ResourceType: resourceTypeInvitation.Id, + Resource: resource, + } + } + + t.Run("confirmed absence emits marker", func(t *testing.T) { + client := mock.NewMockedHTTPClient( + mock.WithRequestMatchHandler( + mock.DeleteOrgsInvitationsByOrgByInvitationId, + notFoundHandler(), + ), + ) + + annos, err := newBuilder(client).Delete(ctx, invitationResID("5551212")) + require.NoError(t, err) + require.True(t, annos.Contains(&v2.ResourceDoesNotExist{}), + "a 404 with no active cancellation must emit the already-absent marker") + }) + + t.Run("ordinary success emits no marker", func(t *testing.T) { + client := mock.NewMockedHTTPClient( + mock.WithRequestMatchHandler( + mock.DeleteOrgsInvitationsByOrgByInvitationId, + http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNoContent) + }), + ), + ) + + annos, err := newBuilder(client).Delete(ctx, invitationResID("5551212")) + require.NoError(t, err) + require.False(t, annos.Contains(&v2.ResourceDoesNotExist{}), + "an active cancellation (204) is ordinary success and must carry no marker") + }) + + t.Run("other error stays error, no marker", func(t *testing.T) { + client := mock.NewMockedHTTPClient( + mock.WithRequestMatchHandler( + mock.DeleteOrgsInvitationsByOrgByInvitationId, + http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + }), + ), + ) + + annos, err := newBuilder(client).Delete(ctx, invitationResID("5551212")) + require.Error(t, err) + require.Nil(t, annos, "a 500 is a real failure: no annotations and no marker") + }) + + t.Run("exact org and invitation id addressing preserved", func(t *testing.T) { + var gotPath string + client := mock.NewMockedHTTPClient( + mock.WithRequestMatchHandler( + mock.DeleteOrgsInvitationsByOrgByInvitationId, + http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotPath = r.URL.Path + w.WriteHeader(http.StatusNoContent) + }), + ), + ) + + annos, err := newBuilder(client).Delete(ctx, invitationResID("5551212")) + require.NoError(t, err) + require.Equal(t, "/orgs/test-org-12/invitations/5551212", gotPath, + "CancelInvite must address org=test-org-12 and id=5551212") + require.False(t, annos.Contains(&v2.ResourceDoesNotExist{})) + }) + + t.Run("malformed invitation id errors before any API call", func(t *testing.T) { + // No CancelInvite handler: the parse failure must short-circuit before + // any HTTP request is issued. + client := mock.NewMockedHTTPClient() + + annos, err := newBuilder(client).Delete(ctx, invitationResID("not-a-number")) + require.Error(t, err) + require.Nil(t, annos) + }) + + t.Run("wrong resource type errors", func(t *testing.T) { + client := mock.NewMockedHTTPClient() + + annos, err := newBuilder(client).Delete(ctx, &v2.ResourceId{ + ResourceType: resourceTypeUser.Id, + Resource: "5551212", + }) + require.Error(t, err) + require.Nil(t, annos) + }) +} From 9437b8980fe2f6e7b30bd25e94d69d111b29234f Mon Sep 17 00:00:00 2001 From: Paul Querna Date: Thu, 23 Jul 2026 06:32:54 +0000 Subject: [PATCH 2/2] refactor(invitation): withhold already-absent marker on unresolved non-404 error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address the automated pr-review suggestion: in a multi-org config, a 404 in one org combined with a non-404 error (e.g. 5xx) in another previously still emitted ResourceDoesNotExist, slightly overstating "confirmed absent." Track a hadRealError flag and emit the marker only when !wasCancelled && !hadRealError, so the typed already-absent outcome strictly means the provider authoritatively confirmed absence with no competing undetermined state — matching SPEC-09a's no-false-positives rule. The pre-existing success/error boundary is unchanged (a 404 still sets isRemoved, so the call still returns success); only the marker emission is tightened. Adds a multi-org test asserting the 404+500 case keeps success but carries no marker. Co-authored-by: c1-squire-dev[bot] --- pkg/connector/invitation.go | 26 +++++++++++-------- pkg/connector/invitation_delete_test.go | 33 +++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 10 deletions(-) diff --git a/pkg/connector/invitation.go b/pkg/connector/invitation.go index dd87f92e..a3a309f7 100644 --- a/pkg/connector/invitation.go +++ b/pkg/connector/invitation.go @@ -348,6 +348,7 @@ func (i *invitationResourceType) Delete(ctx context.Context, resourceId *v2.Reso var ( isRemoved = false wasCancelled = false + hadRealError = false resp *github.Response ) @@ -362,7 +363,10 @@ func (i *invitationResourceType) Delete(ctx context.Context, resourceId *v2.Reso // Invitation is already gone (expired or previously cancelled). // Desired state is achieved, so treat as success. isRemoved = true + continue } + // A non-404 failure (e.g. 5xx) leaves this org's state undetermined. + hadRealError = true } if !isRemoved { @@ -376,16 +380,18 @@ func (i *invitationResourceType) Delete(ctx context.Context, resourceId *v2.Reso var annotations annotations.Annotations annotations.WithRateLimiting(restApiRateLimit) - if !wasCancelled { - // No active cancellation succeeded, yet the invitation is considered - // removed: the provider authoritatively reported it already absent - // (404) for a well-formed invitation id against a configured org. - // Emit the typed already-absent marker so a delete retried after a - // crash is classified as success rather than an ambiguous NotFound - // (SPEC-09a; baton-sdk#1033). An active cancellation is ordinary - // success and intentionally carries no marker. A future baton-sdk - // bump can replace this direct construction with - // resource.ResourceDoesNotExistAnnotations(). + if !wasCancelled && !hadRealError { + // No active cancellation succeeded and no org returned a non-404 + // error, yet the invitation is considered removed: the provider + // authoritatively reported it already absent (404) for a well-formed + // invitation id against a configured org. Emit the typed already-absent + // marker so a delete retried after a crash is classified as success + // rather than an ambiguous NotFound (SPEC-09a; baton-sdk#1033). An + // active cancellation is ordinary success and intentionally carries no + // marker; an unresolved non-404 error means absence was not confirmed, + // so the marker is withheld even though the pre-existing success return + // is preserved. A future baton-sdk bump can replace this direct + // construction with resource.ResourceDoesNotExistAnnotations(). annotations.Append(&v2.ResourceDoesNotExist{}) } return annotations, nil diff --git a/pkg/connector/invitation_delete_test.go b/pkg/connector/invitation_delete_test.go index 1ad5c185..80179255 100644 --- a/pkg/connector/invitation_delete_test.go +++ b/pkg/connector/invitation_delete_test.go @@ -3,6 +3,7 @@ package connector import ( "context" "net/http" + "strings" "testing" v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" @@ -101,6 +102,38 @@ func TestInvitationDelete(t *testing.T) { require.False(t, annos.Contains(&v2.ResourceDoesNotExist{})) }) + t.Run("multi-org: 404 plus a non-404 error withholds the marker but keeps success", func(t *testing.T) { + // One configured org reports the invitation already absent (404) while + // another returns a transient 5xx (state undetermined). The pre-existing + // behavior returns success (isRemoved is set by the 404), but absence was + // not authoritatively confirmed everywhere, so no marker is emitted. + const secondOrg = "test-org-99" + client := mock.NewMockedHTTPClient( + mock.WithRequestMatchHandler( + mock.DeleteOrgsInvitationsByOrgByInvitationId, + http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if strings.Contains(r.URL.Path, secondOrg) { + w.WriteHeader(http.StatusInternalServerError) + return + } + w.WriteHeader(http.StatusNotFound) + }), + ), + ) + + gh := github.NewClient(client) + b := InvitationBuilder(InvitationBuilderParams{ + client: gh, + orgCache: newOrgNameCache(gh), + orgs: []string{invitationTestOrgLogin, secondOrg}, + }) + + annos, err := b.Delete(ctx, invitationResID("5551212")) + require.NoError(t, err) + require.False(t, annos.Contains(&v2.ResourceDoesNotExist{}), + "an unresolved non-404 error means absence was not confirmed: no marker") + }) + t.Run("malformed invitation id errors before any API call", func(t *testing.T) { // No CancelInvite handler: the parse failure must short-circuit before // any HTTP request is issued.