[CE-235] baton-github: support containerization. #120
Conversation
WalkthroughThis PR replaces per-method pagination/annotation returns with a unified SyncOpAttrs/SyncOpResults API across resource types, adds NewLambdaConnector with PAT/App flows and V2 wiring, moves org-name caching to session-backed storage, and updates configuration fields and tests to match the new APIs. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/connector/connector.go`:
- Around line 256-281: The function NewLambdaConnector currently accepts an
unused cliOpts parameter; either remove cliOpts from the signature and all
callers (and update any docs/tests), or wire cliOpts into the auth flow so
Lambda honors CLI-provided options (e.g. consult cliOpts.TokenSource and
cliOpts.SelectedAuthMethod to override ghc.Token/ghc.AppId/ghc.AppPrivatekeyPath
before calling New(ctx, ghc, privateKey)). If the omission is intentional, add a
clear comment above NewLambdaConnector stating that Lambda-only connectors
intentionally ignore CLI options and why; otherwise update the code paths that
construct ghc or pass credentials so New and its callers receive the CLI-auth
values.
| func NewLambdaConnector(ctx context.Context, ghc *cfg.Github, cliOpts *cli.ConnectorOpts) (connectorbuilder.ConnectorBuilderV2, []connectorbuilder.Opt, error) { | ||
| // TODO(Ben.Su) Update to use field groups rather than schema relationships. | ||
| if err := field.Validate(cfg.Config, ghc); err != nil { | ||
| return nil, nil, err | ||
| } | ||
|
|
||
| var ( | ||
| l = ctxzap.Extract(ctx) | ||
| privateKey string | ||
| ) | ||
| if ghc.AppPrivatekeyPath != "" { | ||
| keyBytes, err := os.ReadFile(ghc.AppPrivatekeyPath) | ||
| if err != nil { | ||
| l.Error("error reading app private key file", zap.Error(err), zap.String("appPrivateKeyPath", ghc.AppPrivatekeyPath)) | ||
| return nil, nil, fmt.Errorf("failed to read app private key file: %w", err) | ||
| } | ||
| privateKey = string(keyBytes) | ||
| } | ||
|
|
||
| cb, err := New(ctx, ghc, privateKey) | ||
| if err != nil { | ||
| l.Error("error creating connector", zap.Error(err)) | ||
| return nil, nil, err | ||
| } | ||
| return cb, nil, nil | ||
| } |
There was a problem hiding this comment.
nit: @jugonzalez12 added auth method to connector opts, so having the field group should simplify this logic
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
btipling
left a comment
btipling
left a comment
There was a problem hiding this comment.
Looks fine but I'm not sure if the org cache should be via the session store. It always fetches the org when it doesn't already exist, it persists it on the connector struct, not global memory and I think this is faster than always getting from the session store but I defer to Lauren or Matt on this.
jugonzalez12
left a comment
jugonzalez12
left a comment
There was a problem hiding this comment.
modulo question about session store
| @@ -0,0 +1,39 @@ | |||
| package connector | |||
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/connector/org_role_test.go`:
- Around line 65-72: The loop should fail fast when
bag.Unmarshal(results.NextPageToken) returns an error to avoid processing the
same invalid token repeatedly; replace the current t.Error(err) handling in the
loop that processes results.NextPageToken (around
test.AssertHasRatelimitAnnotations and bag.Unmarshal) with a failing call (e.g.,
t.Fatalf or t.Fatal with the error) so the test exits immediately on unmarshal
failure rather than continuing the loop.
| test.AssertHasRatelimitAnnotations(t, results.Annotations) | ||
| if results.NextPageToken == "" { | ||
| break | ||
| } | ||
|
|
||
| err = bag.Unmarshal(nextToken) | ||
| err = bag.Unmarshal(results.NextPageToken) | ||
| if err != nil { | ||
| t.Error(err) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
baton-sdk pagination Bag Unmarshal error handling
💡 Result:
In baton-sdk, pagination tokens are typically managed with pagination.Bag, which is a stack of PageState values that can be serialized/deserialized into the pageToken string you receive on subsequent calls. Bag exposes Push/Pop/Current, PageToken(), and helpers like Next() / NextToken() plus Marshal() / Unmarshal(). [1]
Typical pagination flow (what to do, and where Unmarshal errors come from)
- First page:
pageToken == ""(or absent) → create a newBag,Push()an initialPageState(resource type/id + any initial upstream token), then fetch. - Next pages:
pageToken != ""→bag.Unmarshal(pageToken)to restore state, then call your upstream API usingbag.PageToken()(the “cursor” stored in the currentPageState). [1] - After fetching a page:
- If upstream returned a next cursor, advance the bag with
bag.Next(nextCursor)(orNextToken(nextCursor)if you want the new marshaled token in one step). [1]
- If upstream returned a next cursor, advance the bag with
A Bag.Unmarshal error almost always means the incoming pageToken string is not a token previously produced by Bag.Marshal() / NextToken(), e.g. it’s truncated, from a different connector/version, manually edited, or otherwise malformed. PageState is the underlying structure being restored (token, type, id). [1]
Error handling guidance
- Treat
Unmarshalfailures as an invalid pagination token and fail the request (don’t silently restart from page 1, or you can loop/duplicate data). - Return a clear error that includes context (“invalid page token”), and optionally suggest rerunning from the beginning.
- If you’re converting from the Baton “token object” pattern, note there’s also
ErrNoTokenused when a pagination token object is unexpectedly nil. [1]
Minimal Go sketch
bag := &pagination.Bag{}
if pageToken != "" {
if err := bag.Unmarshal(pageToken); err != nil {
return nil, fmt.Errorf("invalid page token: %w", err)
}
} else {
bag.Push(pagination.PageState{
ResourceTypeID: resourceTypeID,
ResourceID: resourceID,
Token: "", // initial upstream cursor if any
})
}
// use bag.PageToken() when calling the upstream API
// ...
// if upstreamNextCursor != "":
_ = bag.Next(upstreamNextCursor) // or bag.NextToken(...)
outToken, _ := bag.Marshal()Connector-dev docs also call out that baton-sdk includes pagination helpers for API calls, which is where this pattern fits. [2]
Sources:
[1] baton-sdk pkg/pagination package docs (Bag/GenBag APIs, ErrNoToken, PageState fields) (pkg.go.dev)
[2] ConductorOne developer docs: baton-sdk includes pagination helpers (conductorone.com)
Citations:
- 1: https://pkg.go.dev/github.com/conductorone/baton-sdk%40v0.7.13/pkg/pagination
- 2: https://www.conductorone.com/docs/developer/intro/?utm_source=openai
🏁 Script executed:
# Check the pagination loop structure and context around the error handling
cd pkg/connector && head -n 80 org_role_test.go | tail -n 40Repository: ConductorOne/baton-github
Length of output: 966
🏁 Script executed:
# Search for other usages of Bag.Unmarshal in the codebase to see common patterns
rg "Unmarshal.*NextPageToken|bag\.Unmarshal" -A 3 -B 2Repository: ConductorOne/baton-github
Length of output: 1166
🏁 Script executed:
# Check imports to understand what 'bag' is and where it comes from
head -n 30 pkg/connector/org_role_test.goRepository: ConductorOne/baton-github
Length of output: 997
Fail fast on next-page token unmarshal errors to avoid test hangs.
t.Error logs the error but allows the loop to continue. If Unmarshal fails on an invalid token, the bag state remains unchanged, potentially causing the same token to be processed repeatedly in the next iteration. Per baton-sdk guidance, unmarshal failures indicate an invalid pagination token and should fail the request immediately.
💡 Suggested fix
err = bag.Unmarshal(results.NextPageToken)
- if err != nil {
- t.Error(err)
- }
+ require.NoError(t, err)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test.AssertHasRatelimitAnnotations(t, results.Annotations) | |
| if results.NextPageToken == "" { | |
| break | |
| } | |
| err = bag.Unmarshal(nextToken) | |
| err = bag.Unmarshal(results.NextPageToken) | |
| if err != nil { | |
| t.Error(err) | |
| test.AssertHasRatelimitAnnotations(t, results.Annotations) | |
| if results.NextPageToken == "" { | |
| break | |
| } | |
| err = bag.Unmarshal(results.NextPageToken) | |
| require.NoError(t, err) |
🤖 Prompt for AI Agents
In `@pkg/connector/org_role_test.go` around lines 65 - 72, The loop should fail
fast when bag.Unmarshal(results.NextPageToken) returns an error to avoid
processing the same invalid token repeatedly; replace the current t.Error(err)
handling in the loop that processes results.NextPageToken (around
test.AssertHasRatelimitAnnotations and bag.Unmarshal) with a failing call (e.g.,
t.Fatalf or t.Fatal with the error) so the test exits immediately on unmarshal
failure rather than continuing the loop.
|
@btipling |
pkg/connector/connector.go
Outdated
| } | ||
|
|
||
| func NewLambdaConnector(ctx context.Context, ghc *cfg.Github, cliOpts *cli.ConnectorOpts) (connectorbuilder.ConnectorBuilderV2, []connectorbuilder.Opt, error) { | ||
| // TODO(Ben.Su) Update to use field groups rather than schema relationships. |
There was a problem hiding this comment.
lmk when this change is addressed
| func NewLambdaConnector(ctx context.Context, ghc *cfg.Github, cliOpts *cli.ConnectorOpts) (connectorbuilder.ConnectorBuilderV2, []connectorbuilder.Opt, error) { | ||
| // TODO(Ben.Su) Update to use field groups rather than schema relationships. | ||
| if err := field.Validate(cfg.Config, ghc); err != nil { | ||
| return nil, nil, err | ||
| } | ||
|
|
||
| var ( | ||
| l = ctxzap.Extract(ctx) | ||
| privateKey string | ||
| ) | ||
| if ghc.AppPrivatekeyPath != "" { | ||
| keyBytes, err := os.ReadFile(ghc.AppPrivatekeyPath) | ||
| if err != nil { | ||
| l.Error("error reading app private key file", zap.Error(err), zap.String("appPrivateKeyPath", ghc.AppPrivatekeyPath)) | ||
| return nil, nil, fmt.Errorf("failed to read app private key file: %w", err) | ||
| } | ||
| privateKey = string(keyBytes) | ||
| } | ||
|
|
||
| cb, err := New(ctx, ghc, privateKey) | ||
| if err != nil { | ||
| l.Error("error creating connector", zap.Error(err)) | ||
| return nil, nil, err | ||
| } | ||
| return cb, nil, nil | ||
| } |
There was a problem hiding this comment.
nit: @jugonzalez12 added auth method to connector opts, so having the field group should simplify this logic
pkg/config/config.go
Outdated
| ) | ||
|
|
||
| const ( | ||
| GithubAppGroup = "github_app_group" |
There was a problem hiding this comment.
style convention is usually with - for baton
and then you can map the group names in c1
| var Config = field.NewConfiguration( | ||
| []field.SchemaField{ | ||
| accessTokenField, | ||
| orgsField, |
There was a problem hiding this comment.
what does baton-github-test do? I think we need to add a new field for org, since github app only takes a string org, not a list
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/connector/connector.go`:
- Around line 256-280: NewLambdaConnector currently dereferences
cliOpts.SelectedAuthMethod without checking cliOpts, and treats an empty
SelectedAuthMethod as a GitHub App path which can mis-route PAT-capable configs;
add a nil-check for cliOpts and treat an empty SelectedAuthMethod as a fallback:
if cliOpts is nil set group by inspecting ghc for a PAT (e.g., presence of
ghc.PersonalAccessToken or equivalent) and choose
cfg.GithubPersonalAccessTokenGroup when PAT data exists, otherwise choose the
GitHub App group; then use that group to decide between calling newWithGithubPAT
and newWithGithubApp (preserve existing error handling and keep field.Validate
usage).
🧹 Nitpick comments (1)
pkg/config/config.go (1)
94-95: Minor: Inconsistent capitalization inHelpText.Line 95 uses "github app" while other references use "GitHub App" (e.g., line 37: "GitHub App ID", line 43: "GitHub App private key"). Consider using consistent capitalization for brand names.
- HelpText: "Use a github app for authentication", + HelpText: "Use a GitHub App for authentication.",
| func NewLambdaConnector(ctx context.Context, ghc *cfg.Github, cliOpts *cli.ConnectorOpts) (connectorbuilder.ConnectorBuilderV2, []connectorbuilder.Opt, error) { | ||
| // TODO(Ben.Su) Update to use field groups rather than schema relationships. | ||
| if err := field.Validate(cfg.Config, ghc); err != nil { | ||
| return nil, nil, err | ||
| } | ||
|
|
||
| var ( | ||
| appClient *github.Client | ||
| ts = oauth2.StaticTokenSource( | ||
| &oauth2.Token{AccessToken: patToken}, | ||
| ) | ||
| group = cliOpts.SelectedAuthMethod | ||
| cb *GitHub | ||
| err error | ||
| ) | ||
| if jwttoken != "" { | ||
| if len(ghc.Orgs) != 1 { | ||
| return nil, fmt.Errorf("github-connector: only one org should be specified") | ||
| } | ||
|
|
||
| appClient, err = newGitHubClient(ctx, | ||
| ghc.InstanceUrl, | ||
| oauth2.StaticTokenSource( | ||
| &oauth2.Token{AccessToken: jwttoken}, | ||
| ), | ||
| ) | ||
| if group == cfg.GithubPersonalAccessTokenGroup { | ||
| cb, err = newWithGithubPAT(ctx, ghc) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| installation, err := findInstallation(ctx, appClient, ghc.Orgs[0]) | ||
| if err != nil { | ||
| return nil, err | ||
| return nil, nil, err | ||
| } | ||
| return cb, nil, nil | ||
| } | ||
|
|
||
| token, err := getInstallationToken(ctx, appClient, installation.GetID()) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| cb, err = newWithGithubApp(ctx, ghc) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
| return cb, nil, nil | ||
| } |
There was a problem hiding this comment.
Guard against nil cliOpts / empty auth selection to avoid panic or wrong auth path.
Line 263 dereferences cliOpts without a nil check, which will panic if any caller passes nil. Also, an empty SelectedAuthMethod currently defaults to GitHub App even when a PAT is present, which can route valid PAT configs into the wrong flow.
🛡️ Proposed guard + fallback selection
- var (
- group = cliOpts.SelectedAuthMethod
- cb *GitHub
- err error
- )
+ var (
+ group string
+ cb *GitHub
+ err error
+ )
+ if cliOpts != nil {
+ group = cliOpts.SelectedAuthMethod
+ }
+ if group == "" {
+ if ghc.Token != "" {
+ group = cfg.GithubPersonalAccessTokenGroup
+ } else {
+ group = cfg.GithubAppGroup
+ }
+ }To confirm call-site behavior, please verify whether cliOpts can be nil or SelectedAuthMethod can be empty:
#!/bin/bash
# Identify call sites and check how cliOpts is passed/initialized.
rg -n "NewLambdaConnector\\(" -C 3
rg -n "SelectedAuthMethod" -C 3🤖 Prompt for AI Agents
In `@pkg/connector/connector.go` around lines 256 - 280, NewLambdaConnector
currently dereferences cliOpts.SelectedAuthMethod without checking cliOpts, and
treats an empty SelectedAuthMethod as a GitHub App path which can mis-route
PAT-capable configs; add a nil-check for cliOpts and treat an empty
SelectedAuthMethod as a fallback: if cliOpts is nil set group by inspecting ghc
for a PAT (e.g., presence of ghc.PersonalAccessToken or equivalent) and choose
cfg.GithubPersonalAccessTokenGroup when PAT data exists, otherwise choose the
GitHub App group; then use that group to decide between calling newWithGithubPAT
and newWithGithubApp (preserve existing error handling and keep field.Validate
usage).
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all call sites of NewLambdaConnector to verify cliOpts is always non-nil
rg -nP '\bNewLambdaConnector\s*\(' -A 5 -B 2
# Search for SelectedAuthMethod initialization and usage patterns
rg -nP '\bSelectedAuthMethod\b' -C 3
# Check ConnectorOpts struct definition to understand the field
rg -nP 'type\s+ConnectorOpts\s+struct' -A 10
Length of output: 800
@Bencheng21, thanks for confirming that cliOpts is guaranteed non-nil by the SDK's calling convention—that addresses the panic concern.
However, there's a second issue in the original review that remains: what happens when SelectedAuthMethod is an empty string?
Looking at the current logic (lines 262-270):
group = cliOpts.SelectedAuthMethod
if group == cfg.GithubPersonalAccessTokenGroup {
cb, err = newWithGithubPAT(ctx, ghc)
// ...
}
// Falls through to GitHub App
cb, err = newWithGithubApp(ctx, ghc)If SelectedAuthMethod is "", it won't match GithubPersonalAccessTokenGroup, so the code will route to the GitHub App path at line 270. But your config defines the PAT group as the default (Default: true on line 97), so an empty selection should arguably choose PAT, not App.
Can SelectedAuthMethod ever be empty in practice? If so, should the code explicitly handle that case to align with the default group configuration?
🧬 Code Graph Analysis Results
-
File: pkg/customclient/client.go
- Lines 15-17
Client struct { *uhttp.BaseHttpClient } - Lines 19-23
func New(client *github.Client) *Client { return &Client{ BaseHttpClient: uhttp.NewBaseHttpClient(client.Client()), } }
- Lines 15-17
-
File: pkg/config/conf.gen.go
- Lines 6-16
Github struct { Token string `mapstructure:"token"` Orgs []string `mapstructure:"orgs"` Enterprises []string `mapstructure:"enterprises"` InstanceUrl string `mapstructure:"instance-url"` SyncSecrets bool `mapstructure:"sync-secrets"` OmitArchivedRepositories bool `mapstructure:"omit-archived-repositories"` AppId string `mapstructure:"app-id"` AppPrivatekeyPath string `mapstructure:"app-privatekey-path"` Org string `mapstructure:"org"` }
- Lines 6-16
-
File: pkg/config/config.go
-
Lines 75-107
Config = field.NewConfiguration( []field.SchemaField{ accessTokenField, orgsField, EnterprisesField, instanceUrlField, syncSecrets, omitArchivedRepositories, appIDField, appPrivateKeyPath, orgField, }, field.WithConstraints(fieldRelationships...), field.WithConnectorDisplayName("GitHub v2"), field.WithHelpUrl("/docs/baton/github-v2"), field.WithIconUrl("/static/app-icons/github.svg"), field.WithFieldGroups([]field.SchemaFieldGroup{ { Name: GithubPersonalAccessTokenGroup, DisplayName: "Personal access token", HelpText: "Use a personal access token for authentication.", Fields: []field.SchemaField{accessTokenField, orgsField, omitArchivedRepositories}, Default: true, }, { Name: GithubAppGroup, DisplayName: "GitHub app", HelpText: "Use a github app for authentication", Fields: []field.SchemaField{appIDField, appPrivateKeyPath, orgField, syncSecrets, omitArchivedRepositories}, Default: false, }, }), ) -
Lines 8-8
GithubAppGroup = "github-app-group"
-
There was a problem hiding this comment.
falls back to the other authentication
There was a problem hiding this comment.
@Bencheng21, thanks for the confirmation on cliOpts.
Regarding your comment "falls back to the other authentication"—can you clarify what you mean?
Looking at the code flow:
- Line 263:
if group == cfg.GithubPersonalAccessTokenGroup→ uses PAT - Line 271: Otherwise (including when
group == "") → uses GitHub App
Question: When SelectedAuthMethod is empty (e.g., user doesn't explicitly select an auth method), does the SDK/CLI layer automatically populate it with the default group value (GithubPersonalAccessTokenGroup since Default: true on line 97)?
Or are you saying that when SelectedAuthMethod is empty, falling back to the GitHub App path is the intended behavior, even though PAT is marked as the default in the field group configuration?
If the SDK ensures SelectedAuthMethod is always populated with the default, then the empty-string case won't occur in practice and the current code is fine. But if empty is possible, we should verify the fallback aligns with the field group defaults.
🐰
pkg/config/config.go
Outdated
| DisplayName: "GitHub app", | ||
| HelpText: "Use a github app for authentication", | ||
| Fields: []field.SchemaField{appIDField, appPrivateKeyPath, orgsField, syncSecrets, omitArchivedRepositories}, | ||
| Default: true, |
There was a problem hiding this comment.
we should set the default to GithubPersonalAccessTokenGroup since app was added later so there are probably existing configs with no group since there were no field groups previously
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@pkg/connector/connector.go`:
- Around line 340-360: The code stores a request Context in appTokenRefresher
which can become stale; instead remove the ctx field from appTokenRefresher and
ensure token refreshes use a long‑lived context (e.g. context.Background()).
Concretely: update the appTokenRefresher type to drop the ctx member, update its
Token refresh implementation to use context.Background() (or accept a context
parameter during the actual refresh call), and change the
oauth2.ReuseTokenSource construction (the appTokenRefresher instantiation) to no
longer pass ctx but rely on the background context for long‑lived token
refreshes; keep references to appTokenRefresher, oauth2.ReuseTokenSource, and
appJWTTokenRefresher consistent.
- Around line 282-305: newWithGithubPAT may receive an empty personal access
token (ghc.Token), which creates an oauth2 client with no credentials and yields
confusing downstream failures; add a defensive validation at the start of
newWithGithubPAT that returns a clear error when ghc.Token == "" (e.g.,
fmt.Errorf("github personal access token is required")) so callers fail fast
with an actionable message; keep the check before creating
oauth2.StaticTokenSource and reference the function name newWithGithubPAT and
the ghc.Token field when implementing this change.
| func newWithGithubPAT(ctx context.Context, ghc *cfg.Github) (*GitHub, error) { | ||
| ts := oauth2.StaticTokenSource( | ||
| &oauth2.Token{AccessToken: ghc.Token}, | ||
| ) | ||
| ghClient, err := newGitHubClient(ctx, ghc.InstanceUrl, ts) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| graphqlClient, err := newGitHubGraphqlClient(ctx, ghc.InstanceUrl, ts) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return &GitHub{ | ||
| client: ghClient, | ||
| customClient: customclient.New(ghClient), | ||
| instanceURL: ghc.InstanceUrl, | ||
| orgs: ghc.Orgs, | ||
| enterprises: ghc.Enterprises, | ||
| graphqlClient: graphqlClient, | ||
| orgCache: newOrgNameCache(ghClient), | ||
| syncSecrets: ghc.SyncSecrets, | ||
| omitArchivedRepositories: ghc.OmitArchivedRepositories, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the Token field definition and any validation rules
rg -n "Token" pkg/config/ -B 2 -A 2 | head -100Repository: ConductorOne/baton-github
Length of output: 1915
🏁 Script executed:
#!/bin/bash
# Find field.Validate to understand what it validates
rg -n "field\.Validate|func.*Validate" pkg/ -B 2 -A 5 --max-count=50Repository: ConductorOne/baton-github
Length of output: 1203
🏁 Script executed:
#!/bin/bash
# Find where newWithGithubPAT is called
rg -n "newWithGithubPAT" pkg/ -B 3 -A 3Repository: ConductorOne/baton-github
Length of output: 789
🏁 Script executed:
#!/bin/bash
# Look at the GitHub config structure
rg -n "type Github struct" pkg/config/ -A 20Repository: ConductorOne/baton-github
Length of output: 1332
🏁 Script executed:
#!/bin/bash
# Look at the full accessTokenField definition
rg -n "accessTokenField\s*=" pkg/config/config.go -A 10Repository: ConductorOne/baton-github
Length of output: 480
🏁 Script executed:
#!/bin/bash
# Check what field.Validate does and if it validates required fields
rg -n "func.*Validate.*field|WithRequired" . --type go | head -30Repository: ConductorOne/baton-github
Length of output: 1912
🏁 Script executed:
#!/bin/bash
# Check field group logic - is one of the groups required?
rg -n "WithFieldGroups|Default.*true" pkg/config/config.go -B 2 -A 2Repository: ConductorOne/baton-github
Length of output: 489
🏁 Script executed:
#!/bin/bash
# Look for field.Validate implementation to understand what it validates
rg -n "func.*Validate.*cfg\.|func Validate" vendor/github.com/conductorone/baton-sdk/pkg/field/ -A 15 | head -60Repository: ConductorOne/baton-github
Length of output: 5606
🏁 Script executed:
#!/bin/bash
# Check if there's any validation in NewLambdaConnector after field.Validate call
rg -n "field.Validate" pkg/connector/connector.go -A 20 | head -50Repository: ConductorOne/baton-github
Length of output: 539
🏁 Script executed:
#!/bin/bash
# Search for any other validation of Token field in the codebase
rg -n "ghc\.Token|\.Token\s*=|\.Token\s*==|Token.*empty" pkg/ --type goRepository: ConductorOne/baton-github
Length of output: 260
🏁 Script executed:
#!/bin/bash
# Find the Validate function that takes (config, instance) parameters
rg -n "^func Validate\(" vendor/github.com/conductorone/baton-sdk/pkg/field/ -A 30Repository: ConductorOne/baton-github
Length of output: 3154
🏁 Script executed:
#!/bin/bash
# Look at how field groups are validated and if one must have required fields filled
rg -n "FieldsMutuallyExclusive|field.*group|ValidateRequired" vendor/github.com/conductorone/baton-sdk/pkg/field/ -A 5 -B 2 | head -80Repository: ConductorOne/baton-github
Length of output: 4417
Add validation for non-empty personal access token.
The Token field in accessTokenField is not marked with WithRequired(true), so field.Validate will not catch an empty token. An empty token can reach this function, creating an OAuth2 client with an empty access token, leading to authentication failures with unclear error messages downstream. A defensive check here provides clearer error context immediately.
Suggested fix
func newWithGithubPAT(ctx context.Context, ghc *cfg.Github) (*GitHub, error) {
+ if ghc.Token == "" {
+ return nil, errors.New("personal access token is required")
+ }
ts := oauth2.StaticTokenSource(
&oauth2.Token{AccessToken: ghc.Token},
)🤖 Prompt for AI Agents
In `@pkg/connector/connector.go` around lines 282 - 305, newWithGithubPAT may
receive an empty personal access token (ghc.Token), which creates an oauth2
client with no credentials and yields confusing downstream failures; add a
defensive validation at the start of newWithGithubPAT that returns a clear error
when ghc.Token == "" (e.g., fmt.Errorf("github personal access token is
required")) so callers fail fast with an actionable message; keep the check
before creating oauth2.StaticTokenSource and reference the function name
newWithGithubPAT and the ghc.Token field when implementing this change.
| ts := oauth2.ReuseTokenSource( | ||
| &oauth2.Token{ | ||
| AccessToken: token.GetToken(), | ||
| Expiry: token.GetExpiresAt().Time, | ||
| }, | ||
| &appTokenRefresher{ | ||
| ctx: ctx, | ||
| instanceURL: ghc.InstanceUrl, | ||
| installationID: installation.GetID(), | ||
| jwtTokenSource: oauth2.ReuseTokenSource( | ||
| &oauth2.Token{ | ||
| AccessToken: jwttoken, | ||
| Expiry: time.Now().Add(jwtExpiryTime), | ||
| }, | ||
| &appJWTTokenRefresher{ | ||
| appID: ghc.AppId, | ||
| privateKey: string(keyBytes), | ||
| }, | ||
| ), | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Storing context.Context in struct can cause stale context issues.
The appTokenRefresher struct stores ctx at construction time (line 346), but token refresh may occur much later when this context could be cancelled or stale. The oauth2.ReuseTokenSource may call Token() during any API request, potentially long after the original context has expired.
🛠️ Suggested approach: Pass context through or use background context for long-lived token sources
Consider either:
- Using
context.Background()for the token refresher since it's long-lived, or - Restructuring to pass context through the call chain
ts := oauth2.ReuseTokenSource(
&oauth2.Token{
AccessToken: token.GetToken(),
Expiry: token.GetExpiresAt().Time,
},
&appTokenRefresher{
- ctx: ctx,
+ ctx: context.Background(),
instanceURL: ghc.InstanceUrl,
installationID: installation.GetID(),Note: Using context.Background() loses cancellation propagation but avoids stale context. Evaluate the trade-off based on your token refresh requirements.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ts := oauth2.ReuseTokenSource( | |
| &oauth2.Token{ | |
| AccessToken: token.GetToken(), | |
| Expiry: token.GetExpiresAt().Time, | |
| }, | |
| &appTokenRefresher{ | |
| ctx: ctx, | |
| instanceURL: ghc.InstanceUrl, | |
| installationID: installation.GetID(), | |
| jwtTokenSource: oauth2.ReuseTokenSource( | |
| &oauth2.Token{ | |
| AccessToken: jwttoken, | |
| Expiry: time.Now().Add(jwtExpiryTime), | |
| }, | |
| &appJWTTokenRefresher{ | |
| appID: ghc.AppId, | |
| privateKey: string(keyBytes), | |
| }, | |
| ), | |
| }, | |
| ) | |
| ts := oauth2.ReuseTokenSource( | |
| &oauth2.Token{ | |
| AccessToken: token.GetToken(), | |
| Expiry: token.GetExpiresAt().Time, | |
| }, | |
| &appTokenRefresher{ | |
| ctx: context.Background(), | |
| instanceURL: ghc.InstanceUrl, | |
| installationID: installation.GetID(), | |
| jwtTokenSource: oauth2.ReuseTokenSource( | |
| &oauth2.Token{ | |
| AccessToken: jwttoken, | |
| Expiry: time.Now().Add(jwtExpiryTime), | |
| }, | |
| &appJWTTokenRefresher{ | |
| appID: ghc.AppId, | |
| privateKey: string(keyBytes), | |
| }, | |
| ), | |
| }, | |
| ) |
🤖 Prompt for AI Agents
In `@pkg/connector/connector.go` around lines 340 - 360, The code stores a request
Context in appTokenRefresher which can become stale; instead remove the ctx
field from appTokenRefresher and ensure token refreshes use a long‑lived context
(e.g. context.Background()). Concretely: update the appTokenRefresher type to
drop the ctx member, update its Token refresh implementation to use
context.Background() (or accept a context parameter during the actual refresh
call), and change the oauth2.ReuseTokenSource construction (the
appTokenRefresher instantiation) to no longer pass ctx but rely on the
background context for long‑lived token refreshes; keep references to
appTokenRefresher, oauth2.ReuseTokenSource, and appJWTTokenRefresher consistent.
Bencheng21
force-pushed
the
ben/CE235_containerize_github
branch
from
9f4c618 to
e449202
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/connector/repository_test.go (1)
65-69:⚠️ Potential issue | 🟡 MinorFail fast on pagination token unmarshal errors to prevent test hangs.
Using
t.Errorlogs the error but allows the loop to continue. IfUnmarshalfails, the bag state remains unchanged, causing the same token to be processed repeatedly. Userequire.NoErrorto fail immediately.💡 Proposed fix
err = bag.Unmarshal(results.NextPageToken) - if err != nil { - t.Error(err) - } + require.NoError(t, err)pkg/connector/enterprise_role.go (1)
42-52:⚠️ Potential issue | 🟠 MajorPotential race condition in cache initialization check.
The
len(o.roleUsersCache) == 0check at line 43 is performed outside the mutex lock, which could lead to a race condition where multiple goroutines simultaneously determine the cache is empty and callfillCacheconcurrently.🔒 Proposed fix to make the check thread-safe
func (o *enterpriseRoleResourceType) getRoleUsersCache(ctx context.Context) (map[string][]string, error) { + o.mu.Lock() + isEmpty := len(o.roleUsersCache) == 0 + o.mu.Unlock() + - if len(o.roleUsersCache) == 0 { + if isEmpty { if err := o.fillCache(ctx); err != nil { return nil, fmt.Errorf("baton-github: error caching user roles: %w", err) } } o.mu.Lock() defer o.mu.Unlock() return o.roleUsersCache, nil }Alternatively, consider using
sync.Oncefor initialization:type enterpriseRoleResourceType struct { resourceType *v2.ResourceType client *github.Client customClient *customclient.Client enterprises []string roleUsersCache map[string][]string mu *sync.Mutex + cacheOnce sync.Once + cacheErr error }
🤖 Fix all issues with AI agents
In `@pkg/connector/api_token.go`:
- Around line 114-121: The loop over tokens calls apiTokenResource(ctx, t) and
on error currently returns a non-nil resourceSdk.SyncOpResults together with
err; change this to match the file's standard pattern by returning (nil, nil,
err) when apiTokenResource fails. Locate the for _, t := range tokens loop in
pkg/connector/api_token.go and update the error branch that references
apiTokenResource, pageToken and annotations so it returns nil, nil, err instead
of returning a partial SyncOpResults.
🧹 Nitpick comments (1)
pkg/connector/user.go (1)
103-135: Inconsistent PerPage value usage.The
ListMembersOptionsat line 127-131 usesmaxPageSizeconstant, while other resource types (e.g.,api_token.goline 90,invitation.goline 68) useopts.PageToken.Size. This inconsistency could lead to different pagination behaviors across resource types.♻️ Proposed fix for consistency
listOpts := github.ListMembersOptions{ ListOptions: github.ListOptions{ Page: page, - PerPage: maxPageSize, + PerPage: opts.PageToken.Size, }, }
| for _, t := range tokens { | ||
| resource, err := apiTokenResource(ctx, t) | ||
| if err != nil { | ||
| return nil, pageToken, annotations, err | ||
| return nil, &resourceSdk.SyncOpResults{ | ||
| NextPageToken: pageToken, | ||
| Annotations: annotations, | ||
| }, err | ||
| } |
There was a problem hiding this comment.
Inconsistent error handling pattern.
When apiTokenResource fails within the loop, this code returns a non-nil SyncOpResults along with the error. This differs from all other error paths in this file and other resource types, which return (nil, nil, err). Returning partial results with an error can cause unexpected behavior in callers that may not expect both a result and an error.
🐛 Proposed fix for consistent error handling
for _, t := range tokens {
resource, err := apiTokenResource(ctx, t)
if err != nil {
- return nil, &resourceSdk.SyncOpResults{
- NextPageToken: pageToken,
- Annotations: annotations,
- }, err
+ return nil, nil, err
}
rv = append(rv, resource)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for _, t := range tokens { | |
| resource, err := apiTokenResource(ctx, t) | |
| if err != nil { | |
| return nil, pageToken, annotations, err | |
| return nil, &resourceSdk.SyncOpResults{ | |
| NextPageToken: pageToken, | |
| Annotations: annotations, | |
| }, err | |
| } | |
| for _, t := range tokens { | |
| resource, err := apiTokenResource(ctx, t) | |
| if err != nil { | |
| return nil, nil, err | |
| } | |
| rv = append(rv, resource) | |
| } |
🤖 Prompt for AI Agents
In `@pkg/connector/api_token.go` around lines 114 - 121, The loop over tokens
calls apiTokenResource(ctx, t) and on error currently returns a non-nil
resourceSdk.SyncOpResults together with err; change this to match the file's
standard pattern by returning (nil, nil, err) when apiTokenResource fails.
Locate the for _, t := range tokens loop in pkg/connector/api_token.go and
update the error branch that references apiTokenResource, pageToken and
annotations so it returns nil, nil, err instead of returning a partial
SyncOpResults.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/config/config.go (1)
14-74:⚠️ Potential issue | 🟠 MajorRemove unconditional
WithRequired(true)and use field constraints to gate required validation by auth group.Lines 19, 40, 49, and 66 mark PAT and App authentication fields as unconditionally required. In baton-sdk,
WithRequired(true)applies regardless of field group membership, so users selecting GitHub App auth will still be required to provide a personal access token (and vice versa), causing validation failures.Replace individual
WithRequired(true)with conditional constraints: useFieldsDependentOnto require fields only when their parent auth group is selected. For example: "if App group is active, thenappIDField,appPrivateKeyPath, andorgFieldare required."(The TODO on line 12 confirms this issue was previously flagged as uncertain.)
pkg/config/config.go
Outdated
| accessTokenField, | ||
| appPrivateKeyPath, | ||
| ), | ||
| field.FieldsRequiredTogether( |
There was a problem hiding this comment.
nit: I think we can just remove this FieldsRequiredTogether now that we mark them as required in the groups. These change how the UI looks now
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/config/config.go (1)
62-98:⚠️ Potential issue | 🟡 MinorTighten GitHub capitalization in user-facing copy.
Minor copy fix for consistency in UI/help text.✍️ Suggested copy edits
- field.WithDisplayName("Github App Organization"), - field.WithDescription("Organization of your github app"), + field.WithDisplayName("GitHub App Organization"), + field.WithDescription("Organization of your GitHub app"), @@ - HelpText: "Use a github app for authentication", + HelpText: "Use a GitHub app for authentication",
5 participants
Test
App authentication.
Pat token.
Grant
Revoke.
Summary by CodeRabbit
New Features
Refactor