CXP-383 Enable GitHub last-activity sync - #29
Conversation
# Conflicts: # go.mod # go.sum
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| AppPrivatekeyPath: ghc.AppPrivatekeyPath, | ||
| Org: ghc.Org, | ||
| DirectCollaboratorsOnly: ghc.DirectCollaboratorsOnly, | ||
| SyncLastActivity: ghc.SyncLastActivity, |
There was a problem hiding this comment.
🟡 Suggestion: DefaultCapabilitiesBuilder (lines 45-58) deliberately enumerates every conditionally-registered builder (APITokenBuilder, EnterpriseRoleBuilder, LicenseBuilder) so the config-less capabilities output advertises them all, but the new usage-app / "GitHub Activity" syncer is not listed and this type does not implement EventFeeds. Since builder.GetCapabilities derives resource types from ResourceSyncers() and CAPABILITY_EVENT_FEED_V2 from EventFeeds(), the default capabilities will omit both — which contradicts docs/connector.mdx:401 ("on a brand-new connector, this resource type is included by default"). Note newUsageAppBuilder/newUsageEventFeed are unexported in baton-github v0.4.5, so this likely needs an upstream export before it can be wired here.
There was a problem hiding this comment.
Known, already discussed on baton-github#188: DefaultCapabilitiesBuilder only feeds the offline capabilities CLI/catalog artifact (pre-install marketplace listing), not the live per-instance capability check that actually gates real syncs — that one calls GetMetadata() against the real running connector, which already implements EventFeeds()/usage-app correctly. So this has no effect on whether the feature works for a configured connector; it's a cosmetic gap in the pre-install catalog listing only. Deliberately not fixing it here — it'd require exporting currently-private constructors from baton-github for a purely cosmetic win.
Connector PR Review: CXP-383 Update the sdk's version and enable Usage Event FeedsBlocking Issues: 1 | Suggestions: 1 | Threads Resolved: 0 Review SummaryThe new commits since Security IssuesNone found. Correctness Issues
Suggestions
Prompt for AI agents |
Also document the GitHub App permission required for sync-last-activity alongside the existing PAT scope requirement. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Changes requested without context. I'll execute the review again
Connector PR Review: CXP-383 Enable GitHub last-activity syncBlocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0 Review SummaryThe new commit bumps vendored Security IssuesNone found. Correctness IssuesNone found. The Suggestions
Previously reported, still open (no new inline comments posted):
Prompt for AI agents |
The config field's display name is "Sync user last activity" (pkg/config/config.go:81), but the docs referred to it as "Sync last activity" in four places, which wouldn't match what users see in the C1 config UI. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
FeliLucero1
left a comment
FeliLucero1
left a comment
There was a problem hiding this comment.
Went through this focusing on the stuff authored here (the vendored v0.4.5 changes I'd raise upstream separately). Mostly docs — nothing blocking in the Go. The capabilities thread I'll leave to your reply there, but see the note on line 401, I think that one sentence is fixable regardless of how that lands.
Also minor: title says "update the sdk's version" but baton-sdk was already v0.30.0 on main — only baton-github moves. Same for the v0.26.0 → v0.30.0 line in the description.
Two leftovers in docs/connector.mdx I couldn't anchor inline (both outside the diff):
- Line ~30 still says pending invitations show as Unspecified — this PR is what changes that to Pending (the
invitation.gostatus change in v0.4.5). Worth updating in the same PR that breaks it. - Capabilities table (~17-27) doesn't have a GitHub Activity row yet — bot flagged this one too. Sync checked, Provision blank, footnote that it only syncs with
sync-last-activityon.
| AppPrivatekeyPath: ghc.AppPrivatekeyPath, | ||
| Org: ghc.Org, | ||
| DirectCollaboratorsOnly: ghc.DirectCollaboratorsOnly, | ||
| SyncLastActivity: ghc.SyncLastActivity, |
There was a problem hiding this comment.
Not about this line specifically, but while you're in here: v0.4.5 also adds AppPrivatekey (raw PEM) upstream, and appPrivateKeyPEM() prefers it over the path. Githubenterprise never got the field, so it's always empty for this connector.
Not a regression — app-privatekey-path is still WithRequired(true) so the fallback always hits — just means GitHub App users here can't use the paste-the-PEM option that baton-github has. Intentional?
There was a problem hiding this comment.
As discussed on the SU today, we will skip these and enable them only if requested
Requires a new Review
- Reword last-activity docs to reflect that attribution is global across all synced orgs, not per-organization - Drop the ambiguous "included by default on brand-new connectors" claim for the GitHub Activity resource type - Fix stale invitation status doc (Unspecified -> Pending, per the v0.4.5 vendor bump already in this PR) - Add missing GitHub Activity row to the capabilities table - Clarify that sync-last-activity is based on audit-log activity in the config field description (regenerated config_schema.json and README accordingly) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
JavierCarnelli-ConductorOne
changed the title
Resolved conflicts in go.mod/go.sum: kept baton-github v0.4.5 (this PR's change) and took baton-sdk v0.30.1 (main's newer patch bump, unrelated to this PR). Regenerated go.sum and vendor/ via `go mod tidy && go mod vendor` against the resolved go.mod; verified config_schema.json and baton_capabilities.json need no changes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| func nextAuditLogPage(resp *github.Response) string { | ||
| if resp.NextPageToken != "" { | ||
| return resp.NextPageToken | ||
| } | ||
| if resp.NextPage != 0 { | ||
| return strconv.Itoa(resp.NextPage) | ||
| } | ||
| return "" | ||
| } |
There was a problem hiding this comment.
🟠 Bug: nextAuditLogPage never finds the audit-log cursor on github.com/GHEC, so each org is truncated to one page (100 entries) per pass. GitHub's audit-log Link header uses after=<cursor> with no page= param, and go-github puts that value in Response.After — see its own doc comment: "For APIs that support before/after pagination, such as OrganizationsService.AuditLog. Before string / After string" (vendor/github.com/google/go-github/v69/github/github.go:672-674). In populatePageValues (same file, 731-750) page is "", so strconv.Atoi("") fails → NextPageToken = "" and NextPage = 0, while r.After gets the cursor. Both branches here return "", reachedBoundary is false, and the loop advances to the next org — silently dropping every entry past the first page, so members whose only recent activity falls beyond it get no last-activity date.
The request side has the matching half of the bug: the cursor is fed into ListCursorOptions.Page (line 153), but this endpoint expects ListCursorOptions.After. Fix: return resp.After (keeping the NextPageToken/NextPage fallbacks for GHES) and set opts.After from the stored cursor instead of opts.Page. Needs to land upstream in baton-github.
| case isNotFoundError(resp) || isPermissionError(resp): | ||
| l.Warn("org lacks audit-log access, skipping it for this pass", | ||
| zap.String("org", orgName), zap.Error(err)) | ||
| entries, resp = nil, nil |
There was a problem hiding this comment.
🟡 Suggestion: an org that permanently lacks audit-log access (no read:audit_log, or a plan without audit log) is re-probed and re-warned on every pass, and the feed polls continuously rather than once per sync. With several such orgs that is a steady stream of Warn lines plus one wasted API call per org per poll, indefinitely. Consider caching the known-inaccessible orgs on usageEventFeed (or sampling the warning per criteria L7) so the condition is reported once rather than every pass. Upstream fix in baton-github.
|
|
||
| require ( | ||
| github.com/conductorone/baton-github v0.4.0 | ||
| github.com/conductorone/baton-github v0.4.6 |
There was a problem hiding this comment.
🟡 Suggestion: the bump lands on v0.4.6, but the PR description says "v0.4.0 to v0.4.5" and its "Dependency bump impact" list only covers baton-github #184 and #181. The vendored diff also carries an unrelated, undescribed change set: a new app-privatekey config field, removal of WithRequired(true) from app-privatekey-path (now enforced at runtime in appPrivateKeyPEM), and \n-unescaping in loadPrivateKeyFromString. None of that affects this connector — pkg/config/config.go keeps its own required app-privatekey-path and NewLambdaConnector never sets AppPrivatekey — but please update the description so the review and release notes match what actually shipped.
Requires a new review
mateoHernandez123
left a comment
mateoHernandez123
left a comment
There was a problem hiding this comment.
Approving. The GHES-style truncation we flagged is addressed by the baton-github v0.4.6 bump (#192): nextAuditLogPage now follows numeric NextPage, which is the Link shape this connector's instance-URL / GHES path actually sees.
The built-in C1 schema finding was incorrect for a deployment-supported lambda — the field ships with the connector release schema, no hand-edit of the C1 config contract.
Leftover, not blocking here: github.com/GHEC after= cursors still land in resp.After and are still sent as ListCursorOptions.Page. This connector's docs send github.com users to the GitHub integration, so that belongs in baton-github, not this PR.
Picks up the audit-log pagination fix (resp.After checked first for the org audit-log endpoint's cursor, fixing silent truncation on GHEC and most GHES orgs) and per-org sampled warnings for orgs that permanently lack audit-log access (baton-github#193). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| if resp != nil && !reachedBoundary { | ||
| if nextPage, isPage := nextAuditLogPage(resp); nextPage != "" { | ||
| cursor.AuditLogCursor = nextPage | ||
| cursor.AuditLogCursorIsPage = isPage | ||
| continue | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: now that after= cursors actually advance, an empty page with a rel="next" link keeps the loop paging without ever advancing cursor.OrgIndex. reachedBoundary can only fire when an entry is at/before since, but created:>=<since> already filters those out server-side, so the only termination signal for an org is nextAuditLogPage returning "". Consider also advancing to the next org when len(entries) == 0, so a server that keeps emitting a next-link can't stall the pass. (Medium confidence — bounded per call by maxAuditLogPagesPerCall, but the stall would persist across polls.)
5 participants
Summary
sync-last-activityconfig field (wired into both the PAT and GitHub App auth groups) and threads it throughNewLambdaConnector, enabling GitHub org member last-activity tracking frombaton-github#188.baton-githubdependency from v0.4.0 to v0.4.7. (baton-sdkis already v0.30.0 onmain; this PR doesn't change it.)replacedirective used during development/testing; the vendored code now comes straight from the publishedbaton-githubmodule.Dependency bump impact (v0.4.0 → v0.4.7)
Applies to every existing install, regardless of whether
sync-last-activityis enabled:RESOURCE_STATUS_PENDINGinstead ofSTATUS_UNSPECIFIED(baton-github #184).User,Invitation,Team,OrgRole,App, andAPITokenresources (baton-github #181, an SDK-deprecation cleanup).These are one-time data-shape changes on the next sync; no action needed.
Test plan
go build ./...,go vet ./...,go test ./...passgolangci-lint run ./...clean🤖 Generated with Claude Code