feat(invitation): emit ResourceDoesNotExist marker on confirmed-absent delete (SPEC-09a)#180
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| 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{}) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: In a multi-org config, if one org returns 404 (isRemoved=true, wasCancelled stays false) while another returns a non-404 real error (e.g. 500), the loop still exits with isRemoved=true and emits ResourceDoesNotExist. That overstates the "authoritatively confirmed absent" semantics since one org's state was never determined. Low impact (invitation IDs are org-scoped and single-org is typical, and the pre-existing loop already ignored that 500), but you may want to only emit the marker when no non-404 error occurred across orgs. (confidence: low)
Connector PR Review: feat(invitation): emit ResourceDoesNotExist marker on confirmed-absent delete (SPEC-09a)Blocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0 Review SummaryScanned the full PR diff for security and correctness. The change adds the typed already-absent marker to invitationResourceType.Delete only on the pure-absence success path, gated by wasCancelled/hadRealError flags so it fires solely when every configured org authoritatively returned 404 with no active cancellation and no non-404 error. The marker logic is order-independent and correct, error paths and addressing are unchanged, helpers extractRateLimitData and isNotFoundError are nil-safe, no go.mod or SDK changes, and the new invitation_delete_test.go gives good coverage: confirmed absence, ordinary success, 500 error, exact addressing, malformed id, wrong type, and multi-org 404 plus 5xx. No new issues found. Security IssuesNone found. Correctness IssuesNone found. SuggestionsNone. |
|
Thanks — addressed the multi-org suggestion in Note: the |
|
Heads up: the red |
…t 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] <c1-squire-dev[bot]@users.noreply.github.com>
…n-404 error 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] <c1-squire-dev[bot]@users.noreply.github.com>
72e0b83 to
9437b89
Compare
1 participant
What
Make the invitation
ResourceDeleteremit the typedc1.connector.v2.ResourceDoesNotExist{}marker (withnilerror) when the provider authoritatively confirms the addressed invitation is already absent, instead of returning an untyped success.This is the second connector producer of the SPEC-09a already-absent delete outcome (after baton-aws#140), and the first non-AWS one.
Why
Per pqprime SPEC-09a (REQ-VPQ-009A) and conceptually the baton-sdk contract in baton-sdk#1033: a
ResourceDeleter(V2)whose provider call authoritatively confirms the correctly-addressed resource is already gone should returnannotations.New(&v2.ResourceDoesNotExist{}), nil— the resource-plane analogue ofGrantAlreadyRevoked.This lets C1 distinguish a confirmed already-gone delete (typed success ⇒
REVOKED) from an opaque / mis-addressedNotFound(untyped error ⇒REVOKE_UNKNOWN), so a delete retried after a crash is classified correctly rather than ambiguously. There is no code dependency on #1033 — the marker type is already vendored in baton-sdkv0.19.1; this PR is independently mergeable.Change
pkg/connector/invitation.go—invitationResourceType.Deletealready treated aCancelInvite404 as success (desired state achieved). The only new behavior: the marker is appended solely on the pure-absence success path — i.e. no active cancellation succeeded across the configured orgs, yet the invitation is considered removed because a well-formed invitation id returned 404 against a configured org.CancelInvitesucceeded) remains success and carries no marker.NotFoundis swallowed and no error path is broadened.wasCancelledflag, so a real cancellation in one org is never masked by a 404 in another.Intentionally excluded: the user deleter
userResourceType.Deletecurrently surfaces 404 as an error (wrapGitHubError, on bothUsers.GetByIDandRemoveOrgMembership) and has no existing already-absent success branch. Adding the marker there would broaden behavior, which SPEC-09a forbids for this step; it is left for a separate, deliberate change.No dependency churn
No
go.mod/ SDK bump. Theresource.ResourceDoesNotExistAnnotations()helper from #1033 is unreleased (absent from vendoredv0.19.1), so the marker is constructed directly (annotations.New/Append+&v2.ResourceDoesNotExist{}) — both aliases were already imported. A code comment notes swapping to the helper after a future SDK bump. No version files touched.Tests
New
pkg/connector/invitation_delete_test.go(go-github-mock inline harness, matchinginvitation_test.go):Contains(ResourceDoesNotExist), nil errorDELETE /orgs/test-org-12/invitations/5551212Verified:
gofmtclean,go build ./...,go test -race ./pkg/connector/green (focused + full package, no regressions),golangci-lintadds zero new issues.Draft: opened as part of the SPEC-09a connector rollout for review before merge.
🤖 Generated with Claude Code