fix(client): page through SearchEntitlements results by arreyder · Pull Request #152 · ConductorOne/cone

arreyder · 2026-09-03T02:50:06Z

Why

client.SearchEntitlements sends page_size: 100 and then makes exactly one call, never reading NextPageToken — the body carried a literal // TODO(morgabra) Pagination. Every entitlement past the first page is silently dropped.

This is invisible today only because the server ignores page_size and returns everything in one response. The proto has always documented the endpoint as paginated (page_size, page_token, next_page_token, "the server returns one page of results and the nextPageToken until all results are retrieved"). The moment the server honors that, every cone caller truncates at 100 with no error.

That server change is ductone/c1 (SearchEntitlements pagination, flag-gated and off by default). This PR must land and ship before that flag is turned on for any tenant.

Affects cone search, cone aws, and cone generate-alias, which all go through this function.

What

Loop until the page token is empty, converting each page before moving on.

Per-page conversion is load-bearing. The response's expanded array is indexed per response — ExpandedMap holds offsets into that page's expanded objects. Concatenating raw pages and expanding once would resolve those offsets against the wrong array and attach the wrong app/resource to each entitlement. The existing expansion body is now convertSearchEntitlementsPage, called once per page.
Stops on an empty token, never on a short page. The server applies granted_status filtering after cutting the page, so an intermediate page can be empty while later pages still hold results. Breaking on a short page would reintroduce the truncation in a subtler form.
A nil list is an empty page, not an error. This also fixes what looks like a pre-existing bug: protobuf JSON omits empty repeated fields, so a search matching nothing returns {} and today fails with search-entitlements: list is nil instead of reporting no results. Worth a second opinion — it's a behavior change beyond pagination.
Stuck-token guard at 8 repeats, mirroring c1's MaxRepeatedPageTokenLimit, so a server that never advances the token can't spin the client forever.

Testing

Four tests drive the real loop against an httptest server (no testify — this repo doesn't depend on it):

Test	Pins
`FollowsNextPageToken`	3 pages, all 4 entitlements returned in page order, server saw `["", "tok1", "tok2"]`
`DoesNotStopOnEmptyPage`	empty middle page doesn't end pagination
`EmptyResultIsNotAnError`	nil list returns an empty slice
`StopsOnStuckPageToken`	non-advancing token errors, bounded

Mutation-verified: forcing the loop to break after one iteration (the old behavior) fails three of the four.

go build ./..., the full pkg/client suite, go vet, and golangci-lint run ./pkg/client/... (0 issues) all pass.

Note

ConductorOne/cone-private carries its own copy of this function with the same bug; it's fixed in a companion PR (it's on an older SDK, so the diff differs).

🤖 Generated with Claude Code

SearchEntitlements sent page_size=100 and then made exactly one call, never reading NextPageToken, so every entitlement past the first page was silently dropped. The server has always documented this endpoint as paginated; it just never enforced it, so the bug was invisible. Loop until the token is empty, converting each page before moving on. The per-page conversion is load-bearing: the response's expanded array is indexed per response, so ExpandedMap offsets only resolve against the page they came from. Merging raw pages first would attach the wrong app or resource to each entitlement. Stop on an empty token rather than a short page. The server applies granted-status filtering after it cuts the page, so an intermediate page can be empty while later pages still hold results. A nil list is now an empty page rather than an error. The wire format omits an empty repeated field, so a search matching nothing arrives as a nil list and previously failed with "search-entitlements: list is nil". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

github-actions · 2026-09-03T02:52:45Z

+		if nextPageToken == pageToken {
+			repeatedToken++
+			if repeatedToken >= maxRepeatedSearchEntitlementsPageToken {
+				return nil, fmt.Errorf("search-entitlements: page token repeated %d times, pagination is not advancing", repeatedToken)
+			}
+		} else {
+			repeatedToken = 0
+		}
+		pageToken = nextPageToken


🟡 Suggestion: the guard only catches a token that repeats the immediately preceding one — repeatedToken resets to 0 on any change, so a server cycling tok1 → tok2 → tok1 → tok2 loops forever and rv grows unbounded. Since this is the only bound on the loop, consider also capping total pages (or total results) as a backstop, e.g. if pages++; pages > maxPages { return nil, fmt.Errorf(...) }. Confidence: medium — requires a misbehaving server, but it's the difference between a bounded error and a hung CLI.

github-actions · 2026-09-03T02:52:52Z

+	rv := make([]*EntitlementWithBindings, 0, len(expandableList))
 	for _, v := range expandableList {
 		rv = append(rv, &EntitlementWithBindings{
 			Entitlement: AppEntitlement(*v.Entitlement.AppEntitlement),


🟡 Suggestion (pre-existing, now on a hotter path): NewExpandableEntitlementWithBindings only checks v.Entitlement == nil, but AppEntitlementView.AppEntitlement is itself a *AppEntitlement that the wire format omits when empty. A response containing {"entitlement":{}} makes this deref panic. Worth adding v.Entitlement.AppEntitlement == nil to the nil check in NewExpandableEntitlementWithBindings (or skipping such entries) — paginating multiplies the number of pages that must all be well-formed. Confidence: high on the deref, low on how likely the server is to emit it.

github-actions · 2026-09-03T02:52:56Z

+func serveEntitlementPages(t *testing.T, pages []entitlementPage) (*httptest.Server, *[]string) {
+	t.Helper()
+	seen := make([]string, 0, len(pages))
+	call := 0


🟡 Suggestion: seen and call are written from the httptest handler goroutine and read from the test goroutine after SearchEntitlements returns, with no synchronization establishing that ordering. Requests are sequential so this is unlikely to misbehave, but it can trip go test -race. A sync.Mutex around both (and in a small accessor returning a copy of seen) would make it airtight. Same pattern in TestSearchEntitlementsStopsOnStuckPageToken with calls. Confidence: low-medium.

github-actions · 2026-09-03T02:57:07Z

General PR Review: fix(client): page through SearchEntitlements results

Blocking Issues: 0 | Suggestions: 3 | Threads Resolved: 0
Criteria: Criteria status: none loaded - .claude/skills/ci-review.md was not found at trusted base dd29af4a7260.
Review mode: full
View review run

Review Summary

Scanned the full PR diff for security and correctness: the change replaces a single-shot SearchEntitlements call with a NextPageToken loop and extracts the per-page expansion into convertSearchEntitlementsPage. Per-page conversion is the right call — ExpandedMap offsets are response-scoped, so merging raw pages first would mis-resolve them — and I verified all four callers (search_entitlements.go, aws.go, generate_alias.go, get_drop_task.go) handle an empty slice correctly, so dropping the list is nil error is a safe behavior change. No dependency, go.mod, or go.sum changes. No blocking issues; three non-blocking suggestions below.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

pkg/client/entitlement.go:145-153 — the stuck-token guard resets on any token change, so a server cycling tokens (tok1 then tok2 then tok1) loops forever with unbounded accumulation; consider a total-page backstop. Confidence: medium.
pkg/client/entitlement.go:209 — pre-existing nil deref: v.Entitlement.AppEntitlement is never nil-checked, so an entitlement whose appEntitlement field is omitted panics. Confidence: high on the deref, low on likelihood.
pkg/client/entitlement_pagination_test.go:21-24 — seen and call are mutated from the handler goroutine and read from the test goroutine without synchronization; may trip go test -race. Confidence: low-medium.

Prompt for AI agents

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

## Suggestions

In `pkg/client/entitlement.go`:
- Around line 145-153: The only termination guard in the pagination loop compares
  the new token against the immediately preceding one and resets `repeatedToken`
  to 0 whenever the token changes. A server that cycles tokens (tok1 -> tok2 ->
  tok1 -> tok2) never trips the guard, so the loop runs forever and `rv` grows
  without bound. Add a second backstop: track the total number of pages fetched in
  the loop and return an error once it exceeds a sane maximum (e.g. 10000 pages),
  so any non-terminating server produces a bounded error instead of a hung CLI.
- Around line 209: `AppEntitlement(*v.Entitlement.AppEntitlement)` dereferences a
  pointer that is only partially guarded.
  `NewExpandableEntitlementWithBindings` (line 61) checks `v.Entitlement == nil`
  but not `v.Entitlement.AppEntitlement == nil`, and
  `AppEntitlementView.AppEntitlement` is a pointer to AppEntitlement that
  protobuf JSON omits when empty. A response page containing an entitlement
  object whose appEntitlement field is absent panics with a nil pointer
  dereference. Extend the nil check in `NewExpandableEntitlementWithBindings` to
  also return nil when `v.Entitlement.AppEntitlement == nil`, so the existing
  "search-entitlements: entitlement is nil" error path handles it. This is
  pre-existing, but pagination increases the number of pages that must all be
  well-formed.

In `pkg/client/entitlement_pagination_test.go`:
- Around line 21-24 (`serveEntitlementPages`): the `seen` slice and the `call`
  counter are written inside the httptest handler goroutine and read from the test
  goroutine after `SearchEntitlements` returns, with no synchronization
  establishing happens-before between the two. Requests are sequential so the
  values are correct in practice, but this can be reported by `go test -race`.
  Guard both with a `sync.Mutex` and return an accessor func that takes the lock
  and returns a copy of `seen`, instead of returning a pointer to the slice.
  Apply the same fix to the `calls` counter in
  `TestSearchEntitlementsStopsOnStuckPageToken`.

github-actions

No blocking issues found.

github-actions Bot reviewed Sep 3, 2026

View reviewed changes

Navigation Menu

Uh oh!

fix(client): page through SearchEntitlements results - #152

fix(client): page through SearchEntitlements results#152
arreyder wants to merge 1 commit into
mainfrom
chrisrhodes/search-entitlements-pagination

arreyder commented Sep 3, 2026

Uh oh!

github-actions Bot Sep 3, 2026

Uh oh!

github-actions Bot Sep 3, 2026

Uh oh!

github-actions Bot Sep 3, 2026

Uh oh!

github-actions Bot commented Sep 3, 2026

Uh oh!

github-actions Bot left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

arreyder commented Sep 3, 2026

Why

What

Testing

Note

Uh oh!

github-actions Bot Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Sep 3, 2026

General PR Review: fix(client): page through SearchEntitlements results

Review Summary

Security Issues

Correctness Issues

Suggestions

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant