feat(github): associate GitHub installation id with a team#2113
Merged
whutchinson98 merged 4 commits intomainfrom Mar 23, 2026
Conversation
Contributor
WalkthroughAdds GitHub App installation handling: a new Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust/cloud-storage/github/src/domain/service/sync/handle_installation.rs`:
- Around line 29-33: The logs currently emit raw PII and user identifiers (e.g.,
macro_id values like "macro|user@user.com" and sender_github_user_id) via
tracing::info/tracing::warn calls; update each logging call (the
tracing::info/tracing::warn invocations around the installation event handler)
to remove macro_id and sender_github_user_id and instead log only
installation_id plus non‑PII operational fields such as team_count (or other
aggregate counts) for visibility; apply the same change to the other occurrences
noted (the similar tracing calls at the other locations in this handler).
In `@rust/cloud-storage/github/src/domain/service/sync/test.rs`:
- Around line 1253-1260: The test installation_created_no_github_link currently
only checks for Ok(()) but must also assert that no inserts happened; after
calling service.process_webhook_event(&event).await.unwrap(), add an assertion
that service.repo.installation_associations().is_empty() (or equivalent
accessor) to ensure insert_installation_team_associations was not called; apply
the same additional assertion to the corresponding
installation_deleted_no_github_link test so both verify the skip path leaves
installation_associations empty.
- Line 203: The test stub's installation_associations currently stores only
(String, Vec<Uuid>) and drops the installed_by actor; change its type to include
the installer (e.g., Mutex<Vec<(String, Vec<uuid::Uuid>, uuid::Uuid)>> or
Option<uuid::Uuid>) and update every place that constructs or inspects
installation_associations (the stub field name installation_associations and the
tests like installation_created_associates_teams) to record and assert the
installed_by value passed by pg_github_sync_repo::persist (the installed_by
parameter) so the tests validate the actor is persisted and caught if wrong.
In `@rust/cloud-storage/github/src/outbound/pg_github_sync_repo.rs`:
- Line 110: The tracing spans currently capture PII-bearing parameters; update
each #[tracing::instrument(skip(self), err)] attribute in this file to skip the
sensitive params by name (e.g. use #[tracing::instrument(skip(self,
github_user_id, macro_id, team_ids, installed_by), err)]), ensuring
github_user_id, macro_id, team_ids, and installed_by are not emitted as span
fields; apply the same change to the other instrument attributes in this file
that currently only skip(self).
In
`@rust/cloud-storage/macro_db_client/migrations/20260323152227_github_app_installation_id_team_association.sql`:
- Around line 2-8: Rename the snake_case columns in the new table
github_app_installation_team to camelCase: change team_id → teamId and
installed_by → installedBy in the CREATE TABLE definition (keeping id and
PRIMARY KEY as-is). Then update any Rust queries in pg_github_sync_repo.rs that
reference these columns to select the new names (or alias them back to
snake_case if the code expects snake_case, e.g., `"teamId" as "team_id"`), and
regenerate the SQLx cache files so the query macros reflect the schema change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f5af155f-5176-4bf9-938d-767ff1e1b799
📒 Files selected for processing (23)
rust/cloud-storage/github/.sqlx/query-28c518d844cd4d843c097af1551f6f384a4763514d278b5444341390f58a8281.jsonrust/cloud-storage/github/.sqlx/query-37b5168f739c490a3373c65eab35f247a34af0a8e659cbad61cb99e3094e5a60.jsonrust/cloud-storage/github/.sqlx/query-3a7be99469ed150e0506efa786b672198a6d3419c6bd5548418520cd3e6a8e95.jsonrust/cloud-storage/github/.sqlx/query-4afa79ca95bdbcebe9d3de7ebb204636419e6cf89e650a8716c40ab2e588aa5b.jsonrust/cloud-storage/github/.sqlx/query-4c695b011d49ba7d5fce4b309875ed952dc386ac760444d7883c5a76718f15be.jsonrust/cloud-storage/github/.sqlx/query-4c8f1c99fe7ca583e05b4ef8aba08d4fdf58135dc3e99ed73381a29f94b8a270.jsonrust/cloud-storage/github/.sqlx/query-54e3755d63a676a7fa70da77321e618032f0130c73eb6b0ae985f07c5c6c448b.jsonrust/cloud-storage/github/.sqlx/query-6a83d04482526e2ceb9e9eb812d3ceaf1c8c7fafb25ba1cbecb4dadc543fad67.jsonrust/cloud-storage/github/.sqlx/query-7617f808df94ffe77c8195a911ee6b230a21fe9935e099d1310d244248ca253a.jsonrust/cloud-storage/github/.sqlx/query-79d3bc72e6f424765706f7deea426379ae49cbd70dd122daedd95343618b9ac6.jsonrust/cloud-storage/github/.sqlx/query-96d853b887f2e6d7aac6b15ddd2b71a8d8900d2ef30047da7ca1a5940503b6d0.jsonrust/cloud-storage/github/.sqlx/query-b3aeddd507e0efee0e0cecef6eae7503820f0f3302e95a459790c5cb78e5dfb0.jsonrust/cloud-storage/github/.sqlx/query-e7235d6ad2033a2ff7be0bf57ab042cd71e3b3d0e1f8a9750e55cdda92e70c6f.jsonrust/cloud-storage/github/.sqlx/query-ed43958d3b45528df34f3216299e47e00e2aa671b9d235abffa4ad8bfe126c12.jsonrust/cloud-storage/github/fixtures/github_installation_test_data.sqlrust/cloud-storage/github/src/domain/models/sync.rsrust/cloud-storage/github/src/domain/ports/sync.rsrust/cloud-storage/github/src/domain/service/sync/handle_installation.rsrust/cloud-storage/github/src/domain/service/sync/mod.rsrust/cloud-storage/github/src/domain/service/sync/test.rsrust/cloud-storage/github/src/outbound/pg_github_sync_repo.rsrust/cloud-storage/github/src/outbound/pg_github_sync_repo/test.rsrust/cloud-storage/macro_db_client/migrations/20260323152227_github_app_installation_id_team_association.sql
💤 Files with no reviewable changes (11)
- rust/cloud-storage/github/.sqlx/query-54e3755d63a676a7fa70da77321e618032f0130c73eb6b0ae985f07c5c6c448b.json
- rust/cloud-storage/github/.sqlx/query-28c518d844cd4d843c097af1551f6f384a4763514d278b5444341390f58a8281.json
- rust/cloud-storage/github/.sqlx/query-96d853b887f2e6d7aac6b15ddd2b71a8d8900d2ef30047da7ca1a5940503b6d0.json
- rust/cloud-storage/github/.sqlx/query-37b5168f739c490a3373c65eab35f247a34af0a8e659cbad61cb99e3094e5a60.json
- rust/cloud-storage/github/.sqlx/query-7617f808df94ffe77c8195a911ee6b230a21fe9935e099d1310d244248ca253a.json
- rust/cloud-storage/github/.sqlx/query-4afa79ca95bdbcebe9d3de7ebb204636419e6cf89e650a8716c40ab2e588aa5b.json
- rust/cloud-storage/github/.sqlx/query-4c695b011d49ba7d5fce4b309875ed952dc386ac760444d7883c5a76718f15be.json
- rust/cloud-storage/github/.sqlx/query-3a7be99469ed150e0506efa786b672198a6d3419c6bd5548418520cd3e6a8e95.json
- rust/cloud-storage/github/.sqlx/query-e7235d6ad2033a2ff7be0bf57ab042cd71e3b3d0e1f8a9750e55cdda92e70c6f.json
- rust/cloud-storage/github/.sqlx/query-79d3bc72e6f424765706f7deea426379ae49cbd70dd122daedd95343618b9ac6.json
- rust/cloud-storage/github/.sqlx/query-b3aeddd507e0efee0e0cecef6eae7503820f0f3302e95a459790c5cb78e5dfb0.json
rust/cloud-storage/github/src/domain/service/sync/handle_installation.rs
Outdated
Show resolved
Hide resolved
...ge/macro_db_client/migrations/20260323152227_github_app_installation_id_team_association.sql
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust/cloud-storage/github/src/domain/service/sync/handle_installation.rs`:
- Around line 37-59: The handler currently returns early when no GitHub link
(macro_id is None) or when team_ids is empty, which drops installs; modify the
logic in handle_installation (the block using macro_id, installation_id and the
call to self.repo.get_user_team_ids) to persist a pending installation record
instead of returning Ok(()). Specifically, when macro_id is None create/update a
pending record tied to installation_id indicating "awaiting_github_link", and
when team_ids.is_empty() create/update a pending record indicating
"awaiting_team_membership" (use your existing repo methods or add new repo
methods to insert into the github_app_installation_team / pending installation
table and expose reconciliation/backfill paths), ensuring errors map to
GithubError::Internal as before.
In `@rust/cloud-storage/github/src/domain/service/sync/test.rs`:
- Around line 202-203: The test stub currently records installation_associations
as a Vec<(String, Vec<uuid::Uuid>, String)>, which logs whole batches and
diverges from production's per-(installation_id, team_id) insert with ON
CONFLICT DO NOTHING; change the stub to record individual rows (installation_id,
team_id, installed_by) — e.g., use a Mutex<HashSet<(String, uuid::Uuid,
String)>> or dedupe on each insert — and update the stubbed insert method (the
code that pushes into installation_associations in this test module) to insert
one (installation_id, team_id, installed_by) tuple per team_id and ignore
duplicates so retry/redelivery tests match pg_github_sync_repo.rs semantics.
- Around line 1280-1295: The test installation_deleted_is_skipped currently
expects a no-op but the DB keeps orphaned rows because
github_app_installation_team only INSERTs; implement a new
Service::handle_installation_deleted(&self, installation_id: i64) that removes
associations for that installation (delete from github_app_installation_team by
installation_id and update any repo.installation_associations cache), call it
from Service::process_webhook_event when event.action == "deleted" for
"installation" events, and update the test to assert the association is removed
by checking service.repo.installation_associations().is_empty() after
processing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a456cb2c-39ce-45b7-9013-dda26824434e
📒 Files selected for processing (3)
rust/cloud-storage/CLAUDE.mdrust/cloud-storage/github/src/domain/service/sync/handle_installation.rsrust/cloud-storage/github/src/domain/service/sync/test.rs
Comment on lines
+37
to
+59
| let macro_id = match macro_id { | ||
| Some(id) => id, | ||
| None => { | ||
| tracing::warn!( | ||
| installation_id, | ||
| "no github link found for sender, cannot associate installation with teams" | ||
| ); | ||
| return Ok(()); | ||
| } | ||
| }; | ||
|
|
||
| let team_ids = self | ||
| .repo | ||
| .get_user_team_ids(¯o_id) | ||
| .await | ||
| .map_err(|e| GithubError::Internal(e.into()))?; | ||
|
|
||
| if team_ids.is_empty() { | ||
| tracing::info!( | ||
| installation_id, | ||
| "user has no teams, skipping installation association" | ||
| ); | ||
| return Ok(()); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Call sites of the installation-association repo methods:"
rg -n -C2 '\b(get_macro_id_by_github_user_id|get_user_team_ids|insert_installation_team_associations)\s*\(' rust/cloud-storage
echo
echo "All direct references to the installation-team table:"
rg -n -C2 'github_app_installation_team' rust/cloud-storageRepository: macro-inc/macro
Length of output: 12365
Don't silently drop installs without a GitHub link or teams.
When no GitHub link is found (line 44) or the user has no teams (line 59), the handler returns Ok(()) without persisting any record of the installation. These repo methods and the github_app_installation_team table are only called/accessed from this handler. If a user later links GitHub or joins a team, there is no mechanism to backfill the association unless they reinstall the app.
Store a pending installation record or implement a reconciliation path instead of returning early.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/github/src/domain/service/sync/handle_installation.rs`
around lines 37 - 59, The handler currently returns early when no GitHub link
(macro_id is None) or when team_ids is empty, which drops installs; modify the
logic in handle_installation (the block using macro_id, installation_id and the
call to self.repo.get_user_team_ids) to persist a pending installation record
instead of returning Ok(()). Specifically, when macro_id is None create/update a
pending record tied to installation_id indicating "awaiting_github_link", and
when team_ids.is_empty() create/update a pending record indicating
"awaiting_team_membership" (use your existing repo methods or add new repo
methods to insert into the github_app_installation_team / pending installation
table and expose reconciliation/backfill paths), ensuring errors map to
GithubError::Internal as before.
whutchinson98
deleted the
hutch/macro-cyeBELYbDapcNFedFoUEJ-associate-github-installation-id-with-a-team
branch
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.
No description provided.