feat: emit PENDING status on invitation resources - #184
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // attributes. Expired invitations stay PENDING - they are still not a | ||
| // usable account - and carry the distinction in the status details. | ||
| resourceSdk.WithResourceProfile(profile), | ||
| resourceSdk.WithResourceStatus(v2.Status_RESOURCE_STATUS_PENDING, status), |
There was a problem hiding this comment.
🟡 Suggestion: docs/connector.mdx:26 still says "if an invitation is pending, the account status will be shown as Unspecified", which this change makes wrong. Update it to Pending in this PR so the docs don't lag the emitted status.
Separately, both status fields carry defined_only enum validation, so a reader still on the pre-PENDING proto descriptor would reject enum value 4 rather than fall back to unspecified. The PR description covers the c1-first rollout ordering, but nothing in the connector enforces it — worth confirming that ordering is tracked before this leaves draft, since there is no config gate to fall back to.
There was a problem hiding this comment.
Docs fixed in 382a57f — connector.mdx now says Pending, with the expired-invitations nuance (also Pending, distinguished by invitation_expired in status details).
On the defined_only ordering concern: the situation changed since this review ran. c1 PR ductone/c1#24091 was narrowed to treat only explicit STATUS_PENDING as pending (UNSPECIFIED turned out to be emitted for real accounts by other connectors). We also verified c1's c1z ingestion runs no enum validation on synced resources — a pre-bump c1 decodes value 4 and maps it to UNSPECIFIED via the existing default arm, i.e. today's behavior exactly. So no ordering is unsafe and no config gate is needed; the feature simply stays inert until c1 ships its vendored-SDK bump mapping PENDING. The PR description's Rollout section is updated with this.
|
|
||
| require ( | ||
| github.com/conductorone/baton-sdk v0.24.6 | ||
| github.com/conductorone/baton-sdk v0.25.1-0.20260825204020-991ca45253a7 |
There was a problem hiding this comment.
🟡 Suggestion: two things on this bump.
- It pins an unreleased pseudo-version (
v0.25.1-0.20260825204020-991ca45253a7). The PR already commits to swapping to the tagged release before undrafting — flagging so it isn't lost. - The bump is scoped in the description to "adds the two PENDING enum values", but v0.24.6 → v0.25.x also changes runtime behavior the connector inherits: the default dotc1z storage engine flips from SQLite to Pebble for new files (
pkg/dotc1z/engine_registry.go,pkg/field/defaults.go), and the--diff-syncs/--base-sync-id/--applied-sync-idflags are removed along with diff-sync support. Nothing in this repo references those flags, so there is no build or CI break, but the storage-engine default change affects the artifact this connector produces and is worth calling out in the PR description and release notes rather than shipping silently.
There was a problem hiding this comment.
Both addressed in the PR description:
-
Pseudo-version: acknowledged and tracked — the pin stays while feat: add PENDING status for invitation-style principals baton-sdk#1104 is unreleased (its branch has since advanced by two test/comment-only commits, 8a66dfb and 9de8e65, with no API change, so the pin needn't chase them); the swap to the tagged release remains the gate for undrafting, isolated as its own commit.
-
The SDK bump's behavior changes are now documented in a dedicated "SDK bump: behavior beyond the enum" section — the SQLite→Pebble default engine flip for new c1z files (verified against vendored pkg/field/defaults.go) and the removal of diff-sync flags — so they land in release notes rather than silently.
Connector PR Review: feat: emit PENDING status on invitation resourcesBlocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0 Review SummaryThe new commit ( Security IssuesNone found. Correctness IssuesNone found. SuggestionsNone. |
Invitation resources previously emitted STATUS_UNSPECIFIED via the
deprecated trait-level WithStatus option, because the SDK had no way to
express "pending" and NewUserTrait force-defaults an unset trait status
to ENABLED.
Now that the SDK has a PENDING value, emit it explicitly:
- resource level via the non-deprecated WithResourceStatus, matching
the accepted-member path in user.go
- trait level via WithDetailedStatus, which is still required to
override NewUserTrait's ENABLED default
Both levels carry the invitation_status value (pending acceptance vs
expired) as status details. Expired invitations stay PENDING rather than
getting a distinct enum value: an expired invite is still not a usable
account, and the details field preserves the distinction.
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
382a57f to
995a4a1
Compare
| // Set explicitly: NewUserTrait defaults an unset trait status to | ||
| // ENABLED, which an unaccepted invitation is not. | ||
| //nolint:staticcheck // trait status is deprecated but must be set to override the ENABLED default. | ||
| resourceSdk.WithDetailedStatus(v2.UserTrait_Status_STATUS_PENDING, status), |
There was a problem hiding this comment.
do we need this now that we have Status_RESOURCE_STATUS_PENDING for WithResourceStatus?
There was a problem hiding this comment.
we've been using these type of helpers to prioritize resource traits over user,group,etc traits https://github.com/ConductorOne/baton-sdk/blob/94143a9f85b525bbaf28922560aae978d460c5eb/pkg/types/resource/resource_attrs.go#L78
There was a problem hiding this comment.
You're right — dropped in 487f98d. We audited every trait-status reader across baton-sdk and c1 before conceding: all of them resolve resource-first (baton-sdk's GetStatus never consults the trait when the resource status is set; c1's uplift and ProvisionAppUser paths go through that same helper; the CLI csv/xlsx exporters too), so the trait set was belt-and-suspenders with no reader. One artifact to be aware of: the trait status now falls to NewUserTrait's ENABLED force-default, so the raw trait annotation in a c1z reads ENABLED next to the PENDING resource status — no code path reads it, and the tests pin it so a change gets caught. Dropping it also removed the file's last deprecated-API nolint.
There was a problem hiding this comment.
Agreed — GetStatus's resource-first precedence is exactly what makes this safe, and it's now the documented contract: the tests assert the resolved status via that helper rather than reading the trait. Possible SDK follow-up if the force-defaulted ENABLED trait bothers anyone as stored data: skip the ENABLED default in NewUserTrait when the resource already carries an explicit status, so the trait would be genuinely absent instead of falsely ENABLED.
Consumers resolve status resource-first via GetStatus, which never consults the trait when the resource status is set. Audited every reader in baton-sdk and c1: none reads the trait directly. The trait falls to NewUserTrait's ENABLED default, pinned in tests as a known artifact readers must not consult. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
What changes
Pending GitHub org invitations are modeled as a dedicated
invitationresource type. Until now they had no way to say "pending": the connector emittedUserTrait_Status_STATUS_UNSPECIFIEDthrough the deprecatedWithStatustrait option, with a//nolint:staticchecksuppression, because the SDK had no PENDING value andNewUserTraitforce-defaults an unset trait status toENABLED.The SDK now has that value, so invitations emit it explicitly:
WithResourceStatus(v2.Status_RESOURCE_STATUS_PENDING, <detail>), the same non-deprecated option the accepted-member path inuser.gouses. Per review, the trait-level status is deliberately not set: every consumer resolves status resource-first via the SDK'sGetStatus(audited across baton-sdk tooling and c1's uplift/provisioning paths — no direct trait reader exists). The trait falls toNewUserTrait's ENABLED force-default, which the tests pin as a known artifact that must not be read directly. This also removes the file's last deprecated-API//nolint.The
<detail>string is the existinginvitation_statusvalue, so consumers can tell the two flavors apart:RESOURCE_STATUS_PENDINGinvitation_pending_acceptanceRESOURCE_STATUS_PENDINGinvitation_expiredExpired invitations stay PENDING rather than getting a distinct enum value. An expired invite is still not a usable account, and the details field preserves the distinction without inventing new enum semantics.
What changes for consumers
Invitation resources move from
STATUS_UNSPECIFIEDtoSTATUS_PENDINGat the resource level (resolve viaGetStatus; the trait-level status is a force-default artifact). Nothing else about the resource changes — profile, IDs, entitlements, grants, and provisioning are untouched.Dependency
Depends on ConductorOne/baton-sdk#1104Resolved: #1104 merged and is included in the releasedbaton-sdk v0.26.0, which this repo'smainalready uses. This branch was rebased onto that main and the pseudo-version pin commit was dropped entirely — the PR is now the four connector files only, with nogo.mod/vendor changes. The stated undraft gate (tagged SDK release) is satisfied.Rollout ordering
Updated: c1's PENDING handling (ductone/c1#24091) was narrowed during review to treat only explicit
STATUS_PENDINGas pending — explicitUNSPECIFIEDis deliberately not a pending signal, because other connectors (Cloudflare, MongoDB Atlas, and others) emit it for real, active accounts. Consequences for ordering:4and maps it toUNSPECIFIEDvia the existingdefault:arm — i.e., invitations behave exactly as they do today (defined_onlyrules matter only to consumers that callValidate(), which c1's read paths do not).RESOURCE_STATUS_PENDINGinNewAppUserStatusV2. Until all three are live, pending invitations keep today's behavior — no better, no worse.SDK bump: behavior beyond the enum
The SDK bump now arrives via this repo's
main(v0.26.0) rather than this PR, but for release notes:v0.24.6 → v0.26.0changes runtime behavior the connector inherits — the default dotc1z storage engine for newly created c1z files flips from SQLite to Pebble (pkg/field/defaults.go: "Defaults to pebble when unset"; overridable via the storage-engine flag), and diff-sync support (--diff-syncs/--base-sync-id/--applied-sync-id) is removed. Nothing in this repo references the removed flags.Testing
pkg/connector/invitation_test.goasserts both status levels for both fixtures via a newrequireInvitationPendinghelper. Verified the trait-level assertion is load-bearing: removing theWithDetailedStatusline makes the trait status come back as1(ENABLED) instead of4(PENDING) while the resource level stays PENDING — exactly the trap the removed comment described — and three subtests fail.🤖 Generated with Claude Code