Skip to content

feat: multi-installation GitHub App support + images search#346

Open
bencered wants to merge 1 commit intoColeMurray:mainfrom
bencered:feat/multi-installation-and-search
Open

feat: multi-installation GitHub App support + images search#346
bencered wants to merge 1 commit intoColeMurray:mainfrom
bencered:feat/multi-installation-and-search

Conversation

@bencered
Copy link
Copy Markdown
Contributor

Changes

Multi-installation GitHub App support

  • getAllGitHubAppConfigs() parses comma-separated GITHUB_APP_INSTALLATION_ID env var
  • listRepositories() merges and deduplicates repos across all installations
  • checkRepositoryAccess() falls back through installations; throws when ALL fail (vs silent null)
  • Empty installation IDs filtered out (trailing commas, whitespace)

Images settings search

  • Added search input to filter repositories on the images settings page
  • aria-label for accessibility

Tests

  • 37 tests covering parsing edge cases, multi-installation fallback, error surfacing, deduplication

Config

  • terraform.tfvars updated with comma-separated installation IDs (gitignored)

bencered

This comment was marked as resolved.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 14, 2026

Greptile Summary

This PR adds multi-installation GitHub App support by introducing getAllGitHubAppConfigs() to parse comma-separated GITHUB_APP_INSTALLATION_ID values, and updates GitHubSourceControlProvider to fan out repo listing/access checks across all installations with deduplication and graceful degradation. It also adds a search input to the images settings page.

  • Bug: pull-request-service.ts calls generatePushAuth() without the repo owner/name even though session.repo_owner / session.repo_name are available at that point. With multi-installation support, the omitted arguments cause generatePushAuth to always fall back to the primary installation's token — a push to a repo owned by a secondary installation will fail authentication.
  • Minor: this.appConfig is written in the constructor of GitHubSourceControlProvider but never read again; all methods now exclusively use this.allAppConfigs, making the field dead code.
  • The listBranches multi-installation logic is a "first success wins" sequential fallback, which differs from the parallel Promise.allSettled strategy used in listRepositories. This is intentional and correct (branches are repo-specific), but worth noting.
  • The images-settings search filter is straightforward and correctly memoised; the combined owner/name match path adds useful support for slash-separated queries.

Comments Outside Diff (1)

  1. packages/control-plane/src/session/pull-request-service.ts, line 105 (link)

    generatePushAuth called without repo context

    session.repo_owner and session.repo_name are available here (they're used on lines 127–128 and 147–148), but they aren't passed to generatePushAuth(). With multi-installation support, generatePushAuth without arguments always falls back to the primary installation (this.allAppConfigs[0]). If the session's repository belongs to a secondary installation, this will produce a token from the wrong installation, causing the subsequent git push to fail with an authentication error.

Prompt To Fix All With AI
This is a comment left during a code review.
Path: packages/control-plane/src/session/pull-request-service.ts
Line: 105

Comment:
**`generatePushAuth` called without repo context**

`session.repo_owner` and `session.repo_name` are available here (they're used on lines 127–128 and 147–148), but they aren't passed to `generatePushAuth()`. With multi-installation support, `generatePushAuth` without arguments always falls back to the primary installation (`this.allAppConfigs[0]`). If the session's repository belongs to a secondary installation, this will produce a token from the wrong installation, causing the subsequent `git push` to fail with an authentication error.

```suggestion
        pushAuth = await this.deps.sourceControlProvider.generatePushAuth(session.repo_owner, session.repo_name);
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: packages/control-plane/src/source-control/providers/github-provider.ts
Line: 47-52

Comment:
**Unused private field**

`this.appConfig` is assigned in the constructor but never read anywhere in the class body — every method now branches through `this.allAppConfigs` instead. The field can be removed to avoid confusion about whether it still drives any behaviour.

How can I resolve this? If you propose a fix, please make it concise.

Last reviewed commit: a8e354f

bencered

This comment was marked as resolved.

bencered

This comment was marked as resolved.

@bencered bencered force-pushed the feat/multi-installation-and-search branch from 670e4ab to 085082a Compare March 14, 2026 00:34
@bencered bencered marked this pull request as draft March 14, 2026 13:13
@bencered bencered force-pushed the feat/multi-installation-and-search branch from 085082a to d5228d2 Compare March 15, 2026 01:47
- getAllGitHubAppConfigs() parses comma-separated GITHUB_APP_INSTALLATION_ID
- listRepositories() merges and deduplicates repos across all installations
- checkRepositoryAccess() throws when any installation errors (stricter, safer)
- listBranches() and generatePushAuth() iterate all installations to find the owning token
- Images settings: search input to filter repositories (aria-label for a11y)
- deploy-cf.sh: reads secrets from env vars (no hardcoded values)
- 45 tests covering multi-installation fallback, error surfacing, deduplication
@bencered bencered force-pushed the feat/multi-installation-and-search branch from d5228d2 to a8e354f Compare March 15, 2026 01:50
@bencered bencered marked this pull request as ready for review March 15, 2026 02:05
@ColeMurray
Copy link
Copy Markdown
Owner

what's the use-case?

@bencered
Copy link
Copy Markdown
Contributor Author

what's the use-case?

Allows people who work across multiple GitHub organisations (e.g. agencies, contractors) to use a single OpenInspect instance across all of them. Means that they wouldn't have to set up a fresh instance for each org.

@ColeMurray
Copy link
Copy Markdown
Owner

Multi-installation gap analysis

Reviewed the full codebase to understand the blast radius. The PR adds multi-installation parsing and routing on the TypeScript control plane, but several other layers still assume a single installation ID. Here's the complete picture.


1. pull-request-service.ts:105generatePushAuth() called without repo context

The only production call site for generatePushAuth passes no arguments, so it always falls back to allAppConfigs[0]. session.repo_owner / session.repo_name are available and used 21 lines later (lines 127–128). Pushes to repos on a secondary installation will get a token scoped to the wrong org → 403.

// Current (line 105)
pushAuth = await this.deps.sourceControlProvider.generatePushAuth();

// Fix
pushAuth = await this.deps.sourceControlProvider.generatePushAuth(session.repo_owner, session.repo_name);

2. Modal/Python — token generation uses single GITHUB_APP_INSTALLATION_ID

The Python side reads GITHUB_APP_INSTALLATION_ID via os.environ.get() and passes it verbatim to generate_installation_token(), which interpolates it directly into the GitHub API URL:

url = f"https://api.github.com/app/installations/{installation_id}/access_tokens"

If the env var contains comma-separated IDs (e.g. "111,222"), this URL becomes malformed → 404 on every token request. This breaks sandbox spawn, restore, and image builds — not just for secondary installations, but for ALL repos, since the primary installation's token can't be generated either.

Affected call sites (all identical pattern):

  • web_api.py:130 — sandbox spawn
  • web_api.py:511 — sandbox restore
  • functions.py:72 — legacy spawn path
  • scheduler/image_builder.py:129 — repo image builds + cron

The token failure is caught silently (except Exception), so sandboxes start without a clone token. Public repos would still clone; private repos fail silently.

Fix options:

  • (a) Have the control plane resolve the correct installation_id per-repo and pass it to Modal as part of the create/restore request. Modal then uses that specific ID.
  • (b) Parse comma-separated IDs on the Python side too (mirroring getAllGitHubAppConfigs), with the control plane telling Modal which one to use.
  • (c) Pre-generate the token on the control plane and pass it to Modal directly. Less clean since tokens expire in 1 hour and would need refresh.

3. GitHub bot — handlers.ts:127

installationId: env.GITHUB_APP_INSTALLATION_ID,

The github-bot reads the env var directly (not via getGitHubAppConfig()) and passes it as-is to generateInstallationToken(). With comma-separated IDs, this produces a malformed GitHub API URL → the bot can't generate tokens for any webhook handling (PR review assignments, @mention commands).


4. Terraform — variables.tf:87

variable "github_app_installation_id" { type = string }

The single string variable propagates unchanged to all three services (control-plane worker, github-bot worker, Modal secret). The TypeScript control plane now correctly parses comma-separated values, but Modal and github-bot do not. There's no separate variable or per-service splitting.


5. Image builder cron — no per-repo installation routing

scheduler/image_builder.py rebuilds repo images on a cron schedule. It calls _generate_clone_token() which uses the single GITHUB_APP_INSTALLATION_ID. Even if the comma-separated parsing were fixed, there's no mechanism to route different repos to different installations during image builds — all repos must be cloneable with the first installation's token.


Summary

Layer Issue Severity
pull-request-service.ts generatePushAuth() called without repo context Bug — PR pushes to secondary-installation repos fail
Python/Modal (all token sites) Comma-separated ID passed verbatim to GitHub API URL Breaking — malformed URL, no tokens generated for any repo
github-bot/handlers.ts Reads env var directly, not via parser Breaking — bot webhook handling fails entirely
Terraform Single string var, no per-service splitting Config gap
Image builder cron No per-repo installation routing Feature gap

The TypeScript control plane changes are well-structured. The core issue is that setting GITHUB_APP_INSTALLATION_ID to comma-separated values will break the Python/Modal and github-bot layers before multi-installation routing can even be exercised. These layers need corresponding changes to parse (or receive) the correct installation ID per-repo.

@ColeMurray
Copy link
Copy Markdown
Owner

based on the gap analysis, i'm wondering if it makes sense to support this. While I do see the convenience of having multiple appIds, it adds a bit of oddities where pretty much any call to interact with GH will be required to loop multiple appIds, all but one of which will fail. Open to suggestions, but my thoughts are it is probably better to deploy multiple instances of the control-plane.

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