Skip to content

CXP-383 Support Last Log In w/ usage event feed for the Enterprise connector - #188

Open
JavierCarnelli-ConductorOne wants to merge 7 commits into
mainfrom
jdc/cxp-383
Open

CXP-383 Support Last Log In w/ usage event feed for the Enterprise connector#188
JavierCarnelli-ConductorOne wants to merge 7 commits into
mainfrom
jdc/cxp-383

Conversation

@JavierCarnelli-ConductorOne

Copy link
Copy Markdown
Contributor

Adds an opt-in usage event feed that streams each org's audit-log activity (web/API actions, excluding raw git operations and bot actors) as incremental usage events, letting the platform derive last-activity for members without a per-user API field to sync directly. The config field is intentionally hidden from this connector's CLI/GUI since it only applies to GitHub Enterprise audit-log access; the enterprise connector variant sets it directly on the shared config struct.

This feature is build to be used by baton-github-enterprise which vendors this connector.

Adds an opt-in usage event feed that streams each org's audit-log
activity (web/API actions, excluding raw git operations and bot
actors) as incremental usage events, letting the platform derive
last-activity for members without a per-user API field to sync
directly. The config field is intentionally hidden from this
connector's CLI/GUI since it only applies to GitHub Enterprise
audit-log access; the enterprise connector variant sets it directly
on the shared config struct.

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

linear-code Bot commented Sep 2, 2026

Copy link
Copy Markdown

CXP-383

Comment thread pkg/connector/usage_event_feed.go Outdated
Comment thread pkg/connector/usage_event_feed.go
Comment thread pkg/connector/connector.go
Comment thread pkg/connector/usage_event_feed.go Outdated
Comment thread pkg/connector/usage_event_feed.go
Comment thread pkg/connector/usage_event_feed.go Outdated
Comment thread pkg/connector/usage_event_feed_test.go
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Connector PR Review: CXP-383 Support Last Log In w/ usage event feed for the Enterprise connector

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

Review Summary

The new commit addresses two of the three findings from the previous review: usage_event_feed.go now bounds-checks cursor.OrgIndex before indexing cursor.Orgs, resetting to index 0 and discarding the stale per-org cursor, and usageEventFromAuditEntry dropped its unused time.Time return, with all three call sites and the tests updated accordingly (verified no other callers remain). The full PR diff was re-scanned for security and correctness — config field plumbing, EventFeeds gating, the feed implementation, and tests — and no new issues were found; the incremental artifact reports no dropped paths and no truncation. The third prior finding, the fabricated STATUS_OVERLIMIT rate-limit annotation, is still present at pkg/connector/usage_event_feed.go:160-166 and is carried below as a suggestion rather than re-flagged inline.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

  • pkg/connector/usage_event_feed.go:160-166 (carried over from the previous review, still open) — when a response carries no X-Ratelimit-* headers, extractRateLimitData (pkg/connector/helpers.go:184-228) returns Remaining: 0 / STATUS_OVERLIMIT, and because the tightest-limit comparison only looks at Remaining, that value always wins — so the 404/403 skip path can attach a spurious over-limit annotation to an otherwise successful pass.
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 lines 160-166: `extractRateLimitData` (pkg/connector/helpers.go:184-228) returns a
  descriptor with `Remaining: 0` and `Status: STATUS_OVERLIMIT` whenever the response has no
  `X-Ratelimit-Remaining` header, because `r` stays at its zero value and the `if r <= 0`
  branch then marks it over-limit. The `tightestRateLimit` selection in ListEvents compares
  only `rl.GetRemaining()`, so that fabricated zero always beats any real value and becomes
  the rate-limit annotation returned for the whole call — including on the 404/403
  skip-and-continue path, where the request merely lacked audit-log permission and was not
  rate limited. Only use a rate-limit reading when the headers were actually present:
  either skip the update when the `X-Ratelimit-Remaining` header is empty, or ignore
  descriptors whose `Limit` is 0 (no headers present) so a header-less response cannot win
  the tightest-limit pick.

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

newWithGithubApp built its GitHub struct without copying
SyncLastActivity, so EventFeeds() always returned nil for App-authenticated
connectors regardless of the config value, with no error or log.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- Track the since-boundary explicitly instead of inferring it from
  whether an event was emitted, so a page of only filtered-out
  entries (bots, missing IDs) no longer ends an org's walk early and
  silently drops later pages of real activity.
- Surface the tightest rate limit seen across a call's audit-log
  requests as an annotation, including on error responses, so the
  SDK can pace polling instead of hitting 429s.
- Fall back to a synthesized event id (org/actor/timestamp/action)
  when GitHub omits _document_id, avoiding ambiguous dedup on empty ids.

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

Skip-and-continue is now restricted to permanent per-org conditions
(403/404), logged at Warn per this repo's log-level convention. Rate
limits, 5xx, and any other error now abort the call instead of
silently completing the pass and permanently losing the unfetched
activity window on the next poll.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Use nanosecond precision for the synthesized event id and the
persisted "since" cursor to avoid same-second id collisions and
duplicate re-emitted events. Also check every audit entry's raw
timestamp against the boundary, not just ones that pass the bot
filter, so an all-filtered page doesn't stall pagination past "since".

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The page budget is a single counter shared across every org processed
in a call, not a per-org cap as the comment claimed.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Comment thread pkg/connector/usage_event_feed.go
Comment thread pkg/connector/usage_event_feed.go
Comment thread pkg/connector/usage_event_feed.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.

@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 marked this pull request as ready for review September 3, 2026 06:31
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.

2 participants