Skip to content

Fix null repos handling for all-repos installations - #180

Merged
hsyntax merged 8 commits into
mainfrom
github-app-part-2
Mar 12, 2026
Merged

Fix null repos handling for all-repos installations#180
hsyntax merged 8 commits into
mainfrom
github-app-part-2

Conversation

@hsyntax

@hsyntax hsyntax commented Mar 12, 2026

Copy link
Copy Markdown
Owner

Summary

  • Preserve null semantics in installations.repos when a GitHub App is installed on all repos (previously null was coerced to [] via ?? [], breaking repos IS NULL queries)
  • Propagate null through installation_repositories incremental merge (all-repos installations stay null)
  • Update UpsertInstallationRequest schema to accept NullOr(Array(RepoEntry))
  • Add integration test for all-repos installation case
  • Update spec to reflect actual implementation (incremental merge, object format, hard delete)

Test plan

  • New test: installation event with all repos → repos is null in DB
  • All 25 existing integration tests still pass
  • pnpm test:integration green

Implements specs/github-app/2-installations.md

hsyntax and others added 3 commits March 11, 2026 22:44
Preserve null semantics in the repos column when a GitHub App is installed
on all repos in an account. Previously payload.repositories was coerced
from null to [] via ?? [], breaking repos IS NULL queries.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

@hsyntax hsyntax left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: 2 finding(s) across the changes.

Comment thread packages/server/src/github/webhook-handler.ts
Comment thread packages/server/src/github/webhook-handler.ts
@hsyntax

hsyntax commented Mar 12, 2026

Copy link
Copy Markdown
Owner Author

This PR is not merge-ready yet because repo-scope transitions can remain overbroad and the new integration tests do not match GitHub's documented installation_repositories payload shape.

  • Verdict: ❌ needs_fix
  • Findings: 3 total (🔴 critical: 0, 🟡 warning: 3, 🔵 info: 0)

Category Breakdown

Category Count
authorization 1
quality 1
state-management 1

@hsyntax hsyntax left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: 3 finding(s) across the changes.


let repos: ReadonlyArray<{ full_name: string }> | null
if (eventType === "installation_repositories") {
const current = yield* store.getInstallationRepos(installationId)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 WARNING (authorization)

installation-repo-state-can-stay-overbroadinstallation_repositories updates currently treat both a missing installation row and an existing repos = null row as terminal states. That breaks legitimate state transitions: if the initial installation event was missed or arrives out of order, the first repo delta is dropped; and if an installation narrows from all repositories to selected repositories, repos can remain NULL, which this codebase treats as full access and can overgrant authorization after access has been revoked.

Suggested fixes
  1. Track deletion separately from absence so installation_repositories events can rebuild state for installations that are missing but not known-deleted.
  2. When repository_selection is selected and the current row is missing or has repos = null, replace repos with an authoritative selected-repo list fetched from GitHub instead of returning early or preserving null.
  3. If an inline resync is not possible, fail closed by marking the installation as needing resync and stop treating repos IS NULL as authoritative until the repo list is refreshed.

}
} else {
repos = current
repos = payload.repositories ?? null

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 WARNING (state-management)

installation-events-can-clobber-repo-scope — Non-deleted installation actions now write repos = payload.repositories ?? null, but GitHub's installation payload does not always include repositories. For actions like suspend, unsuspend, or new_permissions_accepted, an omitted field will rewrite a selected-repo installation to NULL, incorrectly broadening it to 'all repositories'.

Suggested fixes
  1. Only derive repos from payload.repositories when the payload explicitly includes repository data, and preserve the stored value otherwise.
  2. For non-deleted installation actions, load the current installation first and fall back to its existing repo set unless the payload explicitly indicates an all-repos installation.
  3. Limit full repo replacement behavior to actions whose payload shape is known to carry authoritative repository scope, such as installation.created.

@@ -399,6 +477,63 @@ describe("webhook receiver", () => {
{ timeout: 60_000 },

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 WARNING (quality)

installation-repositories-tests-use-invalid-payloads — The new installation_repositories integration tests omit repositories_added and repositories_removed, even though GitHub documents both arrays as required for this event. That weakens coverage because the tests validate handler behavior against a synthetic payload shape rather than real deliveries.

Suggested fixes
  1. Add repositories_added: [] and repositories_removed: [] to both new installation_repositories test payloads.
  2. Extract a shared helper or fixture for installation_repositories payloads so every test includes the documented required arrays and repository_selection consistently.

Note

This finding applies to line 314 but is shown here because that line is not in the diff.

@hsyntax
hsyntax merged commit 7bf011f into main Mar 12, 2026
8 checks passed
@hsyntax
hsyntax deleted the github-app-part-2 branch March 12, 2026 06:24
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.

1 participant