chore: gh agent config also works with direct org id#1028
chore: gh agent config also works with direct org id#1028adityachoudhari26 merged 2 commits intomainfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 2 minutes and 11 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughExported the internal GitHub App client helper as Changes
Sequence DiagramsequenceDiagram
participant Caller as Dispatcher/Test
participant Config as ParseJobAgentConfig
participant Resolver as resolveOwner
participant Client as AppClient
participant API as GitHub API
Caller->>Config: ParseJobAgentConfig(ctx, config)
Config->>Resolver: resolveOwner(ctx, config)
alt owner field present
Resolver->>Resolver: return cfg["owner"]
else organizationId provided
Resolver->>Client: AppClient()
Client->>API: Organizations.GetByID(orgID)
API-->>Client: organization
Client-->>Resolver: GitHub client & org
Resolver->>Resolver: extract & return org login
end
Resolver-->>Config: owner string
Config->>Config: construct GithubJobAgentConfig
Config-->>Caller: config or error
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related Issues
Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
🧹 Nitpick comments (1)
apps/workspace-engine/pkg/jobagents/github/githubaction_test.go (1)
209-298: Signature updates look good; consider adding coverage for the neworganizationIdpath.All existing cases correctly pass
context.Background(). However, none of the tests exercise the neworganizationId→ org-login resolution inresolveOwner. As noted onconfig.go, this path is currently unreachable in tests becausegh.AppClient()is called directly; once an injection seam is in place, please add tests for (a) successful org lookup, (b)GetByIDfailure, and (c) empty login → error.Also worth noting:
TestParseJobAgentConfig_MissingOwnerandTestParseJobAgentConfig_EmptyOwnernow pass only incidentally — the returned error message "owner or organizationId is required" happens to contain "owner". If you tighten the assertion (e.g., check fororganizationIdtoo), the tests will better reflect the new contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/jobagents/github/githubaction_test.go` around lines 209 - 298, Tests do not cover the new organizationId→org-login resolution path in resolveOwner and rely on indirect error text; add unit tests for ParseJobAgentConfig that inject a mock GitHub App client (replace direct gh.AppClient() call with an injection seam) and exercise: (a) successful org lookup where organizationId resolves to a non-empty login, (b) GetByID failure from the app client returning an error, and (c) GetByID returning an empty login which should produce an error; also tighten the assertions in TestParseJobAgentConfig_MissingOwner and TestParseJobAgentConfig_EmptyOwner to expect the combined error message about "owner or organizationId" (or include organizationId) so they remain correct after adding the org-id path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/workspace-engine/pkg/jobagents/github/config.go`:
- Around line 55-80: resolveOwner currently calls gh.AppClient() directly which
makes the organizationId branch untestable and brittle; refactor by injecting an
org-resolver dependency (e.g., an interface OrgResolver with GetByID(ctx, id) or
a client-factory function) into ParseJobAgentConfig/GithubAction and change
resolveOwner to accept that resolver instead of calling gh.AppClient(); update
callers to pass gh.AppClient-backed resolver in production and a fake in tests,
then add unit tests covering: successful org lookup → login returned, GetByID
returning an error, and GetByID returning an org with empty login; additionally
handle the case where bot credentials are not configured by returning a clearer
"github bot not configured" error when the app client cannot be created (or
document the requirement) instead of the generic "create github app client"
error.
---
Nitpick comments:
In `@apps/workspace-engine/pkg/jobagents/github/githubaction_test.go`:
- Around line 209-298: Tests do not cover the new organizationId→org-login
resolution path in resolveOwner and rely on indirect error text; add unit tests
for ParseJobAgentConfig that inject a mock GitHub App client (replace direct
gh.AppClient() call with an injection seam) and exercise: (a) successful org
lookup where organizationId resolves to a non-empty login, (b) GetByID failure
from the app client returning an error, and (c) GetByID returning an empty login
which should produce an error; also tighten the assertions in
TestParseJobAgentConfig_MissingOwner and TestParseJobAgentConfig_EmptyOwner to
expect the combined error message about "owner or organizationId" (or include
organizationId) so they remain correct after adding the org-id path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a3db283e-82fa-4093-91e8-f481b6ab58c9
📒 Files selected for processing (4)
apps/workspace-engine/pkg/github/client.goapps/workspace-engine/pkg/jobagents/github/config.goapps/workspace-engine/pkg/jobagents/github/githubaction.goapps/workspace-engine/pkg/jobagents/github/githubaction_test.go
There was a problem hiding this comment.
Pull request overview
Updates the workspace-engine GitHub Actions job agent to accept a GitHub organizationId in config (in addition to owner) by resolving the org login via the GitHub API, and threads context through config parsing.
Changes:
- Update
ParseJobAgentConfigto accept acontext.Contextand support resolvingownerfromorganizationId. - Export
AppClient()in the GitHub client package and use it where an app-authenticated client is needed. - Update the GitHub Actions job agent and its tests to pass context into config parsing.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| apps/workspace-engine/pkg/jobagents/github/githubaction_test.go | Updates unit tests to match the new ParseJobAgentConfig(ctx, ...) signature. |
| apps/workspace-engine/pkg/jobagents/github/githubaction.go | Passes dispatch context into GitHub job agent config parsing. |
| apps/workspace-engine/pkg/jobagents/github/config.go | Adds organizationId support by resolving owner via GitHub API; introduces resolveOwner. |
| apps/workspace-engine/pkg/github/client.go | Exports AppClient() and updates internal callers to use it. |
Comments suppressed due to low confidence (2)
apps/workspace-engine/pkg/jobagents/github/config.go:36
- ParseJobAgentConfig resolves owner (and may call out to the GitHub API) before validating other required fields like repo/workflowId. This means an invalid config could still trigger a network call. Consider validating all purely-local required fields first, then doing organizationId -> owner resolution only after the rest of the config has passed basic validation.
installationId := toInt(jobAgentConfig["installationId"])
if installationId == 0 {
return oapi.GithubJobAgentConfig{}, fmt.Errorf("installationId is required")
}
owner, err := resolveOwner(ctx, jobAgentConfig)
if err != nil {
return oapi.GithubJobAgentConfig{}, err
}
repo, ok := jobAgentConfig["repo"].(string)
if !ok || repo == "" {
return oapi.GithubJobAgentConfig{}, fmt.Errorf("repo is required")
}
workflowId := toInt64(jobAgentConfig["workflowId"])
if workflowId == 0 {
return oapi.GithubJobAgentConfig{}, fmt.Errorf("workflowId is required")
}
apps/workspace-engine/pkg/jobagents/github/githubaction_test.go:216
- ParseJobAgentConfig now supports resolving owner via organizationId, but the test suite here only exercises the explicit owner path. Add coverage for organizationId resolution (success and failure cases), ideally without relying on real GitHub credentials (e.g., by injecting a client/factory or using a mockable interface).
func TestParseJobAgentConfig_Valid(t *testing.T) {
cfg, err := ParseJobAgentConfig(context.Background(), validConfig())
require.NoError(t, err)
assert.Equal(t, 12345, cfg.InstallationId)
assert.Equal(t, "my-org", cfg.Owner)
assert.Equal(t, "my-repo", cfg.Repo)
assert.Equal(t, int64(42), cfg.WorkflowId)
assert.Nil(t, cfg.Ref)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| client, err := gh.AppClient() | ||
| if err != nil { | ||
| return "", fmt.Errorf("create github app client: %w", err) | ||
| } | ||
|
|
||
| org, _, err := client.Organizations.GetByID(ctx, orgID) | ||
| if err != nil { | ||
| return "", fmt.Errorf("lookup organization %d: %w", orgID, err) |
Summary by CodeRabbit
organizationIdfield, enabling automatic resolution of organization details through GitHub API lookup when the direct owner field is not provided.