[CXH-2209] fix: gate clm_role on CLM availability, narrow opt-in error tolerance, log skipped folder-security entries - #64
Conversation
FeliLucero1
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Connector PR Review: [CXH-2209] fix: gate clm_role on CLM availability, narrow opt-in error tolerance, log skipped folder-security entriesBlocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0 Review SummaryThe new commit ( Security IssuesNone found. Correctness IssuesNone found. SuggestionsNone. |
7877d6a to
11ef697
Compare
11ef697 to
9a33a95
Compare
c400c19 to
c003e08
Compare
- 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>
c003e08 to
01763a1
Compare
- 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>
01763a1 to
e5a2f17
Compare
- 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>
e5a2f17 to
44031de
Compare
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>
Validate() previously returned raw client errors from EnsureReady/ EnsureClmReady with no indication of which readiness stage failed or what to do about it (flagged twice by the CI reviewer, never fixed). Wraps both branches with baton-docusign: context, and the CLM branch names the actual remedy. Also trims three comments that narrated past design attempts via commit SHAs (includeClm field doc, Validate()'s doc, a test's doc) down to the current invariant, matching this repo's established comment-tone convention. Doc note: CLM setup docs (README.md, docs/doc-info.md) were still missing the impersonation OAuth scope alongside spring_read/ spring_write, and didn't mention that an already-connected install needs --configure to re-consent since refresh tokens don't resend scopes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…th-limit The CLM readiness remedy message added in d419057 was 246 characters on one line, over golangci-lint's 200-char limit (revive line-length-limit). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
includeClm's opts==nil guard was inert dead code — two lines below, opts.SelectedAuthMethod/opts.TokenSource already dereference opts unconditionally, so a nil opts would panic before the guard mattered. docs/doc-info.md's CLM setup step 3 said CLM syncs "automatically — there is no flag to set", directly contradicting the same file's line 48 (CLM types carry OptInRequired and require explicit opt-in in C1's sync configuration). Reworded to match. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
…adeoff Same finding (opts.WillSyncResourceType vs C1's real per-task filter) has been re-raised three times on this PR with identical file:line evidence. Documenting the decision inline, at the exact line the bot keeps flagging, so it reads as settled rather than an open item each time the diff gets re-reviewed. Also tightens README's self-hosted/service-mode claim: the platform filter does reach the syncer's List() dispatch, but not Validate() — that's the actual gap, not the sentence itself.
… test The existing regression test forced a 401 on /oauth/userinfo — a base credential failure Connector.Validate()'s EnsureReady already catches for every resource type, never reaching the removed branch. The removed tolerance was for a 401/403/404 from /signing_groups itself. Added a second status knob and a test that actually exercises it.
… code oauth.go's clmScopes has been spring_read/spring_write only since an earlier round of live testing confirmed impersonation is JWT-Grant-only and unneeded here; four doc mentions never caught up, including one instructing operators to re-consent for a scope the connector never requests.
…flag signing_group is only advertised by ListResourceTypes() when BATON_INCLUDE_SIGNING_GROUPS is set, and the SDK hard-errors on any sync-resource-types filter entry the connector doesn't advertise. Works today only because every job here also sets that flag.
Unused since the CLM readiness check moved to Connector.Validate() — clm_role never calls the API at all. The test previously spun up a full clmtest mock server solely to populate a field nothing reads.
The comment's own mechanism description already said includeClm is true on hosted and self-hosted runs alike, but the accepted-risk conclusion narrowed impact to self-hosted only — a non sequitur a reviewer correctly called out. The actual reasoning behind the accepted risk was never about which deployment mode is affected: it's a design position that a connector should fail its entire sync when it can't trust whether an enabled resource type is reachable, rather than guess.
| } | ||
| if cb.includeClm != includeClm { | ||
| t.Errorf("includeClm=%v: expected Connector.includeClm=%v, got %v", includeClm, includeClm, cb.includeClm) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: These two tests cover the constructors, but nothing covers the New()-level wiring that actually produces includeClm — the opts.WillSyncResourceType(clm*) disjunction at connector.go:236-238. That's the exact path the new CI allowlist (BATON_SYNC_RESOURCE_TYPES=user,group,permission_profile,signing_group) depends on to keep includeClm=false; if someone renames a clm_* resource type ID or drops a term from that disjunction, CI's sync jobs break with a confusing CLM-readiness error and no unit test catches it. A small table test calling New() with a cli.ConnectorOpts{SyncResourceTypeIDs: ...} and asserting cb.includeClm would pin it.
There was a problem hiding this comment.
Good catch — real gap. Fixed in 16a9ef1: added TestNew_IncludeClmDerivation, a table test calling New() with cli.ConnectorOpts{SyncResourceTypeIDs: ...} and asserting includeClm for each of the 5 clm_* resource type IDs individually, plus the no-filter case and CI's exact allowlist. Routes through opts.TokenSource so it needs no mock server.
The existing tests cover the constructors storing includeClm correctly, but nothing exercised New()'s own opts.WillSyncResourceType(clm*) disjunction that computes it — the exact logic CI's own BATON_SYNC_RESOURCE_TYPES allowlist depends on to keep includeClm=false. A renamed clm_* resource type ID or a dropped disjunction term would have gone uncaught.
| # ("invalid resource type 'signing_group' in filter"). If a future job drops that | ||
| # per-job env var, it fails with that confusing filter error instead of just skipping | ||
| # signing groups. | ||
| BATON_SYNC_RESOURCE_TYPES: user,group,permission_profile,signing_group |
There was a problem hiding this comment.
🟡 Suggestion: the comment above warns that a newly registered non-CLM resource type silently loses CI sync coverage if it isn't added here, but nothing enforces that. TestNew_IncludeClmDerivation's "CI's actual allowlist" case (connector_test.go) hardcodes the same four IDs independently, so the two lists can drift apart without any test failing. Consider deriving that test case from alwaysRegisteredTypeIDs minus the clm_* entries, so adding a resource type forces this list to be revisited.
There was a problem hiding this comment.
Good catch, real drift risk. Fixed in 8bd0005: that test case now derives from alwaysRegisteredTypeIDs minus clm_* entries (with signing_group added back separately, since it's intentionally excluded from that var — conditionally registered, not always). Registering a new non-CLM type in alwaysRegisteredTypeIDs now flows into this test automatically.
…aysRegisteredTypeIDs The "CI's actual allowlist" case hardcoded the same 4 IDs ci.yaml's BATON_SYNC_RESOURCE_TYPES declares independently, so the two could drift apart with no test failing. Deriving it from alwaysRegisteredTypeIDs (the connector's own source of truth) minus clm_* entries means registering a new non-CLM resource type there surfaces here too.
| // nonClmAllowlist derives the "no CLM types opted in" test case from | ||
| // alwaysRegisteredTypeIDs — the connector's own source of truth for always-registered | ||
| // resource types — instead of a second hardcoded literal, so registering a new non-CLM | ||
| // resource type there surfaces here too rather than the two lists silently drifting | ||
| // apart. signing_group is added separately since it's intentionally NOT in | ||
| // alwaysRegisteredTypeIDs (conditionally registered, see that var's doc) but is present | ||
| // in CI's real BATON_SYNC_RESOURCE_TYPES allowlist (ci.yaml). | ||
| func nonClmAllowlist() []string { |
There was a problem hiding this comment.
🟡 Suggestion: the doc comment claims that "registering a new non-CLM resource type there surfaces here too", but nothing in this test surfaces or fails in that case — a new non-CLM ID added to alwaysRegisteredTypeIDs simply flows into nonClmAllowlist() and TestNew_IncludeClmDerivation keeps passing. Deriving from alwaysRegisteredTypeIDs does remove the duplicated literal, but the drift this originally targeted — ci.yaml's BATON_SYNC_RESOURCE_TYPES falling behind the registered set and silently losing CI sync coverage — is still unenforced, since nothing reads ci.yaml. Consider narrowing the comment to what it actually guarantees (no duplicated literal), and if you want real enforcement, assert nonClmAllowlist() matches the BATON_SYNC_RESOURCE_TYPES value parsed out of .github/workflows/ci.yaml.
There was a problem hiding this comment.
Fair, the comment overclaimed. Went with real enforcement in 15264e5: added TestNonClmAllowlistMatchesCI, which parses .github/workflows/ci.yaml's actual BATON_SYNC_RESOURCE_TYPES value and asserts it matches nonClmAllowlist() — so the two now can't drift apart silently. Also narrowed nonClmAllowlist's doc comment to what it actually guarantees on its own (no duplicated literal) and pointed to the new test for the cross-file check.
The prior fix's doc comment claimed registering a new non-CLM resource type "surfaces here too", but nothing actually failed in that case — the real drift (ci.yaml's BATON_SYNC_RESOURCE_TYPES falling behind) stayed unenforced since nothing read ci.yaml. Narrowed that comment to what it actually guarantees, and added a test that parses the real workflow file and asserts its allowlist matches nonClmAllowlist().
| t.Fatalf("reading ci.yaml: %v", err) | ||
| } | ||
| var workflow struct { | ||
| Env map[string]string `yaml:"env"` |
There was a problem hiding this comment.
🟡 Suggestion: Decoding the workflow-level env block into map[string]string makes this test brittle to unrelated CI edits — yaml.v3 returns a *yaml.TypeError for any non-string scalar, so adding e.g. BATON_FOO: true or a bare numeric value at the workflow level would fail this test at t.Fatalf("parsing ci.yaml: ...") rather than at the allowlist assertion it exists for. Decoding into map[string]any (or yaml.Node) and type-asserting just BATON_SYNC_RESOURCE_TYPES to string keeps the failure scoped to the thing being guarded.
There was a problem hiding this comment.
Good catch, that's a real brittleness. Fixed in ad17858: decodes into map[string]any now and type-asserts just BATON_SYNC_RESOURCE_TYPES to string, so an unrelated non-string env var elsewhere in that block won't fail this test at the wrong place.
…lly guards Decoding the workflow-level env block into map[string]string made this test brittle to unrelated CI edits: yaml.v3 returns a TypeError for any non-string scalar, so adding e.g. BATON_FOO: true elsewhere in that block would fail the whole decode instead of the allowlist assertion. Decode into map[string]any and type-assert only BATON_SYNC_RESOURCE_TYPES.
| group: docusign-demo-account | ||
| cancel-in-progress: false | ||
| env: | ||
| # This CI account has no CLM subscription, and (unlike a C1-hosted sync) nothing here |
There was a problem hiding this comment.
let's cleanup this comment 🙏
| } | ||
|
|
||
| func (d *Connector) Validate(_ context.Context) (annotations.Annotations, error) { | ||
| // Validate runs once, before any resource type's List() (see baton-sdk's |
There was a problem hiding this comment.
we don't need this history comment and we know how Validate works.
Drop the includeClm/Validate() decision-record section from connector.go (the team already knows it) and shorten ci.yaml's concurrency-block comment to the essential why.
| One check does NOT see that platform filter in either deployment mode: `Connector.Validate()`'s | ||
| upfront CLM-readiness check (see its doc comment in `pkg/connector/connector.go`) runs once, | ||
| before any resource type's `List()` and before the platform filter is applied to anything — | ||
| a known, reviewed, and deliberately accepted gap, not an oversight. |
There was a problem hiding this comment.
🟡 Suggestion: This points readers to Connector.Validate()'s doc comment for the "known, reviewed, and deliberately accepted gap", but the latest commit (ce2c22e) removed exactly that # Known limitation, reviewed and accepted block from the doc comment. The remaining comment at pkg/connector/connector.go:141-149 says nothing about the platform filter never reaching ConnectorOpts, so the cross-reference now dead-ends and this paragraph is the only surviving record of the rationale. Either drop the "see its doc comment" pointer or restore a one-line note in the doc comment.
There was a problem hiding this comment.
Good catch, dead pointer from my own edit. Fixed in d9ad380 — dropped the cross-reference, the paragraph already stands on its own.
ce2c22e removed the "Known limitation" section this pointed at; the README paragraph already carries the full rationale on its own.
8 participants
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:
clm_role,clm_folder,clm_group,clm_member,clm_permission_set, andsigning_group(not a CLM type, but gated by the sameisOptInFeatureUnavailableErrortolerance 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 ofclm_role/clm_folder/.../signing_groupis registered unconditionally and gated only byOptInRequired(or, forsigning_group, the--include-signing-groupsflag) — once opted in, a real capability failure now fails the sync rather than returning success with zero resources.isOptInFeatureUnavailableError/client.IsClmDiscoveryError/clmDiscoveryErrormachinery bullet 2 describes (the discovery-vs-data-call distinction) is gone entirely — there's no tolerance branch left for it to narrow.OptInRequiredis 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'stest-groups/test-signing-groups/test-permission-profilesCI jobs broke for exactly this reason once the fail-loud change landed; fixed by pinningBATON_SYNC_RESOURCE_TYPESin.github/workflows/ci.yaml, and added a README/docs note for anyone else running the connector directly.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.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
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_rolesynced 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_roleresources 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_rolehas no entitlements/grants of its own, and its only real use — as a principal in aclm_foldersecurity grant — never applied either, sinceclm_folderis 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.isOptInFeatureUnavailableErrortolerated 401/403/404/412 from any CLM call, not just the account-discovery call it was reasoned aboutclm_role, then ultimately replaced entirely by the fail-loud change above.)clm_folder'sGrants()silently skipped folder-security entries with an unmappedAccessType(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: