Skip to content

[CXH-2211] feat: add clm_workflow_queue resource type - #67

Open
FeliLucero1 wants to merge 26 commits into
mainfrom
clm-workflow-queue-support
Open

[CXH-2211] feat: add clm_workflow_queue resource type#67
FeliLucero1 wants to merge 26 commits into
mainfrom
clm-workflow-queue-support

Conversation

@FeliLucero1

Copy link
Copy Markdown
Contributor

Description

  • Bug fix
  • New feature

Useful links:

Implements 4a from the CXH-1975 QE validation notes: syncs DocuSign
CLM's WorkflowQueue object — the API's own term for what the CLM admin
console reportedly calls "Task Groups", the surface the customer (The
Trade Desk) explicitly asked for. That naming equivalence is an
unconfirmed assumption, not a documented fact; the resource type is
named after the API term (clm_workflow_queue), not the UI term, until
someone with access to a real CLM admin console can confirm it.

The API has no list-all endpoint for workflow queues and no reverse
lookup from a queue to its members — only a per-member read (GET
.../members/{id}/workflowqueues). So List() discovers the distinct set
by scanning every clm_member once and deduping, and — as a side effect
of that same scan — builds a queueID -> []memberID index in the SDK's
session cache (newly enabled via connectorrunner.WithSessionStoreEnabled
in main.go, previously unused in this connector) so Grants() can read a
queue's membership back out directly instead of re-scanning every
member per queue, which would turn one O(members) traversal into
O(queues * members) — a real cost given the open rate-limit issue on
this connector for the same customer (CXP-704).

Read-only: the API documents work-item assign/unassign, not
queue-membership grant/revoke, so there's no Grant/Revoke here, matching
clm_permission_set's precedent for a CLM object with no write endpoint.

Like every other CLM endpoint in this connector, the response shapes
are documented-but-unexercised — no live CLM tenant was available to
confirm them.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Comment thread pkg/connector/clm_workflow_queues.go Outdated
Comment thread pkg/connector/clm_workflow_queues.go Outdated
var allAnnos annotations.Annotations

memberPageToken := ""
for {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: this loop pages through every member and issues one extra request per member inside a single List() call, so the SDK gets no checkpoint, no rate-limit backoff opportunity between pages, and the full queue+membership set is buffered in memory (the repo's own F2 guidance in CHANGE_TYPES.md/mixin). Given the open rate-limit issue this PR's README cites (CXP-704), consider draining one member page per List() invocation using the pagination bag (like clm_members.go), accumulating discovered queues in the session store, and returning the resources only on the final page. (confidence: medium — the no-list-all-endpoint constraint is real, this is about how the scan is chunked)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was already addressed in afac0cc (chunk clm_workflow_queue's member scan across List() calls, one ListMembers page per call with a real NextPageToken checkpoint, incremental session-store persistence per chunk) — matches clm_members.go's shape now. Sorry for the thread going stale, marking resolved.

Comment thread pkg/connector/clm_workflow_queues.go Outdated
Comment thread pkg/connector/clm_workflow_queues.go Outdated
Comment thread pkg/connector/clm_workflow_queues.go Outdated
Comment thread pkg/client/clm_client_test.go
@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Connector PR Review: [CXH-2211] feat: add clm_workflow_queue resource type

Blocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base e5261f1dcad7.
Review mode: incremental since 95c05ac
View review run

Review Summary

The new commit only touches the empty-member-Href skip in clm_workflow_queues.go and its test scaffolding, and it addresses all three prior findings: the skip now has its own SkippedMembersNoID counter so its sampling is independent of the two tolerated-failure causes, the log field moved from member_email to member_username (no PII), and clmtest.Server.AddMemberWithoutHref plus TestClmWorkflowQueueBuilder_List_SkipsMemberWithEmptyHref now cover the guard, including asserting GetMemberWorkflowQueues is never called for the malformed member, which is what keeps the escalation counter untouched. The full PR diff was re-scanned for security and correctness (no go.mod/go.sum changes in this PR; the incremental artifact reported no dropped paths and no truncation), and no new issues were found. One half of the earlier test-coverage finding is still open rather than regressed, the sibling empty-queue-Href guard still has no seeding hook or test, and the human review notes on log levels and baton_capabilities.json remain open in their existing threads.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

None.

Reviewed at head 78b175b806086437e1ef1014b3cc021331906c87 against base e5261f1dcad78a21c5b49cf759d2606cb2b266d8 (workflow ConductorOne/github-workflows/.github/workflows/pr-review.yaml@refs/heads/main). The machine-readable review-state marker could not be written this run because the review sandbox blocked every command able to emit it, so the next review will fall back to full mode.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking issues found — see review comments.

- Extract List()'s member-scan discovery into
  discoverClmWorkflowQueueMembership, a pure API-domain helper (no
  v2.Resource, no session cache) that List() just calls and converts.
  Matches every sibling CLM builder's simpler "discover, then build
  resources" shape instead of mixing five concerns in one function.
- Merge queuesByID/membersByQueueID into one clmWorkflowQueueMembershipEntry
  map — the two always shared the same key set, so two maps and two
  trailing loops collapse into one of each with no behavior change.
- Factor GetMemberGroups' and GetMemberWorkflowQueues' near-identical
  page-to-completion loops into a shared clmPageToCompletion[T] generic
  helper, so the bound/non-advancing-token guard only needs fixing once
  if it's ever wrong.
- Add the missing baton-docusign: prefix on the per-member fetch error
  in the new discovery helper.

Not changed, on purpose: Grants()'s Warn-level log on a session-cache
miss (one precedent already exists in this package — singing_groups.go
— and the code's own comment explains why Warn fits this specific
"shouldn't happen" case; judged not worth changing). Also not changed:
Grants() still returns a queue's full membership unpaginated — flagged
in review as a real but pre-existing pattern (clm_folders.go's Grants()
has the identical, and worse, shape already on main) rather than
something unique to this PR; left for a separate, broader fix.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@FeliLucero1 FeliLucero1 changed the title feat: add clm_workflow_queue resource type (Pylon #11836) [CXH-2209] feat: add clm_workflow_queue resource type Aug 10, 2026
@linear-code

linear-code Bot commented Aug 10, 2026

Copy link
Copy Markdown

CXH-2209

CXH-2211

Comment thread pkg/client/clm_client.go
Comment thread pkg/client/clm_client.go
Comment thread pkg/connector/clm_workflow_queues.go Outdated

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking issues found — see review comments.

- Tolerate isOptInFeatureUnavailableError per-member in the workflow-queue
  member scan (a deleted member or a scope gap on one member's endpoint no
  longer aborts the whole clm_workflow_queue sync).
- Skip clm_workflow_queue gracefully (not a hard sync-wide error) when the
  session cache write fails, matching every other CLM builder's degrade
  pattern — WithSessionStoreEnabled only opts in to a store existing; the
  parent process can still fall back to NoOpSessionStore at runtime.
- Fail Grants() loudly on a cache miss instead of emitting zero grants,
  which is indistinguishable from a queue's membership being genuinely
  emptied out.
- Guard against an empty Href collapsing every queue with no Href into one
  bogus merged resource.
- Dedupe repeated RateLimitDescription annotations down to the latest one.
- Fix the package doc's endpoint inventory and a doc comment that landed on
  the wrong declaration after the clmPageToCompletion refactor.
- Add pagination coverage for GetMemberWorkflowQueues.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Comment thread pkg/connector/clm_workflow_queues.go Outdated
Comment thread pkg/connector/clm_workflow_queues.go Outdated
Comment thread pkg/connector/clm_workflow_queues.go Outdated
Comment thread pkg/connector/clm_workflow_queues.go Outdated

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found.

- Narrow the per-member workflow-queue-fetch tolerance to codes.NotFound
  only (an isolated deleted-member case); Unauthenticated/PermissionDenied/
  FailedPrecondition now bail the whole resource type via
  errClmWorkflowQueuesUnavailable instead of being silently swallowed
  per-member, which could otherwise produce a partial membership set that
  C1 reads as revoked access.
- Apply logarithmic sampling (1, 10, 100, then every 1000) to the two
  per-member/per-queue Warn logs so a large account doesn't flood the log.
- Preserve accumulated RateLimitDescription annotations on List()'s
  graceful-skip paths instead of discarding them.
- Track the last RateLimitDescription by index instead of value in
  dedupeRateLimitAnnotations, dropping the only direct
  google.golang.org/protobuf import in the repo (go.mod still marked it
  // indirect).
Comment thread pkg/connector/clm_workflow_queues.go Outdated
Comment thread pkg/connector/clm_workflow_queues.go Outdated
Comment thread pkg/connector/clm_workflow_queues.go Outdated

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found.

Bug: discoverClmWorkflowQueueMembership escalated ANY per-member
isOptInFeatureUnavailableError to "CLM unavailable, return zero
resources" regardless of scan position. A PermissionDenied/Unauthenticated
on member N (token expiring or scope revoked mid-scan) after earlier
members had already contributed real queues would silently discard every
already-discovered queue as if the whole feature were unavailable — the
same false-deletion risk ListMembers' own memberPageToken == "" narrowing
exists to avoid. Now only escalates to errClmWorkflowQueuesUnavailable
while membership is still empty; once queues have been found, CLM is
clearly available, so a later failure fails loud instead.

Adds Server.ForceMemberWorkflowQueuesStatus to clmtest to test all three
per-member outcomes: NotFound skip-and-continue, PermissionDenied on the
first member (graceful skip), PermissionDenied after some discovery (hard
failure).
Comment thread pkg/connector/clm_workflow_queues.go Outdated
Comment thread pkg/connector/clm_workflow_queues.go Outdated

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found.

Two gaps in the previous fix, both confirmed via review:
- It only recognized a replay of the single most recently applied
  chunk. A resume rolling back more than one chunk (re-issuing an
  earlier token) went undetected and re-ran that chunk in full,
  double-counting its failures the same way the original bug did.
- replayChunk spent a full ListMembers call just to recompute the
  next-page token it already knew from the original chunk.

Store NextExpectedInputToken (the frontier) instead of the last
applied input: any incoming token that doesn't match it is a replay,
regardless of how many chunks it's behind, and resuming from the
stored frontier needs zero API calls. Added a regression test for the
multi-chunk-rollback case, verified via mutation testing.
Comment thread pkg/connector/clm_workflow_queues.go Outdated
Comment thread pkg/connector/clm_workflow_queues.go Outdated

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found.

NextExpectedInputToken alone can't distinguish a first call from a
replay when the whole scan fits in one page — both leave the token
at "". Add ScanComplete to disambiguate, and route that case through
replayChunk instead of re-running the per-member scan.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found.

// (if any) is silently missing from this sync otherwise.
state.SkippedMembers++
if n := state.SkippedMembers; n == 1 || n == 10 || n == 100 || n%1000 == 0 {
ctxzap.Extract(ctx).Warn("baton-docusign: failed to get CLM workflow queues for member, skipping",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C1 surfaces Warn logs prominently in production. Because this is a non-fatal skip inside the per-member scan, it can emit repeatedly during a normal sync and create noisy alerts without requiring operator action.

Please use Debug here, as with the session-store skips above. If this condition should stop the sync and require intervention, return an error instead of logging and continuing.

// this member and keep scanning.
state.SkippedMembers++
if n := state.SkippedMembers; n == 1 || n == 10 || n == 100 || n%1000 == 0 {
ctxzap.Extract(ctx).Warn("baton-docusign: CLM member not found while scanning workflow queues, skipping",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An isolated member disappearing between the member list and this lookup is an expected race, and the scan continues successfully. Logging it at Warn would surface an actionable-looking production alert for a condition that needs no operator action.

Please use Debug here.

if queueID == "" {
state.SkippedQueues++
if n := state.SkippedQueues; n == 1 || n == 10 || n == 100 || n%1000 == 0 {
ctxzap.Extract(ctx).Warn("baton-docusign: CLM workflow queue has an empty Href, skipping",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An empty Href is skipped and does not fail the sync. Since this can occur repeatedly while scanning queue entries, Warn would create prominent production noise without an action the operator can take.

Please use Debug here.

// console's 'Task Groups'?" naming question. Uses StaticEntitlementSyncerV2 for the
// same reason clm_group does: every queue shares the same single "member"
// entitlement.
clmWorkflowQueueResourceType = &v2.ResourceType{

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clm_workflow_queue is registered here, but baton_capabilities.json on this branch still has no entry for it (siblings like clm_role / clm_permission_set are present), so platform caps/UI lag until the post-merge bot runs.

Please regen and commit it in this PR (./connector capabilities with the usual env) — same habit as shipping the type itself.

return "clm_workflow_queue_members:" + queueID
}

// clmWorkflowQueueBuilder syncs CLM WorkflowQueues — the API's own term for what the CLM

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Docs / nit] ~40 lines of design log on the type (Task Groups hypothesis, session rewrite math, earlier single-call version, WithSessionStoreEnabled). Hard to scan before the struct.

Trim to the contract a reader needs, e.g.:

// clmWorkflowQueueBuilder syncs CLM WorkflowQueues (UI "Task Groups" — unconfirmed).
// No list-all / no queue→members: List() scans members one page per call, caches
// membership per queue in the session store; Grants() reads that cache (not O(N×M)).
// Read-only — API has work-item assign/unassign, not membership grant/revoke.

Move the longer rationale to the PR / ticket. Non-blocking.

if err != nil {
if isOptInFeatureUnavailableError(err) {
if !state.SucceededAtLeastOnce {
// Nothing has proven this endpoint works for this account yet.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Docs / nit] This escalation branch's inline comment restates DocuSign 404 semantics, the false-deletion race, and the threshold rationale at essay length.

Suggested trim:

// Before any success: tolerate opt-in codes, but require
// clmWorkflowQueueUnavailableThreshold consecutive failures before soft-skipping
// the whole type (one isolated NotFound must not wipe queues). CLM 404 means
// missing OR no access — same as other opt-in signals.

Non-blocking.

"google.golang.org/grpc/status"
)

// clmWorkflowQueueUnavailableThreshold is how many CONSECUTIVE tolerated per-member

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Docs / nit] Const docs don't need the "no live tenant / judgment call" narration — one line is enough:

// Consecutive opt-in failures (no success yet) before soft-skipping the type.

Non-blocking.

Comment thread pkg/client/clm_models.go
{Name: "SuperAdministrator"},
}

// ClmWorkflowQueue represents a CLM WorkflowQueue object (the API's own term for what

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Docs / nit] Task Groups caveat + "documented-but-unexercised" already live on the builder — don't repeat them on every model.

// ClmWorkflowQueue is a CLM WorkflowQueue (Member's workflow-queues response shape).

Non-blocking.

// why the scan is chunked this way instead of running to completion inside one call.
const clmSessionKeyWorkflowQueueDiscoveryState = "clm_workflow_queue_discovery_state"

// clmWorkflowQueueDiscoveryState is List()'s accumulator: the registry of distinct

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Docs / nit] Same pattern on the state struct — keep field docs for NextExpectedInputToken / ScanComplete (those are non-obvious), drop the restated "why chunking exists" paragraph that duplicates the builder doc.

Non-blocking.

@luisina-santos luisina-santos left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@btipling btipling removed their assignment Aug 13, 2026
…ue is unavailable

Per luisina-santos's review feedback: clm_workflow_queue is OptInRequired,
and C1's opt-in toggle doesn't validate the account can actually use it
before letting a customer enable it. Silently succeeding with zero resources
when it can't reach the endpoint hides that misconfiguration instead of
surfacing it - the same principle already applied to clm_role, clm_folder,
clm_group, clm_member, clm_permission_set, and signing_group on a separate,
not-yet-merged branch (clm-role-optin-error-observability-fixes).

Changes both places List() concluded "CLM unavailable" and returned success
with zero resources: the first-page ListMembers() error path, and the
per-member scan's escalation-threshold-reached path. Left the threshold
counting logic itself untouched (still requires 3 consecutive failures
before concluding "systemic unavailable" rather than an isolated
mid-scan member deletion) - only the terminal action changes, from
returning nil to returning the error. Also left the isolated
NotFound-after-success skip alone: that's an ordinary item-level race during
an otherwise-working scan, not an "is CLM available" signal, and forcing it
to fail the whole sync would make large/churning accounts fail
non-deterministically based on scan-order timing.

This CI account has no CLM subscription and these jobs run the connector
directly with no resource-type filter, so clm_workflow_queue's new fail-loud
behavior broke test-groups/test-signing-groups/test-permission-profiles the
same way it did on the other branch - added the same BATON_SYNC_RESOURCE_TYPES
fix, and a README note pointing self-hosted/CLI users at it.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Comment thread .github/workflows/ci.yaml Outdated
# the whole sync rather than skipping gracefully when CLM isn't available (see
# pkg/connector/clm_workflow_queues.go's List()) — exclude the CLM types here
# explicitly to test the ones this job cares about.
BATON_SYNC_RESOURCE_TYPES: user,group,permission_profile,signing_group

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: sync-resource-types is an allowlist, so this drops all six CLM types from CI, not just clm_workflow_queue. Against this no-CLM account the other five (clm_member, clm_role, clm_group, clm_permission_set, clm_folder) were previously exercising their isOptInFeatureUnavailableError graceful-skip paths end-to-end on a real DocuSign account — that regression protection is now gone silently, in all three jobs. Adding them back keeps the coverage while still excluding the one type that must hard-fail:

Suggested change
BATON_SYNC_RESOURCE_TYPES: user,group,permission_profile,signing_group
BATON_SYNC_RESOURCE_TYPES: user,group,permission_profile,signing_group,clm_member,clm_role,clm_group,clm_permission_set,clm_folder

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, real regression. Fixed in 6c147ce — listed the other 5 CLM types back into the allowlist so their graceful-skip path stays exercised end-to-end against this no-CLM account.

Comment thread README.md
Comment on lines 122 to +123
(or without a CLM subscription on the account), each CLM resource type's sync is skipped
gracefully rather than erroring the whole sync.
gracefully rather than erroring the whole sync — **except `clm_workflow_queue`**, which

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: this README carve-out is now correct, but the same "no CLM ⇒ no CLM resources, nothing breaks" claim still stands unqualified in the two customer-facing docs this PR also touches: docs/connector.mdx:32 ("accounts without CLM simply sync no CLM resources") and docs/doc-info.md:48 ("accounts without CLM sync no CLM resources"). After d0b92ed, a customer who toggles the clm_workflow_queue opt-in on a non-CLM account gets a hard sync failure that takes the eSignature resource types down with it — worth stating in connector.mdx (and doc-info.md) so support isn't reading a doc that says the opposite.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in 6c147ce — added the clm_workflow_queue exception to both docs/connector.mdx and docs/doc-info.md so support doesn't read a doc that contradicts the actual behavior.

Comment on lines +175 to +177
// clm_workflow_queue is OptInRequired, and C1's opt-in toggle doesn't check the
// account can actually use it first — see clm_roles.go's identical rationale. List()
// must fail loudly here rather than silently succeed with zero resources.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: the clm_roles.go pointer is a dead end — that file is a hardcoded 5-role list with no API call and no error-tolerance logic at all, so it documents no "identical rationale". The actual rationale lives in clm_workflow_queues.go's escalation branch (and the review thread that asked for the change).

Suggested change
// clm_workflow_queue is OptInRequired, and C1's opt-in toggle doesn't check the
// account can actually use it first — see clm_roles.go's identical rationale. List()
// must fail loudly here rather than silently succeed with zero resources.
// clm_workflow_queue is OptInRequired, and C1's opt-in toggle doesn't check the
// account can actually use it first — see List()'s escalation branch in
// clm_workflow_queues.go for the full rationale. List() must fail loudly here
// rather than silently succeed with zero resources.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in 6c147ce — repointed at the actual rationale in clm_workflow_queues.go's escalation branch instead of the dead-end clm_roles.go reference.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found.

…gaps

sync-resource-types is an allowlist, not a denylist — the previous fix
excluded all 6 CLM types from test-groups/test-signing-groups/
test-permission-profiles when only clm_workflow_queue needed excluding.
That silently dropped end-to-end CI coverage of clm_member/clm_role/
clm_group/clm_permission_set/clm_folder's still-unchanged
isOptInFeatureUnavailableError graceful-skip path against a real no-CLM
DocuSign account. Listed the other 5 back in explicitly.

Also: docs/connector.mdx and docs/doc-info.md both still claimed
unqualified "no CLM subscription -> no CLM resources, nothing breaks" -
no longer true for clm_workflow_queue after d0b92ed, where enabling it on
a non-CLM account now fails the whole sync. And a test comment pointed at
clm_roles.go as having "identical rationale" for this pattern, but that
file has no API call or error-tolerance logic at all - repointed at the
actual rationale in clm_workflow_queues.go's escalation branch.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
// member-carol is scanned after member-alice (who succeeds and contributes a queue),
// so this exercises the "isolated NotFound" branch specifically, not the "nothing has
// succeeded yet" escalation
// TestClmWorkflowQueueBuilder_List_SkipsGracefullyAfterConsecutiveFailures covers.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: This cross-reference (and the identical one at line 554) points at TestClmWorkflowQueueBuilder_List_SkipsGracefullyAfterConsecutiveFailures, which doesn't exist — the test was renamed to TestClmWorkflowQueueBuilder_List_FailsAfterConsecutiveFailures, and "skips gracefully" now describes the opposite of its actual fail-loud behavior. Same class of stale reference this commit fixed at line 176; worth updating both to the current name/wording. (confidence: high)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in 95c05ac — updated both cross-references to the current test name/wording.

chunkMembersByQueue := make(map[string][]string)

for _, member := range members {
memberID := clmIDFromHref(member.Href)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: memberID isn't guarded against an empty Href the way queueID is at line 243. An empty ID produces GET /v2/{acct}/members//workflowqueues, which 404s and is then tolerated — but on the pre-success path each such 404 increments ConsecutiveUnavailableFailures, so three malformed members early in scan order would hard-fail the sync and misreport it as "CLM workflow queues unavailable". Consider skipping empty member IDs (warn + continue) before the API call, symmetric with the queue-ID guard. (confidence: medium — depends on the CLM API ever omitting Href, which no live tenant confirmed either way)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in 95c05ac — added the same skip-and-continue guard queueID already has. Didn't add a dedicated test for it since there's no existing test for the analogous empty-queueID skip either (no easy way to seed a member/queue with an empty Href via clmtest's current helpers without adding new mock infra), but happy to add one if you'd rather have it covered explicitly.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found.

memberID wasn't checked for empty the way queueID already is a few lines
down. An empty ID would call GetMemberWorkflowQueues(ctx, "") ->
GET .../members//workflowqueues, which 404s — and on the pre-success path,
each such 404 now counts toward clmWorkflowQueueUnavailableThreshold since
d0b92ed's fail-loud change, so a handful of malformed members early in scan
order could hard-fail the entire sync and misreport it as "CLM workflow
queues unavailable" rather than "found members with no usable ID." Added
the same skip-and-continue guard the queue-ID case already has.

Also fixed two comment cross-references left pointing at
TestClmWorkflowQueueBuilder_List_SkipsGracefullyAfterConsecutiveFailures
(renamed to ...FailsAfterConsecutiveFailures in d0b92ed) with the old
"skips gracefully" wording, now describing the opposite of the test's
actual behavior.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Comment thread pkg/connector/clm_workflow_queues.go Outdated
Comment on lines +179 to +183
state.SkippedMembers++
if n := state.SkippedMembers; n == 1 || n == 10 || n == 100 || n%1000 == 0 {
ctxzap.Extract(ctx).Warn("baton-docusign: CLM member has an empty Href, skipping",
zap.String("member_email", member.Email), zap.Int("total_occurrences", n))
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: state.SkippedMembers is now shared by three distinct skip causes (empty Href here, pre-success tolerated failure at :224, isolated NotFound at :237), and the n == 1 || n == 10 || n == 100 || n%1000 == 0 sampling keys off that shared counter. So on an account where, say, 100+ tolerated failures happen before the first malformed member, the first ever empty-Href skip logs nothing — an entire failure class silently disappears from the sync log, which is the opposite of what the log-sampling pattern is meant to guarantee. Consider a separate counter per cause (e.g. SkippedMembersNoID) so each cause's first, 10th, and 100th occurrence is always visible.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in 78b175b — gave it its own SkippedMembersNoID counter so its sampling is independent of the other two skip causes.


for _, member := range members {
memberID := clmIDFromHref(member.Href)
if memberID == "" {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: this new skip branch has no test, and neither does its sibling empty-queue-Href guard at :258 — clmtest currently has no way to seed a member (or queue) with an empty Href, so nothing exercises either path. Given how thoroughly the rest of this builder's branches are pinned, a small clmtest helper (e.g. AddMemberWithoutHref) plus a test asserting the scan continues, discovers the other members' queues, and does not touch ConsecutiveUnavailableFailures would keep this guard from silently regressing.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in 78b175b — Server.AddMemberWithoutHref plus a test confirming the scan skips it, still discovers the other members' queues, and never calls GetMemberWorkflowQueues for it.

Comment thread pkg/connector/clm_workflow_queues.go Outdated
state.SkippedMembers++
if n := state.SkippedMembers; n == 1 || n == 10 || n == 100 || n%1000 == 0 {
ctxzap.Extract(ctx).Warn("baton-docusign: CLM member has an empty Href, skipping",
zap.String("member_email", member.Email), zap.Int("total_occurrences", n))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion (low confidence): member_email puts PII into connector logs, which get shipped and retained. There is precedent in this repo (pkg/connector/singing_groups.go:106), and with Href empty there is no stable ID left to log, so this may be a deliberate trade-off — but member.UserName or the raw (empty/malformed) Href value would identify the malformed record about as well without the email address.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair, switched to UserName in 78b175b.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found.

…s log, add test coverage

Three more findings on the guard added in 95c05ac:

- It reused the shared SkippedMembers counter, whose 1/10/100/1000 log
  sampling keys off that shared value — so on an account where enough
  tolerated failures or isolated NotFounds happened first, the first-ever
  malformed-member skip could log nothing at all. Gave it its own
  SkippedMembersNoID counter so each failure class's sampling is independent.
- The Warn log carried member.Email; switched to member.UserName, which
  identifies the malformed record just as well without logging PII.
- Neither this guard nor its sibling empty-queueID guard had test coverage,
  since clmtest had no way to seed a member with an empty Href. Added
  Server.AddMemberWithoutHref and a regression test confirming the scan
  skips the malformed member, still discovers the other members' queues,
  and never calls GetMemberWorkflowQueues for it.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants