Skip to content

CXP-383 Support audit log pagination for both GHEC and GHES - #192

Merged
JavierCarnelli-ConductorOne merged 1 commit into
mainfrom
fix/pagination-token
Sep 16, 2026
Merged

JavierCarnelli-ConductorOne merged 1 commit into
mainfrom
fix/pagination-token

Conversation

@JavierCarnelli-ConductorOne

Copy link
Copy Markdown
Contributor

The pagination token on GHEC and GHES is different for each. So this PR adds support to both of them

@linear-code

linear-code Bot commented Sep 16, 2026

Copy link
Copy Markdown

CXP-383

// numeric "page=N" Link headers parse into Response.NextPage (int) instead,
// leaving NextPageToken empty. Checking only NextPageToken silently truncates
// GHES audit logs to a single page.
func nextAuditLogPage(resp *github.Response) string {

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 new GHES numeric-page branch has no test coverage. TestUsageEventFeed_ListEvents_ContinuesPastAnAllFilteredPage only exercises the GHEC path (Link: <...?page=cursor2>; rel="next"), so the resp.NextPage fallback this PR adds would still pass if it regressed. Consider adding a sibling test whose mocked handler emits a numeric Link: <...?page=2>; rel="next" header and asserting the second request is issued with page=2. (confidence: high)

@github-actions

Copy link
Copy Markdown
Contributor

Connector PR Review: CXP-383 Support audit log pagination for both GHEC and GHES

Blocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 6db6df6744de.
Review mode: full
View review run

Review Summary

Scanned the full PR diff (one file, pkg/connector/usage_event_feed.go) for security and correctness. The change extracts the audit-log next-page decision into nextAuditLogPage, preserving the existing GHEC opaque-cursor behavior via Response.NextPageToken and adding a GHES fallback to the numeric Response.NextPage, which matches the repo's existing fmtGitHubPageToken convention in helpers.go. The helper is only reached under the resp != nil guard at pkg/connector/usage_event_feed.go:206, strconv was already imported, Organizations.GetAuditLog has no other call site that needs the same fix, and go.mod/go.sum are unchanged — no security or correctness issues found.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

  • pkg/connector/usage_event_feed.go:249 — the new GHES numeric-page branch is untested; existing pagination coverage only exercises the non-numeric GHEC cursor.
Prompt for AI agents
Verify each finding against the current code and only fix it if needed.

## Suggestions

In `pkg/connector/usage_event_feed.go`:
- Around line 249 (`nextAuditLogPage`): the GHES numeric-page fallback that reads
  `resp.NextPage` has no test. The existing test
  `TestUsageEventFeed_ListEvents_ContinuesPastAnAllFilteredPage` in
  `pkg/connector/usage_event_feed_test.go` only emits a non-numeric GHEC cursor
  Link header (`<https://api.github.com/orgs/octo-org/audit-log?page=cursor2>; rel="next"`),
  so it exercises only the `NextPageToken` branch. Add a sibling table or test —
  e.g. `TestUsageEventFeed_ListEvents_PaginatesNumericGHESPages` — that uses
  `mock.WithRequestMatchHandler(mock.GetOrgsAuditLogByOrg, ...)` and sets a numeric
  Link header (`<https://ghes.example.com/api/v3/orgs/octo-org/audit-log?page=2&per_page=100>; rel="next"`)
  on the first response and no Link header on the second. Assert that two requests
  were made, that the second request carries the `page=2` query parameter, and that
  events from both pages are returned. Optionally also add a direct unit test for
  `nextAuditLogPage` covering the three cases: NextPageToken set, NextPage set, and
  neither set (expect "").

@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.

@JavierCarnelli-ConductorOne
JavierCarnelli-ConductorOne merged commit 39aa762 into main Sep 16, 2026
9 checks passed
@JavierCarnelli-ConductorOne
JavierCarnelli-ConductorOne deleted the fix/pagination-token branch September 16, 2026 16:25
JavierCarnelli-ConductorOne added a commit that referenced this pull request Sep 17, 2026
…ngs (#193)

* fix: correct audit-log pagination cursor and sample skip-org warnings

- nextAuditLogPage now checks resp.After first (the cursor GHEC and
  GHES actually return for the org audit-log endpoint's rel="next"
  Link), falling back to NextPageToken/NextPage for any page-style
  Link header GHES may still emit. The prior fix (#192) only checked
  NextPageToken/NextPage, so it silently truncated every GHEC org -
  and most GHES orgs - to a single page.
- Request side now sends the cursor via After or Page depending on
  which shape was received (usageEventPageToken.AuditLogCursorIsPage),
  instead of always sending Page.
- Orgs that permanently lack audit-log access are now warned via a
  sampled logger (1st, 10th, 100th, then every 1000th occurrence,
  with total_occurrences) instead of on every poll pass forever. The
  counter is intentionally shared across all orgs on the feed, not
  keyed per org.

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

* fix: key skipped-org warning sampling per org, not shared

A single shared counter meant one permanently-inaccessible org could
drive the sampling budget high enough that another org's first
failure landed on a non-sampled occurrence and was never logged,
hiding it from operators entirely (PR #193 review).

perKeySampledWarn keys the sampler by org name so every distinct org
gets its own guaranteed 1st/10th/100th/every-1000th occurrence,
independent of how noisy any other org is.

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

---------

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
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.

3 participants