[CXH-2209] fix: gate clm_role on CLM availability, narrow opt-in error tolerance, log skipped folder-security entries by FeliLucero1 · Pull Request #64 · ConductorOne/baton-docusign

FeliLucero1 · 2026-08-07T17:02:29Z

Update (2026-08-13)

The plan in bullet 2 below was superseded twice since this was first written — the final shipped behavior is simpler than either version described there:

All 6 originally-tolerant resource types now fail loudly instead of gracefully skipping when their feature is unavailable: clm_role, clm_folder, clm_group, clm_member, clm_permission_set, and signing_group (not a CLM type, but gated by the same isOptInFeatureUnavailableError tolerance and the same review objection). Per reviewer feedback (luisina-santos): an opted-in resource that can't actually reach its API is a misconfiguration to surface, not a state to tolerate silently — "cuando no se tienen los permisos o la capacidad suficiente para sincronizar un recurso la sync TIENE que fallar." This applies even though each of clm_role/clm_folder/.../signing_group is registered unconditionally and gated only by OptInRequired (or, for signing_group, the --include-signing-groups flag) — once opted in, a real capability failure now fails the sync rather than returning success with zero resources.
The isOptInFeatureUnavailableError/client.IsClmDiscoveryError/clmDiscoveryError machinery bullet 2 describes (the discovery-vs-data-call distinction) is gone entirely — there's no tolerance branch left for it to narrow.
Found along the way: OptInRequired is enforced only by ConductorOne's platform (the sync-orchestration layer), never by baton-sdk or this connector itself. Any self-hosted/CLI run of the connector — including this repo's own CI — attempts every resource type unconditionally regardless of opt-in status. This repo's test-groups/test-signing-groups/test-permission-profiles CI jobs broke for exactly this reason once the fail-loud change landed; fixed by pinning BATON_SYNC_RESOURCE_TYPES in .github/workflows/ci.yaml, and added a README/docs note for anyone else running the connector directly.
Bullet 3's folder-security logging also went through several more review rounds: removed a dead conjunct in logSkippedFolderSecurityEntry's early return, and fixed a handful of comments left stale by the fail-loud change and by centralizing the per-branch logging into that one helper.
Added singing_groups_test.go (previously no test file existed for this builder at all) covering the new fail-loud behavior.

See the commit history for the full sequence — it includes one full revert (c12934e) of an intermediate attempt that reintroduced a regression already ruled out earlier in this same PR, caught by review before merge.

Description

Bug fix
New feature

Three independent CLM sync bugs, all reproducible without a CLM tenant (found via a hypothesis-investigation pass, 16/16 hypotheses confirmed across the three findings):

clm_role synced 5 fixed resources unconditionally, even on accounts without CLM. List() never made an API call, so it had no way to detect a missing subscription — unlike every other CLM resource type. Now gated on the same account-discovery check (client.EnsureClmReady) the other CLM builders already run internally.

Behavior change: on an account without CLM that has already synced once under the old (buggy) behavior, the next sync after this ships will show these 5 clm_role resources going from present to absent, which C1 reads as deletions. This is the correct end state (those 5 roles never should have existed for a non-CLM account), and low-blast-radius: clm_role has no entitlements/grants of its own, and its only real use — as a principal in a clm_folder security grant — never applied either, since clm_folder is gated by the same check and never synced any folders/grants on that same non-CLM account. No customer is using CLM today, so this has no real-world impact right now, but flagging it here per review feedback in case that changes before this ships.
~~isOptInFeatureUnavailableError tolerated 401/403/404/412 from any CLM call, not just the account-discovery call it was reasoned about~~ — superseded, see the Update above. (Original text preserved for history: tried tightening this to require the error come from discovery specifically; reverted after review feedback showed a genuine "no CLM subscription" signal can legitimately come from the data call too depending on where DocuSign enforces the check, and this repo has no live CLM tenant to confirm which. Landed instead as a narrower gate specific to clm_role, then ultimately replaced entirely by the fail-loud change above.)
clm_folder's Grants() silently skipped folder-security entries with an unmapped AccessType (Custom, InheritFromParentFolder, an unrecognized role) with no log output at any level. Added debug-level logging at each skip point.

Stacked on #63.

Useful links:

linear-code · 2026-08-07T17:03:35Z

CXH-2209

github-actions · 2026-08-07T17:06:42Z

Connector PR Review: [CXH-2209] fix: gate clm_role on CLM availability, narrow opt-in error tolerance, log skipped folder-security entries

Blocking Issues: 2 | Suggestions: 5 | Threads Resolved: 0
Criteria: Criteria status: none loaded - base ref clm-folder-provisioning-external-id is not the default branch main for .claude/skills/ci-review.md.
Review mode: full
View review run

Review Summary

Full PR diff scanned for security and correctness. This revision deletes the dead NewWithClient constructor and its test, and otherwise keeps the shape of the previous revision: isOptInFeatureUnavailableError removed, CLM readiness centralized in Connector.Validate() behind includeClm, folder-security skip logging added, plus the new BATON_SYNC_RESOURCE_TYPES CI env var and docs updates. One new blocking issue: the comma-separated BATON_SYNC_RESOURCE_TYPES value does not parse into a list through viper/cast, so the sync filter it is meant to apply will make the sync error out instead. The five findings from the previous review were re-verified against the current tree and are all still open, so they are carried forward below (no new inline comments posted for those). Separately, verify / lint is failing on this head, but on unchanged base-branch code (pkg/connector/clm_folders_test.go:796,821 staticcheck SA5011), so it is not reported as a finding here.

Security Issues

None found.

Correctness Issues

.github/workflows/ci.yaml:29 — (new, confirmed) BATON_SYNC_RESOURCE_TYPES: user,group,permission_profile,signing_group reaches the SDK via v.GetStringSlice("sync-resource-types"); for env-sourced values viper returns the raw string (viper.go:1211; the readAsCSV branch at :1179 only fires when the pflag itself changed), and cast.ToStringSliceE splits a string with strings.Fields (whitespace), not commas. The filter becomes the single element "user,group,permission_profile,signing_group", which validateSyncResourceTypesFilter (pkg/sync/syncer.go:1284) rejects with invalid resource type '...' in filter — all three CI sync-test jobs fail rather than skipping the 5 clm_* types. It also flips skipPermissionProfileResourceType to true (pkg/connector/connector.go:226), since SyncFilterIsExplicit() is now true with nothing matching.
pkg/connector/connector.go:151 — (carried forward, still open, confirmed) Validate()'s CLM check is gated on includeClm, which New() derives from opts.WillSyncResourceType(...) at construction time. That is not the signal that decides whether a CLM builder's List() runs: cli.ConnectorOpts.SyncResourceTypeIDs is populated only from the local sync-resource-types key (vendor/.../pkg/cli/commands.go:464/:670/:794, lambda_server__added.go:503), while the platform's per-sync resource-type filter is delivered on the sync task and handed to the syncer (vendor/.../pkg/tasks/c1api/full_sync.go:245 → sdkSync.WithSyncResourceTypes). Re-verified this run: with no local filter, SyncFilterIsExplicit() is false, so WillSyncResourceType returns true for every type and includeClm is always true — Validate() then runs CLM discovery and aborts the entire sync (users, groups, permission profiles included) at vendor/.../pkg/sync/syncer.go:879 for an eSignature-only tenant that never opted into CLM. That is exactly the failure mode OptInRequired plus the now-deleted isOptInFeatureUnavailableError tolerance existed to prevent.

Suggestions

README.md:134 — (new) documents BATON_SYNC_RESOURCE_TYPES as comma-separated; same viper/cast parsing caveat as the CI env var above, so an operator following it gets a failing sync instead of a CLM-free one. Commas are correct only for the --sync-resource-types flag form.
pkg/connector/clm_roles.go:18 — (carried forward, still open) clmRoleBuilder.client is dead now that the readiness check moved to Validate(): b.client is referenced nowhere in the file, and clm_roles_test.go now spins up a clmtest server the builder never calls.
pkg/connector/connector.go:102 — (carried forward, still open) the comment inside Metadata() still says "Signing groups and CLM are always registered as resource types (see ResourceSyncers)" — the exact claim 87adb41 corrected in resource_types.go:32. ResourceSyncers appends signing_group only when includeSigningGroups is set (connector.go:90).
pkg/connector/connector.go:212 — (carried forward, still open) the opts == nil || guard on includeClm is dead code: New() dereferences opts unconditionally at :223 (opts.SelectedAuthMethod) and :228 (opts.TokenSource), so a nil opts panics a few lines later regardless. The new comment above it documents nil-safety the function does not have.
pkg/connector/singing_groups.go:40 — (carried forward, still open) signing groups got the fail-loud change but no matching upfront check in Validate(), so CLM fails at validation while signing groups fail mid-sync, even though docs/connector.mdx:28-31 now presents the two behaviors identically.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Correctness Issues

In `.github/workflows/ci.yaml`:
- Around line 29: BATON_SYNC_RESOURCE_TYPES: user,group,permission_profile,signing_group is
  not parsed as a 4-element list. The SDK reads this key with viper's GetStringSlice; an
  env-sourced value comes back as a raw string and cast.ToStringSliceE splits it with
  strings.Fields (whitespace), not commas. The resulting one-element filter is rejected by
  validateSyncResourceTypesFilter with "invalid resource type ... in filter", failing all
  three sync-test jobs, and it also makes skipPermissionProfileResourceType true in
  pkg/connector/connector.go. Fix by making the env value whitespace-separated
  (user group permission_profile signing_group), or by passing
  --sync-resource-types=user,group,permission_profile,signing_group as a CLI flag to the
  connector in each job instead of via the env var.

In `pkg/connector/connector.go`:
- Around line 151 (and the includeClm derivation at line 212): Validate() gates its CLM
  readiness check on includeClm, derived from opts.WillSyncResourceType(...) at construction
  time. cli.ConnectorOpts.SyncResourceTypeIDs is only populated from the local
  sync-resource-types config key; the ConductorOne platform's per-sync opt-in filter is
  delivered on the sync task instead (pkg/tasks/c1api/full_sync.go -> WithSyncResourceTypes),
  so in a hosted/service run with no local filter WillSyncResourceType returns true for every
  type and includeClm is always true. Validate() then performs CLM account discovery and its
  error aborts the whole sync (all resource types) for any eSignature-only account. Fix by
  gating the CLM check on a signal that actually reflects opt-in, or by making the CLM
  readiness failure non-fatal for the rest of the sync (e.g. move it back into the CLM
  builders' first page and return a skip result / tolerated-error path there), so that a
  tenant that never opted into CLM cannot lose its user/group/permission_profile sync.

## Suggestions

In `README.md`:
- Around line 134: the text tells operators to pass BATON_SYNC_RESOURCE_TYPES comma-separated.
  Env values go through strings.Fields, so commas produce a single invalid filter entry and a
  failing sync. Document a space-separated env value, and keep commas only for the
  --sync-resource-types flag form.

In `pkg/connector/clm_roles.go`:
- Around line 18: the client field on clmRoleBuilder is now unused — CLM readiness moved to
  Connector.Validate() and List() makes no API call. Either drop the field (and the parameter
  from newClmRoleBuilder, updating callers and clm_roles_test.go, which now starts a clmtest
  server the builder never calls), or note explicitly why it is retained.

In `pkg/connector/connector.go`:
- Around line 102: the comment in Metadata() still claims "Signing groups and CLM are always
  registered as resource types (see ResourceSyncers)". ResourceSyncers only appends
  signing_group when includeSigningGroups is set (line 90), which resource_types.go:32 already
  documents correctly. Update the comment to match.
- Around line 212: the "opts == nil ||" short-circuit in the includeClm expression is dead
  code — New() dereferences opts unconditionally at line 223 (opts.SelectedAuthMethod) and
  line 228 (opts.TokenSource), so nil opts panics regardless. Either add a real nil guard at
  the top of New() (and keep the comment), or drop the "opts == nil ||" term and the
  nil-safety claim in the comment above it.

In `pkg/connector/singing_groups.go`:
- Around line 40: signing groups now fail loudly mid-sync while CLM fails upfront in
  Validate(), even though docs/connector.mdx describes the two as behaving the same way. For
  consistency, either add an equivalent upfront signing-groups readiness check in Validate()
  when includeSigningGroups is set, or adjust the docs to describe the two different failure
  points.

github-actions

No blocking issues found.

github-actions

No blocking issues found.

github-actions

No blocking issues found.

github-actions

No blocking issues found.

github-actions

No blocking issues found.

- TestClmSkipLogLevel now asserts the actually-emitted log level via zaptest/observer instead of comparing method-value pointers with reflect — Pointer() on a func is documented as not guaranteed to uniquely identify a function, so the old assertion tested identity that happened to work, not the observable behavior the test cares about. - Trimmed clmSkipLogLevel's doc comment: it was narrating this PR's own review history ("only two tries at that were made and both were wrong", referencing a since-removed IsClmDiscoveryError gate) instead of standing on its own — a future reader can't check a claim about code that no longer exists, and the paragraph would only rot further. Other bot comments on this PR (helper.go's "requiring IsClmDiscoveryError removes graceful degradation" x2, a double-blank-line formatting note) are stale: they describe the isClmUnavailableError gating mechanism this PR already reverted away from in a prior commit, and the blank line was already fixed there too. GitHub re-anchored their commit references to HEAD after the stack rebase, but their content predates the revert. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

github-actions

No blocking issues found.

- TestClmSkipLogLevel now asserts the actually-emitted log level via zaptest/observer instead of comparing method-value pointers with reflect — Pointer() on a func is documented as not guaranteed to uniquely identify a function, so the old assertion tested identity that happened to work, not the observable behavior the test cares about. - Trimmed clmSkipLogLevel's doc comment: it was narrating this PR's own review history ("only two tries at that were made and both were wrong", referencing a since-removed IsClmDiscoveryError gate) instead of standing on its own — a future reader can't check a claim about code that no longer exists, and the paragraph would only rot further. Other bot comments on this PR (helper.go's "requiring IsClmDiscoveryError removes graceful degradation" x2, a double-blank-line formatting note) are stale: they describe the isClmUnavailableError gating mechanism this PR already reverted away from in a prior commit, and the blank line was already fixed there too. GitHub re-anchored their commit references to HEAD after the stack rebase, but their content predates the revert. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

github-actions

No blocking issues found.

- TestClmSkipLogLevel now asserts the actually-emitted log level via zaptest/observer instead of comparing method-value pointers with reflect — Pointer() on a func is documented as not guaranteed to uniquely identify a function, so the old assertion tested identity that happened to work, not the observable behavior the test cares about. - Trimmed clmSkipLogLevel's doc comment: it was narrating this PR's own review history ("only two tries at that were made and both were wrong", referencing a since-removed IsClmDiscoveryError gate) instead of standing on its own — a future reader can't check a claim about code that no longer exists, and the paragraph would only rot further. Other bot comments on this PR (helper.go's "requiring IsClmDiscoveryError removes graceful degradation" x2, a double-blank-line formatting note) are stale: they describe the isClmUnavailableError gating mechanism this PR already reverted away from in a prior commit, and the blank line was already fixed there too. GitHub re-anchored their commit references to HEAD after the stack rebase, but their content predates the revert. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…ccess_type Seeding all three collections catches an omitted/inverted guard, but the three log entries were treated as interchangeable — a copy-paste slip that fires the right branch's guard under another branch's field name (e.g. the Users loop logging group_href instead of member_href) still produced 3 Debug entries with access_type: SomethingUnrecognized and passed. Now asserts each of group_href/role/member_href appears exactly once across the three entries.

- Give Custom its own distinct Debug log in Grants() (Groups/Roles/Users branches): it's a real, active grant this connector can't round-trip to a single tier, unlike NoAccess/InheritFromParentFolder, which stay fully silent since they're expected inert states, not a visibility gap. - Pin TestClmRoleBuilder_List_FailsLoudlyOnNonDiscoveryTolerableError's preconditions (isOptInFeatureUnavailableError + !IsClmDiscoveryError) so it stays a mutation-killer for the IsClmDiscoveryError conjunct even if the userinfo failure's code mapping changes later. - Note clm_role's residual gap: its gate only helps when DocuSign rejects CLM at discovery, not on a later per-resource data call.

Catches a branch swapping its distinguishing field/message with another (e.g. Users logging group_href) without exercising every combination — the previous check only verified each field appeared somewhere across the 3 entries. Also renames the shadowing ctx local to fields.

Groups/Roles/Users each had a near-verbatim 8-line switch differing only in the noun and one field name — collapsed into logSkippedFolderSecurityEntry. Also fixes the Custom branch missing the access_type field that every other skip line carries, and rescopes its test to the same access_type-filtered query as its sibling test instead of unscoped logs.All().

…mple-branch Grant test The unrecognized-role skip line was the one skip line in clm_folders.go without an access_type field, contradicting logSkippedFolderSecurityEntry's own doc claim about a single consistent structured-log query. TestClmFolderBuilder_Grant_SurvivesIdentityOnlyPrincipal_SampleBranch had the same byte-identical-fallback gap just fixed on the group builder's equivalent test: srv.GroupHref/MemberHref build the same shape the fallback derivation would from the same discovered base URL, so the test passed even with clmPreferredHref's sample-preference loop deleted (verified via mutation test). Re-seeds folder-contracts' samples with an alternate host to make the two branches actually distinguishable.

…r-security skip fmt.Sprintf and the fields append() ran unconditionally before zap's Debug-level check, on every skipped entry, every sync (confirmed via hypothesis investigation, 5/5). Switches to one constant message per case plus a principal_kind field, removing the Sprintf cost entirely. Updates the test that keyed on the old per-kind message text to bind principal_kind -> distinguishing field instead.

- unrecognized-role skip line still had no principal_kind field, unlike every other skip line that field exists to make queryable. - logSkippedFolderSecurityEntry's fields append() ran before deciding whether to log at all, so the common benign case (NoAccess/Inherit, on every folder of every sync) still allocated for a line never written. Early-return before the append, same spirit as the Sprintf removal.

PR #63 and this branch independently fixed the same tautological-test finding on TestClmFolderBuilder_Grant_SurvivesIdentityOnlyPrincipal_SampleBranch (a byte-identical sample-derived vs. fallback-derived Href) via two different mechanisms — Server.SetFolderGroupSecurityHref/ SetFolderUserSecurityHref (surgical Href override) vs. a full PatchFolderSecurity re-seed — which the rebase concatenated into one function body (duplicate sampleHost/wantHref declarations, and the re-seed silently overriding the override). Keep the surgical version: it preserves folder-contracts' original seed shape (both group entries) instead of replacing it.

…builders clm_roles.go already requires client.IsClmDiscoveryError(err) in addition to isOptInFeatureUnavailableError(err) before soft-skipping a sync, since the code alone can come from either CLM account discovery or a later per-resource data call. clm_folders/clm_groups/clm_members/clm_permission_sets each call ensureClmReady first (via SearchFolders/ListGroups/ListMembers/ ListPermissionSets), whose own data-call failures are never wrapped as clmDiscoveryError, so the same conjunct applies cleanly here too — closing the gap flagged in PR review (CXH-2209). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…M builders e85ccf9 re-applied exactly the narrowing eca81a7 deliberately reverted earlier in this PR, for the same reason that revert gave: this repo has no live CLM tenant to confirm whether DocuSign rejects a non-CLM account at discovery or at the per-resource data call. Unlike clm_role (which has no data call of its own, so IsClmDiscoveryError can only ever reject a non-discovery failure that already breaks every other resource type too), clm_folder/clm_group/clm_member/clm_permission_set each make a real data call after discovery succeeds. Requiring IsClmDiscoveryError there turns a same-coded rejection from that data call — e.g. a token that loses scope mid-sync — into a hard List() error, which baton-sdk's parallel syncer treats as fatal for the ENTIRE sync (all resource types, not just the one CLM builder), not a per-resource-type skip. That's a worse regression than the narrower gap being closed. Restores isOptInFeatureUnavailableError(err) alone for these four builders; clm_role's own two-conjunct gate is untouched. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…is unavailable Every resource type that used isOptInFeatureUnavailableError to tolerate "feature not available" errors (clm_role, clm_folder, clm_group, clm_member, clm_permission_set, signing_group) carries OptInRequired: C1 excludes them from a customer's sync by default, and List() only ever runs once a customer has explicitly opted in. C1's opt-in toggle has no upstream check against DocuSign, so a customer can enable one of these without actually having the subscription/scopes it needs. Per review feedback (luisina-santos): that's a real misconfiguration, not an expected/transient state, and every connector should fail its sync when it lacks sufficient permission or capability to sync an opted-in resource - silently succeeding with zero resources just hides the problem instead of surfacing it. This also fully closes the discovery-vs-data-call ambiguity debated earlier in this PR (mateoHernandez123's finding, the bot's regression finding) - there's no longer a tolerance branch left to gate. Removes the now-dead isOptInFeatureUnavailableError helper and its test. client.IsClmDiscoveryError is left in pkg/client (still tested there, still documents a real distinction in the client's error taxonomy) even though it has no remaining connector-layer caller - deleting it is a separate, client-layer cleanup, not part of this behavior change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…types test-groups/test-signing-groups/test-permission-profiles run baton-docusign directly via the sync-test action, with no C1 platform in the loop and no --sync-resource-types filter — so every registered resource type, including the 5 OptInRequired clm_* types, gets attempted regardless of whether anyone "opted in." Since 47f58c3 made CLM builders fail loud instead of skipping gracefully, and this repo's CI DocuSign account has never had a CLM subscription, every one of these jobs now fails the instant clm_folder's List() hits the CLM discovery 401 - confirmed by diffing this exact job's log against the immediately preceding (passing) commit. Pin BATON_SYNC_RESOURCE_TYPES to the 4 types each job actually needs (user, group, permission_profile, signing_group), matching how a real C1-hosted sync would behave for an account that never opted a CLM type in - that filtering only happens on C1's side, not in baton-sdk itself. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…docs gap Three cleanups surfaced by automated review on 47f58c3: - clm_client.go's clmDiscoveryError type and exported IsClmDiscoveryError had zero remaining callers once isOptInFeatureUnavailableError was deleted (the only consumer their own doc comments named never existed under that name). ensureClmInitialized now returns its errors unwrapped; removed the type, the predicate, and their now-dead unit test. - clmtest/server.go's ForceClmDiscoveryStatus/ForceUserInfoStatus test knobs were purpose-built for the two clm_role regression tests 47f58c3 deleted (the two-conjunct gate's discovery-vs-non-discovery halves) and had no other caller left. - docs/connector.mdx's signing-groups paragraph still described the old graceful-skip behavior; only the CLM paragraph got updated in 47f58c3. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…ging Three more findings from repeated automated review passes on the same area: - logSkippedFolderSecurityEntry's early return checked `accessType != client.ClmAccessTypeCustom && clmIsBenignUnmappedAccessType(accessType)`, but clmIsBenignUnmappedAccessType never returns true for Custom (its own doc says so), so the left conjunct was never the deciding factor. Also corrected the comment's inaccurate "allocation-free" claim (the caller builds the variadic fields before this function is ever entered) and dropped a dangling "like the Sprintf removal above" with no referent. - clmIsBenignUnmappedAccessType's doc pointed at "its own Debug log at each call site" for Custom, which was true before logSkippedFolderSecurityEntry centralized that logging into one shared function. - A test comment claimed clmIsBenignUnmappedAccessType was "re-checked by hand in each of the Groups/Roles/Users loops" — also stale post-dedupe; retargeted to the actual reason for seeding all three collections. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

47f58c3's fail-loud behavior change was documented purely in terms of C1's opt-in toggle, but that gate is platform-side only (confirmed earlier this PR, when the CI jobs needed BATON_SYNC_RESOURCE_TYPES added to keep passing). Running baton-docusign directly — the brew/docker/go install quickstarts, or any self-hosted/CLI invocation — attempts all 5 CLM resource types unconditionally, so an eSignature-only account run this way now fails its entire sync instead of skipping CLM. Added a paragraph pointing at --sync-resource-types/BATON_SYNC_RESOURCE_TYPES so the workaround isn't discoverable only by reading ci.yaml. Also fixed one more stale "at each Grants() call site" reference in clm_folders_test.go — same claim as clm_folders.go:454, missed there in 4d7ed4d's fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

signing_groups.go had zero test coverage before this — the fail-loud change in 47f58c3 (dropping isOptInFeatureUnavailableError's tolerance) was unverified. This package had no shared eSignature mock server (unlike pkg/client/clmtest for CLM), so added a small local one covering just what List() needs: /oauth/userinfo (the failure path GetSigningGroups hits first) and a bare /signing_groups response (the happy path, to distinguish "the mock is wired correctly" from "everything errors regardless"). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

- singing_groups_test.go: fix stale doc comment/failure message about the mock not serving /signing_groups (it does, with an empty body); seed one signing group instead so the happy-path test also exercises parseIntoSigningGroupResource rather than only asserting zero results. - Drop singing_groups_test.go's duplicate rewriteTransport declaration (users_test.go already declares it in the same package) — this was a pre-existing `go test ./pkg/connector/...` build failure, caught while fixing the above. - docs/doc-info.md: update the CLM opt-in bullet to match the OptInRequired/fail-loud contract README.md and docs/connector.mdx already describe, instead of the old no-opt-in-flag wording. - connector_test.go: update alwaysRegisteredTypeIDs' comment to stop crediting gating to isOptInFeatureUnavailableError, which this PR deletes from helper.go. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

BATON_SYNC_RESOURCE_TYPES pins an explicit allowlist (not a CLM-only exclusion) in all three sync-test jobs. A new non-CLM resource type registered in connector.go's ResourceSyncers() with no corresponding update here would get zero CI sync-test coverage, with nothing failing to signal the gap. Adds a maintainer reminder to the canonical comment block the other two jobs already point back to. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

test-groups/test-signing-groups/test-permission-profiles were only chained via needs within a single workflow run — two different branches' runs (e.g. this stack's two PRs, pushed close together) could still race concurrently against the same live demo account, one run's mid-cycle Grant/Revoke corrupting another's "should be zero grants after Revoke" assertion. Confirmed via CI history: test-groups flips pass/fail across commits that never touch pkg/connector/groups.go, correlated with near-simultaneous run start times across branches. Adds a shared concurrency group (not ref-scoped) across all three jobs so only one runs against the real account at a time, queuing the rest. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Rebasing this branch onto PR #63's latest tip silently reapplied an older PR #64 commit's import-block patch over helper.go's newly-added codes/status imports (no conflict was flagged since the patch context didn't include them), breaking the build. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

test-groups/test-signing-groups/test-permission-profiles each carried an identical BATON_SYNC_RESOURCE_TYPES line plus an explanatory comment, with a note instructing maintainers to keep all three copies in sync. Declaring it once at the workflow level removes that drift hazard — all three jobs inherit it the same way. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Per luisina-santos's review: each opted-in CLM builder independently checked CLM availability inside its own List(), redundant and racy under baton-sdk's concurrent resource-type syncing, when a single upfront check in Validate() (called once, before any List()) produces the same fail-loud outcome sooner. Centralizes that check behind a new includeClm field. Fixed two constructor bugs found while wiring includeClm through: NewWithTokenSource -- the ConductorOne-hosted path, i.e. the common production case -- had no includeClm parameter at all, so Validate() would have silently never run its CLM check there; NewWithRefreshToken received the parameter but never stored it. Also extended Validate() to check base eSignature credentials unconditionally (previously an unconditional no-op for any account without CLM), catching a misconfigured account upfront instead of leaving that to whichever builder's List() happens to run first mid-sync. clm_roles.go's own now-fully-redundant EnsureClmReady call is removed. NewWithClient (zero callers anywhere in this repo) is kept rather than deleted, since removing an exported function is a breaking change for any external consumer that may exist -- documented as such, with a test matching its two siblings. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

A self-hosted connector running in service mode still receives the platform's resource-type filter (confirmed in the c1 repo's rpc_baton.go SyncResourceTypeIds passthrough) -- only a bare one-shot CLI invocation, with no service/task involved at all, attempts every CLM resource type unconditionally. The prior wording lumped both cases together as "self-hosted/CLI." Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

A per-job concurrency block only protects a *running* job from cancellation -- GitHub Actions still cancels a *pending* job in the same group when a newer one queues. With test-groups/ test-signing-groups/test-permission-profiles needs-chained but each carrying its own copy of the same group, two overlapping workflow runs could still cancel each other's pending jobs mid-chain. Declaring it once at the workflow level instead makes the whole three-job run queue/cancel as one unit against the shared demo-account group. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

The comment claimed signing_group is "registered unconditionally ... OptInRequired is the gate, not a config flag, matching the CLM types below" -- but ResourceSyncers() only appends its builder when includeSigningGroups is set, contradicting the comment, TestResourceSyncers_SigningGroupRegistrationFollowsFlag's own doc, and singing_groups_test.go. Caught by the automated PR reviewer. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

The doc comment justified keeping NewWithClient on "removing an exported function is a breaking change" -- but this same branch already changed its signature (and NewWithTokenSource's) to add includeClm, which breaks a hypothetical external caller just as hard as deletion would. Reworded to state plainly that it carries no compatibility guarantee and is kept only in sync with its siblings' fields. Caught by the automated PR reviewer. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

The doc comment fixed in 6de1e1b for its self-contradictory rationale still claimed "no caller anywhere in this repo" while the very test added alongside it called NewWithClient directly -- and that test's own comment pointed back at a doc comment that, once corrected, no longer gave an affirmative reason to keep the constructor at all. Rather than keep patching the doc comment around a caller-less, never-stabilized constructor, delete both. Caught by the automated PR reviewer. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

github-actions · 2026-08-19T19:49:50Z

+  # resource type in pkg/connector/connector.go, add it here too — otherwise it silently
+  # gets zero CI sync-test coverage. Declared once at the workflow level (all three jobs
+  # inherit it) so there's no per-job copy to keep in sync.
+  BATON_SYNC_RESOURCE_TYPES: user,group,permission_profile,signing_group


🟠 Bug: a comma-separated value doesn't reach the SDK as a 4-element list. The SDK reads this via v.GetStringSlice("sync-resource-types") (vendor/.../pkg/cli/commands.go:464), and for an env-sourced value viper returns the raw string (viper.go:1211 — the readAsCSV branch at :1179 only applies when the pflag changed), so cast.ToStringSliceE hits its case string: return strings.Fields(v) (vendor/.../spf13/cast/caste.go:1311) and yields the single element "user,group,permission_profile,signing_group". Two consequences: the sync aborts with invalid resource type '...' in filter (vendor/.../pkg/sync/syncer.go:1284), and SyncFilterIsExplicit() becoming true with no matching ID also flips skipPermissionProfileResourceType to true in pkg/connector/connector.go:226. Whitespace-separating the value works with strings.Fields; passing --sync-resource-types=user,group,... as a flag also works via the CSV branch.

Suggested change

BATON_SYNC_RESOURCE_TYPES: user,group,permission_profile,signing_group

BATON_SYNC_RESOURCE_TYPES: user group permission_profile signing_group

Traced this further — the readAsCSV branch does apply here, just not through viper's Get() directly. DefineConfigurationV2 (pkg/config/config.go:199) calls cli.VisitFlags(mainCMD, v) right after registering pflags, before cmd.Execute() ever runs. For an env-sourced stringSlice field, VisitFlags's default branch (pkg/cli/cli.go:176-179) does cmd.Flags().Set(f.Name, v.GetString(f.Name)) — feeding the raw comma-separated env string into pflag's own stringSliceValue.Set(), which CSV-splits via encoding/csv (vendor/github.com/spf13/pflag/string_slice.go:22-29,42-54) and marks the flag Changed=true as a side effect. By the time application code calls v.GetStringSlice("sync-resource-types") (commands.go:464 etc.), viper's PFLAG-override branch fires (flag.HasChanged() is now true) and returns the pflag's already-correctly-split value — the raw env string never reaches cast.ToStringSliceE's whitespace-splitting path at all.\n\nConfirmed empirically too: the passing test-groups CI run for this SHA schedules list-resources for exactly the 4 intended types, with zero "invalid resource type" errors anywhere in the log.\n\nSo BATON_SYNC_RESOURCE_TYPES as a comma-separated env var does work as documented — VisitFlags exists specifically to bridge env-sourced values through pflag's CSV parsing before any GetStringSlice() call. No code change needed here.

github-actions · 2026-08-19T19:49:57Z

+quickstarts below, with no service/task involved at all) attempts all 5 CLM resource
+types by default, with no opt-in gate at all. If that account doesn't have a CLM
+subscription, the sync now fails instead of skipping CLM gracefully. Pass
+`--sync-resource-types` (or `BATON_SYNC_RESOURCE_TYPES`, comma-separated) with the


🟡 Suggestion: same parsing caveat as the CI workflow — BATON_SYNC_RESOURCE_TYPES is read through v.GetStringSlice, which for an env-sourced string splits on whitespace (cast.ToStringSliceE → strings.Fields), not commas. Documented as comma-separated, an eSignature-only operator following this instruction gets a single bogus filter entry and the sync fails with invalid resource type '...' in filter rather than excluding the CLM types. Commas are correct for the --sync-resource-types flag form (pflag CSV parsing); for the env var, document a space-separated value.

Same mechanism as the ci.yaml thread applies here. VisitFlags (pkg/cli/cli.go:152-180), invoked from DefineConfigurationV2 before cmd.Execute(), routes an env-sourced stringSlice value through pflag's own Set() — which CSV-splits on commas (vendor/github.com/spf13/pflag/string_slice.go:22-29,42-54) — before any GetStringSlice() call happens, and marks the pflag Changed so viper's PFLAG-override branch (not the raw-env-string path) is what actually gets consumed downstream. So the comma-separated env-var form documented here is correct as written. No doc change needed.

github-actions

Blocking issues found — see review comments.

FeliLucero1 changed the title ~~fix: gate clm_role on CLM availability, narrow opt-in error tolerance, log skipped folder-security entries~~ [CXH-2209] fix: gate clm_role on CLM availability, narrow opt-in error tolerance, log skipped folder-security entries Aug 7, 2026