feat: dynamic repository scoping for organization profiles#297
Draft
jamestelfer wants to merge 20 commits into
Draft
feat: dynamic repository scoping for organization profiles#297jamestelfer wants to merge 20 commits into
jamestelfer wants to merge 20 commits into
Conversation
Spec captures the design from GitHub issue #246: two new YAML literals ({{caller-scoped-repository}} and {{all-repositories}}) for organization profiles, with bidirectional validation, cache key changes, and deprecation of the '*' wildcard. Plan decomposes the work into 12 tasks following the existing codebase patterns, starting from RepositoryScope extension through to integration tests.
Add CallerScoped boolean field to RepositoryScope struct (JSON tag: callerScoped) to represent repositories supplied at request time rather than stored in profile. Changes: - New CallerScoped field with omitempty JSON tag - NewCallerScopedScope() constructor returning CallerScoped=true - IsCallerScoped() method to check caller-scoped state - Updated Contains() to return false for caller-scoped (no stored repos) - Updated IsZero() to exclude caller-scoped from zero value check - Updated NamesForDisplay() to return empty slice for caller-scoped - LogValue() automatically handles via NamesForDisplay() Comprehensive test coverage: - Added TestNewCallerScopedScope() - Added TestRepositoryScope_IsCallerScoped() - Extended all existing table-driven tests with caller-scoped cases - JSON marshaling/unmarshaling roundtrip tested - Log value assertions verified This is foundation for dynamic repository scoping where repos are provided per-request rather than stored in profile configuration.
Replace the raw Repositories []string field with a typed Scope field resolved at compile time. This eliminates repeated runtime interpretation of the wildcard marker and prepares the type for the caller-scoped state. The RepositoryScope() method now returns the stored value directly instead of re-deriving it on each call. Changes: - OrganizationProfileAttr: Replace Repositories []string with Scope RepositoryScope - HasRepository(): Delegate to Scope.Contains() instead of checking for wildcard - RepositoryScope(): Return stored Scope directly - Remove allowAllRepositories() method (replaced by Scope.IsWildcard()) - compileOrganizationProfiles: Add resolveRepositoryScope() to convert raw repositories lists into typed RepositoryScope during compilation - Update all test files to use NewSpecificScope/NewWildcardScope instead of raw repository lists
… in profiles
The profile compiler now accepts two new YAML literals:
- {{caller-scoped-repository}}: resolved to CallerScoped scope
- {{all-repositories}}: resolved to Wildcard scope
Both must be the sole entry in the repositories list. The existing '*'
wildcard is preserved as a deprecated alias for {{all-repositories}}
with a warning emitted at compile time.
Add extractRepositoryScope() to parse the repository-scope query parameter from organization token requests. Rejects empty values, whitespace-only values, and values containing '/' (no owner prefix). The value is passed through without case normalization per spec.
Add repositoryScope string parameter to ProfileTokenVendor and all implementations. Currently passed as empty string everywhere — the actual scoping logic follows in the next commit. Also add RepositoryScopeUnexpectedError and RepositoryScopeRequiredError types for bidirectional validation.
The org vendor now enforces strict bidirectional scoping rules: - caller-scoped profiles require a repository-scope parameter - non-scoped profiles reject a repository-scope parameter - all-repositories profiles reject a repository-scope parameter The handler extracts the repository-scope query parameter for org token requests and passes it through to the vendor chain.
For caller-scoped profiles at the git-credentials endpoint, the repository scope is derived from the Git-supplied URL in the request body. No new plugin parameters are needed. Both caller-scoped and all-repositories profiles return hard errors (not empty-success) on token issuance failure, since they claim coverage of all repositories.
When a repositoryScope is present, the cache key becomes
{digest}:{profile-ref}:{repository-name}. This ensures tokens for
different repositories under the same caller-scoped profile are cached
independently.
All-repositories profiles continue to use the two-component key since
a single token covers all repositories.
Cover caller-scoped and all-repositories profiles at both the token and git-credentials endpoints, including bidirectional validation and input rejection. Added: - testdata/org-profiles-scoped.yaml: Test profile YAML with caller-scoped, all-repositories, and static profiles for integration testing - harness_integration_test.go: OrganizationTokenScoped helper method for testing the repository-scope query parameter - api_integration_test.go: 7 new integration tests covering: - Caller-scoped profiles with valid repository scope (token + git-credentials) - Caller-scoped profiles rejecting missing scope - Static profiles rejecting provided scope - All-repositories profiles rejecting provided scope - Invalid scope values with slashes All tests pass with real HTTP handlers and mock external services.
Extract resolveRequestScope from NewOrgVendor to reduce cyclomatic complexity from 17 to within the 15 limit. The extracted function handles bidirectional scoping validation independently. Also use http.NewRequestWithContext in handler tests per noctx linter.
|
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 42 minutes and 59 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: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (18)
WalkthroughImplements dynamic repository scoping for organization profiles, introducing caller-scoped and all-repositories scope types. Adds repository scope extraction/validation at HTTP handlers, carries scope through ProfileRef and vendor chain, and updates profile compilation, org vendor scope resolution, and cache behavior accordingly. Changes
Sequence DiagramsequenceDiagram
participant Client
participant HTTPHandler
participant ProfileRefBuilder
participant ProfileStore
participant OrgVendor
participant CacheVendor
participant GitHub
Client->>HTTPHandler: POST /token?repository-scope=repo-a
HTTPHandler->>HTTPHandler: extractRepositoryScope("repo-a")
HTTPHandler->>ProfileRefBuilder: NewProfileRefBuilder(store, Org)
HTTPHandler->>ProfileRefBuilder: Build(ctx, PathValuer, scope="repo-a", implicitScope="")
ProfileRefBuilder->>ProfileStore: Lookup profile "my-profile"
ProfileStore-->>ProfileRefBuilder: OrganizationProfileAttr{Scope: CallerScoped}
ProfileRefBuilder->>ProfileRefBuilder: Validate scope vs profile type<br/>(caller-scoped requires scope)
ProfileRefBuilder-->>HTTPHandler: ProfileRef{ScopedRepository: "repo-a"}
HTTPHandler->>CacheVendor: ProfileTokenVendor(ctx, ref, repositories=["repo-a"])
CacheVendor->>CacheVendor: Compute cache key with ScopedRepository
CacheVendor->>OrgVendor: resolveRequestScope(CallerScoped, ref)
OrgVendor->>OrgVendor: Compute effective scope from ref.ScopedRepository
OrgVendor-->>CacheVendor: RepositoryScope{Names: ["repo-a"]}
CacheVendor->>OrgVendor: Vend token for "my-profile" with scope
OrgVendor->>GitHub: CreateAccessToken(repositories=["repo-a"])
GitHub-->>OrgVendor: AccessToken
OrgVendor-->>CacheVendor: ProfileToken{...}
CacheVendor-->>HTTPHandler: ProfileToken{...}
HTTPHandler-->>Client: 200 {ProfileToken with repo-a}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
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)
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 |
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 36 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
harness_integration_test.go (1)
393-416: Consider consolidating org-token request/parse logic to one helper.
OrganizationTokenScopedandOrganizationTokennow duplicate the same POST/HTTP-status/unmarshal path. A shared private helper would reduce drift risk when response handling changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@harness_integration_test.go` around lines 393 - 416, OrganizationTokenScoped and OrganizationToken duplicate POST/response/unmarshal logic; extract a private helper (e.g., a method named organizationTokenRequest or getOrganizationToken) that accepts token, profile, and an optional repositoryScope, builds the path (including url.QueryEscape for repositoryScope), performs c.Request("POST", ...), checks resp.StatusCode == http.StatusOK, calls c.parseError on non-OK, and unmarshals into a vendor.ProfileToken; then have OrganizationTokenScoped and OrganizationToken call that helper and return its result to remove duplicated code and centralize error/response handling.internal/vendor/cached.go (1)
56-59: Minor cleanup: avoid recomputing digest/ref strings while building cache key.This is functionally equivalent but simpler and slightly cheaper on hot paths.
♻️ Proposed refactor
- key := fmt.Sprintf("%s:%s", digester.Digest(), ref.String()) + digest := digester.Digest() + refKey := ref.String() + key := fmt.Sprintf("%s:%s", digest, refKey) if repositoryScope != "" { - key = fmt.Sprintf("%s:%s:%s", digester.Digest(), ref.String(), repositoryScope) + key = fmt.Sprintf("%s:%s:%s", digest, refKey, repositoryScope) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/vendor/cached.go` around lines 56 - 59, Avoid recomputing digester.Digest() and ref.String() when building the cache key in internal/vendor/cached.go: compute digestStr := digester.Digest() and refStr := ref.String() once, then build key using fmt.Sprintf with those variables and include repositoryScope only when non-empty (e.g., conditional fmt.Sprintf or a single format that appends repositoryScope). Update the key construction around the existing key variable to use digestStr and refStr instead of calling the methods twice.docs/superpowers/plans/2026-04-15-dynamic-repository-scoping.md (1)
9-9: Minor: Future Go version reference.The plan references "Go 1.26" which doesn't exist. This appears to be a placeholder for the fictional future date (April 2026). Consider updating to the actual Go version used in the project.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/plans/2026-04-15-dynamic-repository-scoping.md` at line 9, The "Tech Stack" line lists a non-existent Go version "Go 1.26"; replace that placeholder with the actual Go version used by the project (e.g., "Go 1.20" or whichever is current) by editing the string "Go 1.26" in the document so the Tech Stack accurately reflects the project's real Go runtime.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/superpowers/specs/2026-04-15-dynamic-repository-scoping-design.md`:
- Around line 162-167: The nested items under the three repository-scope cases
are currently formatted as an indented code block; change them to a proper
nested Markdown list so MD046 stops firing by removing the leading indentation
and using list markers (e.g., "-" or "1.") for the outer list and consistent
indentation + markers for the inner items; update the three entries referencing
`repository-scope`, `{{caller-scoped-repository}}` (Req 2.2/2.3) and
`{{all-repositories}}` (Req 5.2) to be normal list items instead of indented
code lines so the block renders as a nested list rather than a code block.
In `@internal/vendor/cached_test.go`:
- Around line 654-679: Update TestCacheAllRepositories_SameKeyAsWildcard so the
two invocations of the cached vendor function v vary the requested repository
URL instead of repeating the same args: keep the profile ref as-is (ProfileRef
organization "org", Name "all-repos-profile", Type ProfileTypeOrg) and call v
the first time with one requestedRepoURL value (e.g., "https://repo1") and the
second time with a different requestedRepoURL (e.g., "https://repo2"), then
assert both calls return the same cached token ("first-call"); this ensures the
cache key used by v (from newTestCached / cached implementation) does not depend
on the requestedRepoURL for all-repositories profiles.
In `@internal/vendor/orgvendor.go`:
- Around line 106-119: When profileScope.IsCallerScoped() is true, change the
branching so requestedRepoURL is prioritized: first, if requestedRepoURL != ""
parse it (github.RepoForURL) and derive the repository via
profile.NewSpecificScope; if repositoryScope is also provided but does not match
the derived repository, return an error (e.g., fmt.Errorf or a specific mismatch
error) instead of honoring repositoryScope; only if requestedRepoURL == "" fall
back to using repositoryScope (return
profile.NewSpecificScope(repositoryScope)), and if neither is present return
profile.RepositoryScopeRequiredError{ProfileName: profileName}.
---
Nitpick comments:
In `@docs/superpowers/plans/2026-04-15-dynamic-repository-scoping.md`:
- Line 9: The "Tech Stack" line lists a non-existent Go version "Go 1.26";
replace that placeholder with the actual Go version used by the project (e.g.,
"Go 1.20" or whichever is current) by editing the string "Go 1.26" in the
document so the Tech Stack accurately reflects the project's real Go runtime.
In `@harness_integration_test.go`:
- Around line 393-416: OrganizationTokenScoped and OrganizationToken duplicate
POST/response/unmarshal logic; extract a private helper (e.g., a method named
organizationTokenRequest or getOrganizationToken) that accepts token, profile,
and an optional repositoryScope, builds the path (including url.QueryEscape for
repositoryScope), performs c.Request("POST", ...), checks resp.StatusCode ==
http.StatusOK, calls c.parseError on non-OK, and unmarshals into a
vendor.ProfileToken; then have OrganizationTokenScoped and OrganizationToken
call that helper and return its result to remove duplicated code and centralize
error/response handling.
In `@internal/vendor/cached.go`:
- Around line 56-59: Avoid recomputing digester.Digest() and ref.String() when
building the cache key in internal/vendor/cached.go: compute digestStr :=
digester.Digest() and refStr := ref.String() once, then build key using
fmt.Sprintf with those variables and include repositoryScope only when non-empty
(e.g., conditional fmt.Sprintf or a single format that appends repositoryScope).
Update the key construction around the existing key variable to use digestStr
and refStr instead of calling the methods twice.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d1fda3c9-f67e-43fe-9a82-28886fbc0a4c
📒 Files selected for processing (27)
api_integration_test.godocs/superpowers/plans/2026-04-15-dynamic-repository-scoping.mddocs/superpowers/specs/2026-04-15-dynamic-repository-scoping-design.mdhandlers.gohandlers_test.goharness_integration_test.gointernal/profile/compilation.gointernal/profile/compilation_test.gointernal/profile/config.gointernal/profile/daemon_test.gointernal/profile/profiles.gointernal/profile/profiles_test.gointernal/profile/profiletest/testdata/profiles.yamlinternal/profile/repositoryscope.gointernal/profile/repositoryscope_test.gointernal/profile/store_test.gointernal/vendor/auditvendor.gointernal/vendor/auditvendor_test.gointernal/vendor/cached.gointernal/vendor/cached_test.gointernal/vendor/orgvendor.gointernal/vendor/orgvendor_test.gointernal/vendor/repovendor.gointernal/vendor/repovendor_test.gointernal/vendor/testhelpers_test.gointernal/vendor/vendor.gotestdata/org-profiles-scoped.yaml
Three fuzz targets covering the untrusted input surfaces: - FuzzValidateRepositories: profile YAML repository entries — tests literal recognition, mixed-entry rejection, slash validation, and consistency between validation and scope resolution. - FuzzExtractRepositoryScope: HTTP query parameter — tests slash rejection, empty/whitespace rejection, encoding edge cases, and absence vs presence semantics. - FuzzResolveRequestScope: vendor-layer scope resolution — tests bidirectional validation across all three profile scope types with adversarial scope values and URLs. Makefile fuzz and ci-fuzz targets updated to include the new packages.
jamestelfer
force-pushed
the
feat/dynamic-repository-scoping
branch
from
ef5d3d3 to
35b51d5
Compare
…umeration GitHub API rejections (e.g. nonexistent repo, insufficient app permissions) previously fell through to a generic 500. This leaks information: callers can distinguish "bad input" (400) from "GitHub refused" (500) and probe for repository existence behind a caller-scoped profile. TokenIssuanceError wraps only CreateInstallationToken failures at the GitHub client boundary. Config validation errors (scopesToPermissions) continue to propagate unwrapped so operator-visible 500s still surface misconfiguration. The error's Status() returns a uniform 403/Forbidden that reaches the HTTP response, while Error() remains descriptive for audit logs and debugging.
Phase 1 of the refactor encoding caller-supplied repository scope into
ProfileRef itself. Foundational only: no production code constructs a
scoped ref yet (handlers still use the legacy repositoryScope parameter
threaded through the vendor chain; that's unwound in later phases).
Encoding scope into the ref lets the cache key derive per-repo entries
naturally via ref.String() and removes the dual-source-of-truth between
the ref and the threaded parameter.
URN formats preserved for non-scoped refs so existing strings roundtrip;
scoped org refs render as /repository/{repo} (long) and /{repo} (short).
Profile type validation and name validation are unchanged in this phase.
Handlers now delegate ref construction to an injected ProfileRefBuilder closure and resolve repository scope at the handler boundary before calling the builder. The new PathValuer interface keeps the builder free of HTTP-type dependencies so handler tests can swap in a simple builder closure instead of constructing a full *http.Request and profile store. No observable behaviour change: the default builder ignores scopedRepo, and the vendor chain still consumes the legacy repositoryScope parameter. Type-aware scope validation and the builder's use of scopedRepo land in the next phase; threading the plumbing first keeps those changes to a single concern.
Phase 2b of the ProfileRef-carried scope refactor. The builder now enforces bidirectional scope rules against profile type: caller-scoped profiles require scope (400 if missing), non-caller-scoped profiles reject explicit scope requests (400). URL-derived implicit scope is silently ignored on non-caller-scoped profiles so git-credentials requests against all-repos profiles continue to work. Deviation from plan: builder takes two scope arguments (explicit, implicit) rather than one, to distinguish caller intent from request format. Rationale documented in handlers.go comments. No observable behaviour change beyond the new rejection statuses. Vendor chain still consumes the legacy repositoryScope parameter; Phase 4 removes it.
Phase 3 of the ProfileRef-carried scope refactor. Remove the repositoryScope branch from cache key derivation. Caller-scoped profiles now distinguish themselves at the cache layer through ref.String() which embeds ScopedRepository (Phase 1). Tests updated to prove the distinctness now comes from the ref: - TestCacheCallerScoped_DifferentReposAreSeparateEntries rewritten to use refs with different ScopedRepository values instead of different repositoryScope parameters. - TestCacheCallerScoped_GitCredentialsPath_DistinctCacheEntries added to cover git-credentials path with distinct scoped refs. - TestCacheCallerScoped_SameScopeIsCacheHit added to verify same scoped ref hits cache on second call. The repositoryScope parameter remains on ProfileTokenVendor for this phase; Phase 4 removes it from the signature. Updated the sequenceVendor test fixture to extract base profile names for scoped refs, allowing proper profile lookup in tests.
Phase 4 of the ProfileRef-carried scope refactor. ProfileTokenVendor is now func(ctx, ref, requestedRepoURL) VendorResult. Scope flows through ref.ScopedRepository from the builder, so the vendor chain no longer needs the redundant parameter. resolveRequestScope simplified: the caller-scoped branch reads the ref directly. Non-caller-scoped branch is a straight passthrough because the builder rejects explicit scope requests at the handler boundary (Phase 2b). Single source of truth, no "both present" bug class. Scope validation happens at the handler boundary in ProfileRefBuilder; the vendor trusts the ref. The builder's contract guarantees that caller-scoped refs carry a non-empty ScopedRepository. Deleted FuzzResolveRequestScope: after simplification the function has no meaningful inputs to fuzz. Boundary inputs like repository-scope query values and URL parsing are covered by existing fuzz tests in extractRepositoryScope, so fuzzing this function adds no value. Vendor-level tests that previously asserted rejection of scope on non-caller-scoped profiles are marked t.Skip with a pointer to the builder (Phase 2b) as the enforcement site; the behaviour is now structurally impossible to reach at the vendor level. No observable change beyond URN suffixes for caller-scoped refs (introduced in Phase 1).
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Caution
Early draft — not yet reviewed. This implementation was generated from the design in #246 and has not been manually reviewed. Treat all decisions as provisional until human review is complete.
Purpose
AI coding agent workflows that run from a central Buildkite pipeline need GitHub access to clone, push, and open PRs against a different repository each time. Today that requires a separate organization profile per target repository — a configuration burden that doesn't scale when the workflow operates across dozens or hundreds of repos.
This PR introduces two new YAML literals for the
repositoriesfield in organization profiles:{{caller-scoped-repository}}— the caller names a single repository at request time; the vended token is narrowed to that repository only.{{all-repositories}}— an unambiguous replacement for the terse*wildcard.The existing
*wildcard is preserved as a deprecated alias with a compile-time warning.Strict bidirectional validation prevents misconfiguration: the bridge rejects a scope parameter sent to a profile that doesn't expect one, and rejects a missing scope for a profile that requires one. When GitHub rejects a scoped token request, the bridge returns a generic 403 to avoid leaking whether a repository exists.
Context
docs/superpowers/specs/2026-04-15-dynamic-repository-scoping-design.mddocs/superpowers/plans/2026-04-15-dynamic-repository-scoping.mdOut of scope for this PR (separate repos/follow-ups):
*in v1 (Req 13.2)Summary by CodeRabbit
Release Notes
New Features
repository-scopequery parameter{{caller-scoped-repository}}and{{all-repositories}}) for dynamic repository targetingBug Fixes
Deprecations
*wildcard repository syntax now triggers deprecation warnings; use{{all-repositories}}instead