feat(cloudidentity): add set_members enrichment for authoritative group membership (#1313) #136

Merged

stack72 merged 2 commits from feat/cloudidentity-set-members-1313 into main 2026-07-24 00:05:36 +00:00

Conversation 2 Commits 2 Files changed 6 +358 -15

stack72 commented 2026-07-23 23:27:13 +00:00

Owner

Copy link

Summary

• Add a set_members enrichment method to the GCP cloudidentity groups-memberships model that reconciles membership to a supplied desired set — adds missing members, deletes members whose preferredMemberKey.id is not in the set
• Implemented via the GCP enrichment system (same pattern as IAM add_iam_binding/remove_iam_binding): new .enrich.ts source, metadata registration, codegen regeneration
• Updated codegen/designs/gcp.md section 18 to document the new enrichment

Closes #1313

Test plan

• deno task generate:gcp cloudidentity — set_members method appears in generated output
• Second generation run produces zero diff (idempotency verified)
• deno check on generated model passes
• deno lint on generated model passes
• deno install --frozen passes (lockfile integrity)
• Model version and manifest version both bumped to 2026.07.24.2 with upgrade stanzas

🤖 Generated with Claude Code

## Summary - Add a `set_members` enrichment method to the GCP cloudidentity `groups-memberships` model that reconciles membership to a supplied desired set — adds missing members, deletes members whose `preferredMemberKey.id` is not in the set - Implemented via the GCP enrichment system (same pattern as IAM `add_iam_binding`/`remove_iam_binding`): new `.enrich.ts` source, metadata registration, codegen regeneration - Updated `codegen/designs/gcp.md` section 18 to document the new enrichment Closes #1313 ## Test plan - [x] `deno task generate:gcp cloudidentity` — `set_members` method appears in generated output - [x] Second generation run produces zero diff (idempotency verified) - [x] `deno check` on generated model passes - [x] `deno lint` on generated model passes - [x] `deno install --frozen` passes (lockfile integrity) - [x] Model version and manifest version both bumped to `2026.07.24.2` with upgrade stanzas 🤖 Generated with [Claude Code](https://claude.com/claude-code)

stack72 added 1 commit 2026-07-23 23:27:13 +00:00

feat(cloudidentity): add set_members enrichment for authoritative group membership (#1313) ... 1d493063ed

Add a set_members method to the groups-memberships model that reconciles
membership to a supplied desired set — adds missing members, deletes
members not in the set. Implemented via the GCP enrichment system so
workflows can enforce an exact roster declaratively.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

stack72 added 1 commit 2026-07-23 23:50:55 +00:00

fix: strip trailing whitespace in gcp.md enrichment table ... 48c4805a45

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

stack72 commented 2026-07-24 00:02:42 +00:00

Author

Owner

Copy link

Code Review

Blocking Issues

None.

Suggestions

1. Empty-string member IDs pass validation (cloudidentity-groups-memberships.enrich.ts, line 15): z.string() accepts "". An empty-string id would be added to desiredIds and silently corrupt reconciliation — members with a missing preferredMemberKey.id would never match it (falsy guard on line 71 returns false), so they'd be excluded from toDelete, making the reconciliation non-authoritative for those edge members. Consider z.string().min(1) (or .email()) on the id field.

2. Silently skipped members with missing preferredMemberKey.id (cloudidentity-groups-memberships.enrich.ts, lines 70–73): The toDelete filter drops any current member whose preferredMemberKey.id is absent (return id && !desiredIds.has(id)). If the API ever returns a member without a preferredMemberKey.id (broken/partial response), that member is silently excluded from deletion. The reconciliation would then not be fully authoritative. This is likely an impossible API state, but a defensive comment or log noting the skip would aid future debugging.

3. No guard for empty parent (cloudidentity-groups-memberships.enrich.ts, line 38): const parent = String(g["parent"] ?? "") silently allows an empty string. The subsequent listResources call will produce a confusing GCP API error (e.g., INVALID_ARGUMENT: Invalid parent path) rather than a helpful message. A simple early check (if (!parent) throw new Error("parent is required")) would surface misconfiguration immediately.

4. Design doc authentication note not updated (codegen/designs/gcp.md, §8 Enrichment limitation): The existing limitation note calls out serviceaccounts and storage-buckets as enrichments that call request() without forwarding explicit credentials. The new cloudidentity-groups-memberships enrichment does properly thread credentials through all API calls (listResources, createResource, deleteResource). This is strictly better than the existing enrichments; noting this in the doc (or updating the limitation note to reflect the pattern has been fixed in newer enrichments) would help future enrichment authors follow the newer approach.

## Code Review ### Blocking Issues None. ### Suggestions 1. **Empty-string member IDs pass validation** (`cloudidentity-groups-memberships.enrich.ts`, line 15): `z.string()` accepts `""`. An empty-string `id` would be added to `desiredIds` and silently corrupt reconciliation — members with a missing `preferredMemberKey.id` would never match it (falsy guard on line 71 returns false), so they'd be excluded from `toDelete`, making the reconciliation non-authoritative for those edge members. Consider `z.string().min(1)` (or `.email()`) on the `id` field. 2. **Silently skipped members with missing `preferredMemberKey.id`** (`cloudidentity-groups-memberships.enrich.ts`, lines 70–73): The `toDelete` filter drops any current member whose `preferredMemberKey.id` is absent (`return id && !desiredIds.has(id)`). If the API ever returns a member without a `preferredMemberKey.id` (broken/partial response), that member is silently excluded from deletion. The reconciliation would then not be fully authoritative. This is likely an impossible API state, but a defensive comment or log noting the skip would aid future debugging. 3. **No guard for empty `parent`** (`cloudidentity-groups-memberships.enrich.ts`, line 38): `const parent = String(g["parent"] ?? "")` silently allows an empty string. The subsequent `listResources` call will produce a confusing GCP API error (e.g., `INVALID_ARGUMENT: Invalid parent path`) rather than a helpful message. A simple early check (`if (!parent) throw new Error("parent is required")`) would surface misconfiguration immediately. 4. **Design doc authentication note not updated** (`codegen/designs/gcp.md`, §8 Enrichment limitation): The existing limitation note calls out `serviceaccounts` and `storage-buckets` as enrichments that call `request()` without forwarding explicit credentials. The new `cloudidentity-groups-memberships` enrichment does properly thread `credentials` through all API calls (`listResources`, `createResource`, `deleteResource`). This is strictly better than the existing enrichments; noting this in the doc (or updating the limitation note to reflect the pattern has been fixed in newer enrichments) would help future enrichment authors follow the newer approach.

stack72 commented 2026-07-24 00:03:55 +00:00

Author

Owner

Copy link

Adversarial Review

Medium

1. Partial failure during delete phase leaves group in unrecoverable intermediate state with no diagnostic context — cloudidentity-groups-memberships.enrich.ts:110-121

   The create loop (lines 76-108) runs first and commits members to the group. Then the delete loop (lines 111-121) runs sequentially with no try/catch. If deleteResource throws on the Nth deletion (e.g., transient 503, permission denied on a specific member type, or quota exhaustion), the operation exits with an error, but:

   • All creates are already committed.
   • Deletes 1..N-1 are already committed.
   • Deletes N+1..end are skipped.
   • The caller receives only the thrown error — no information about what was partially applied (the created/deleted counts in the return value are never reached).

   Concrete scenario: Desired [A, B, C], current [B, D, E, F]. Creates [A, C] succeed. Delete D succeeds. Delete E fails (transient). Group is now [A, B, C, E, F] — neither original nor desired. The caller gets an opaque error with no indication that A and C were added and D was removed.

   This is consistent with how other enrichments handle errors (fail-forward, no rollback), but is more consequential here because the method is described as "authoritative reconciliation." A retry with the same inputs will converge (creates are idempotent via AlreadyExistsError handling, remaining deletes will be retried), but only if the caller knows to retry.

   Suggested improvement: Wrap the delete loop in a try/catch that captures partial progress and includes it in the thrown error, e.g.:

   } catch (deleteErr) {
     const partialError = new Error(
       `Partial reconciliation: created ${created.length}, deleted ${deleted.length}/${toDelete.length}. ` +
       `Remaining members were not removed. Original error: ${deleteErr}`
     );
     throw partialError;
   }
   

2. No concurrency control on the reconciliation window — cloudidentity-groups-memberships.enrich.ts:46-121

   The existing IAM enrichments (storage-buckets, serviceaccounts) use etag-based optimistic concurrency on their read-modify-write operations. This enrichment performs a list (line 46), then creates and deletes against the live group with no mechanism to detect concurrent modifications.

   Concrete scenario: Between the initial listResources (line 46) and the delete loop (line 111), another process adds member X to the group. X is not in desiredIds (it wasn't in the initial list, so it wasn't considered). The delete loop picks up X... actually no — the delete loop operates on toDelete which was computed from the initial list, so X wouldn't be in toDelete. But X would appear in the final reconciled list and be written as state, even though it's not in desiredMembers. The result claims total: N including X, making the reported reconciliation inaccurate.

   Conversely, if another process removes a member between the list and the delete loop, deleteResource will fail on a 404 for that member, causing the operation to throw mid-delete (see finding #1).

   The Cloud Identity Memberships API doesn't support etags on individual memberships, so there may not be a clean fix. But this is a limitation worth documenting, at minimum.

Low

3. Duplicate IDs in desiredMembers cause redundant API round-trips — cloudidentity-groups-memberships.enrich.ts:62,69

   desiredIds is built as a Set (deduplicating), but toCreate is filtered from the original desiredMembers array. If desiredMembers contains [{id: "a@x.com"}, {id: "a@x.com"}], toCreate will include both entries. The second create hits AlreadyExistsError, triggers a full group re-list (line 95-100), finds the member, and recovers — so it's not a crash, but it's an unnecessary API call. The Zod schema does not enforce uniqueness.

4. Current members without preferredMemberKey.id are invisible to reconciliation — cloudidentity-groups-memberships.enrich.ts:64-73

   Members returned by the API without a preferredMemberKey.id are silently excluded from both currentById (line 65-66) and toDelete (line 71). These members cannot be managed by this method — they won't be deleted even when absent from desiredMembers. This is a defensible choice but means "authoritative" reconciliation is only authoritative over members that have a preferredMemberKey.id. If such members exist, the result's total count will include them, potentially confusing the caller.

5. Final list after reconciliation doesn't check for truncation — cloudidentity-groups-memberships.enrich.ts:123-129

   The initial list correctly checks nextPageToken and throws if the group is too large (line 55). The final list after reconciliation does not check nextPageToken. If the group grew between the initial and final lists (e.g., concurrent additions), writeResource handles would be written for only a subset of members, and result.total would undercount. Unlikely given the initial check passed, but asymmetric validation.

6. Hardcoded maxPages: 100 in AlreadyExistsError fallback path — cloudidentity-groups-memberships.enrich.ts:99-100

   When a create fails with AlreadyExistsError, the fallback re-lists the group with maxPages hardcoded to 100 instead of using the caller's maxPages argument. Not a practical bug (if the initial list with the user's maxPages succeeded, 100 pages is almost certainly sufficient), but inconsistent.

Verdict

PASS — The enrichment follows existing patterns, handles the primary edge case (AlreadyExistsError race), and the Zod schema is well-defined. The partial-failure and concurrency findings are MEDIUM but consistent with how other enrichments in this codebase work — they don't block merge. The metadata file, index registration, and design doc updates are all correct.

## Adversarial Review ### Medium 1. **Partial failure during delete phase leaves group in unrecoverable intermediate state with no diagnostic context** — `cloudidentity-groups-memberships.enrich.ts:110-121` The create loop (lines 76-108) runs first and commits members to the group. Then the delete loop (lines 111-121) runs sequentially with no try/catch. If `deleteResource` throws on the Nth deletion (e.g., transient 503, permission denied on a specific member type, or quota exhaustion), the operation exits with an error, but: - All creates are already committed. - Deletes 1..N-1 are already committed. - Deletes N+1..end are skipped. - The caller receives only the thrown error — no information about what was partially applied (the `created`/`deleted` counts in the return value are never reached). **Concrete scenario:** Desired `[A, B, C]`, current `[B, D, E, F]`. Creates `[A, C]` succeed. Delete `D` succeeds. Delete `E` fails (transient). Group is now `[A, B, C, E, F]` — neither original nor desired. The caller gets an opaque error with no indication that `A` and `C` were added and `D` was removed. This is consistent with how other enrichments handle errors (fail-forward, no rollback), but is more consequential here because the method is described as "authoritative reconciliation." A retry with the same inputs will converge (creates are idempotent via AlreadyExistsError handling, remaining deletes will be retried), but only if the caller knows to retry. **Suggested improvement:** Wrap the delete loop in a try/catch that captures partial progress and includes it in the thrown error, e.g.: ```typescript } catch (deleteErr) { const partialError = new Error( `Partial reconciliation: created ${created.length}, deleted ${deleted.length}/${toDelete.length}. ` + `Remaining members were not removed. Original error: ${deleteErr}` ); throw partialError; } ``` 2. **No concurrency control on the reconciliation window** — `cloudidentity-groups-memberships.enrich.ts:46-121` The existing IAM enrichments (storage-buckets, serviceaccounts) use etag-based optimistic concurrency on their read-modify-write operations. This enrichment performs a list (line 46), then creates and deletes against the live group with no mechanism to detect concurrent modifications. **Concrete scenario:** Between the initial `listResources` (line 46) and the delete loop (line 111), another process adds member `X` to the group. `X` is not in `desiredIds` (it wasn't in the initial list, so it wasn't considered). The delete loop picks up `X`... actually no — the delete loop operates on `toDelete` which was computed from the initial list, so `X` wouldn't be in `toDelete`. But `X` would appear in the final reconciled list and be written as state, even though it's not in `desiredMembers`. The result claims `total: N` including `X`, making the reported reconciliation inaccurate. Conversely, if another process removes a member between the list and the delete loop, `deleteResource` will fail on a 404 for that member, causing the operation to throw mid-delete (see finding #1). The Cloud Identity Memberships API doesn't support etags on individual memberships, so there may not be a clean fix. But this is a limitation worth documenting, at minimum. ### Low 3. **Duplicate IDs in `desiredMembers` cause redundant API round-trips** — `cloudidentity-groups-memberships.enrich.ts:62,69` `desiredIds` is built as a `Set` (deduplicating), but `toCreate` is filtered from the original `desiredMembers` array. If `desiredMembers` contains `[{id: "a@x.com"}, {id: "a@x.com"}]`, `toCreate` will include both entries. The second create hits `AlreadyExistsError`, triggers a full group re-list (line 95-100), finds the member, and recovers — so it's not a crash, but it's an unnecessary API call. The Zod schema does not enforce uniqueness. 4. **Current members without `preferredMemberKey.id` are invisible to reconciliation** — `cloudidentity-groups-memberships.enrich.ts:64-73` Members returned by the API without a `preferredMemberKey.id` are silently excluded from both `currentById` (line 65-66) and `toDelete` (line 71). These members cannot be managed by this method — they won't be deleted even when absent from `desiredMembers`. This is a defensible choice but means "authoritative" reconciliation is only authoritative over members that have a `preferredMemberKey.id`. If such members exist, the result's `total` count will include them, potentially confusing the caller. 5. **Final list after reconciliation doesn't check for truncation** — `cloudidentity-groups-memberships.enrich.ts:123-129` The initial list correctly checks `nextPageToken` and throws if the group is too large (line 55). The final list after reconciliation does not check `nextPageToken`. If the group grew between the initial and final lists (e.g., concurrent additions), `writeResource` handles would be written for only a subset of members, and `result.total` would undercount. Unlikely given the initial check passed, but asymmetric validation. 6. **Hardcoded `maxPages: 100` in AlreadyExistsError fallback path** — `cloudidentity-groups-memberships.enrich.ts:99-100` When a create fails with AlreadyExistsError, the fallback re-lists the group with `maxPages` hardcoded to `100` instead of using the caller's `maxPages` argument. Not a practical bug (if the initial list with the user's maxPages succeeded, 100 pages is almost certainly sufficient), but inconsistent. ### Verdict **PASS** — The enrichment follows existing patterns, handles the primary edge case (AlreadyExistsError race), and the Zod schema is well-defined. The partial-failure and concurrency findings are MEDIUM but consistent with how other enrichments in this codebase work — they don't block merge. The metadata file, index registration, and design doc updates are all correct.

stack72 merged commit 900dd61a5a into main 2026-07-24 00:05:36 +00:00

stack72 deleted branch feat/cloudidentity-set-members-1313 2026-07-24 00:05:37 +00:00

stack72 referenced this pull request from a commit 2026-07-24 00:05:37 +00:00

feat(cloudidentity): add set_members enrichment for authoritative group membership (#1313) (#136)

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

swamp-club/swamp-extensions!136

No description provided.