Repository navigation
Let several LiveReview accounts share one GitHub App installation, plus /gh-login and link-preview fixes - #139
Conversation
Two people from the same GitHub organization can now each sign up with GitHub: the second person's org gets its own connector for the installation instead of "already connected to other LiveReview accounts". - One connector per (installation, org). An org gets every repository the installation covers only if the person who connected it can access all of them; otherwise just the repositories they can access (GitHub reports both lists, so a hand-edited callback URL gains nothing). The list is refreshed on every GitHub sign-in. - Webhooks fan out: installation and repository events reach every org; PR updates reach every org that has the repository, and only the oldest connector writes the PR description links; comment replies come from the org that reviewed the PR most recently. - "Start a review" and "Review history" links open the user's own org's copy of the PR. - Git Providers / get-started: separate Disconnect (this org only) and Uninstall (removes the App from GitHub for every org, with a warning saying how many other accounts use it). Notes when the App was installed by someone else. - Fix: a failed /gh-login no longer leaves a get-started redirect behind; the callback sets the landing page only on success.
"Also used by N other LiveReview accounts" on the GitHub App card and get-started is now a link: it opens a popup listing those accounts' owners by masked email (first two letters and the domain, e.g. at•••@gmail.com). The server masks them; full addresses are never sent.
Shared LiveReview links (e.g. /gh-login?src=launch-trial) previewed as "LiveReview - A powerful tool for automated code reviews" with the React boilerplate's og:url and a map-collage thumbnail. - Title and description match the launch messaging. - Open Graph tags use the property attribute (Facebook and LinkedIn ignore name="og:..."), plus og:type, og:site_name and image size/alt; Twitter card tags too. - New 1200x630 og-image.png replaces the boilerplate thumbnail.jpg and the SVG og:image social sites don't render. - Cloud builds: og:url and canonical https://livereview.hexmos.com, indexable. Self-hosted builds: noindex, no canonical.
The three points under the headline were 12px; they are now 16px with larger check marks and a little more spacing.
The small "Other ways to sign in" link with 14px icons is now an "Or sign in with" row of 44px Google, LinkedIn and Email tiles in their brand colors, each clickable. All of them open the normal Hexmos sign-in, as before.
The Google, LinkedIn and Email tiles went to the LiveReview login page first, which then needed another click. They now go straight to hexmos.com/signin?app=livereview and come back to the app root. The sign-in URL is shared with the Cloud login page (utils/hexmosSignin).
lovestaco
changed the title
Add Multi-Org GitHub App Installations with Repo ScopingOverviewThis change lets many LiveReview orgs share one GitHub App installation. It scopes each org's repository access to the acting GitHub user's access. It adds disconnect vs uninstall paths and routes shared PR events across orgs. Technical Highlights
Impact
|
![livereview-agent-local[bot]](https://avatars.githubusercontent.com/u/68785089?s=60&v=4){kind=small}
| return nil, false | ||
| } | ||
| allowed = map[string]bool{} | ||
| ids, _ := connector.Metadata["allowed_repo_ids"].([]interface{}) |
There was a problem hiding this comment.
Severity: critical
Silent type assertion on allowed_repo_ids. If metadata stores ids as []string/[]int64, or the key is absent, allowed stays empty while scoped=true. Every repo is then skipped and pruned, deleting all connector repositories.
Suggestions:
- Validate the assertion and return scoped=false (or an error) when allowed_repo_ids is missing or not []interface{}.
- Treat an empty allowed set as 'not scoped' rather than 'nothing allowed' to avoid mass deletion.
| for id := range allowed { | ||
| keep = append(keep, id) | ||
| } | ||
| if err := store.DeleteRepositoriesByConnectorExcept(int64(connectorID), keep); err != nil { |
There was a problem hiding this comment.
Severity: critical
DeleteRepositoriesByConnectorExcept with an empty keep list deletes every repository for the connector. This is destructive and runs whenever allowed parses to empty.
Suggestions:
- Skip the prune call when keep is empty, or guard it behind a positive allowed count.
- Log the number of pruned repositories for auditability.
| log.Printf("[GitHubApp] disconnect connector %d (org %d) failed: %v", connectorID, orgID, err) | ||
| return c.JSON(http.StatusInternalServerError, map[string]string{"error": "failed to disconnect"}) | ||
| } | ||
| if err := repos.DeleteRepositoriesByConnector(connectorID); err != nil { |
There was a problem hiding this comment.
Severity: warning
DisconnectAppConnector runs before DeleteRepositoriesByConnector. If repo deletion fails, the connector is already marked removed but repositories remain; a retry may 404 and never clean them.
Suggestions:
- Delete repositories first, then mark the connector removed, or make the operation idempotent/retryable.
- Ensure GetGitHubTokenByConnectorID still returns removed connectors so retries can finish cleanup.
| return c.JSON(http.StatusInternalServerError, map[string]string{"error": "failed to disconnect"}) | ||
| yes := true | ||
| for _, r := range recs { | ||
| if err := store.SetAppConnectorFlags(r.ID, &yes, nil); err != nil { |
There was a problem hiding this comment.
Severity: warning
Loop removes connectors one by one after the GitHub uninstall. A mid-loop failure leaves earlier connectors removed and later ones orphaned, pointing at an App that no longer exists.
Suggestions:
- Wrap the per-connector cleanup in a transaction or continue-on-error with a summary, so all connectors are attempted.
- Log which connectors were left unprocessed so they can be reconciled.
| yes := true | ||
| if err := store.SetAppConnectorFlags(connectorID, &yes, nil); err != nil { | ||
| log.Printf("[GitHubApp] disconnect connector %d (org %d) failed: %v", connectorID, orgID, err) | ||
| recs, err := store.ListAppConnectorsByInstallation(cfg.BaseURL, meta.InstallationID) |
There was a problem hiding this comment.
Severity: warning
?uninstall=true lets one org owner uninstall the App from GitHub, removing every other org's connector for that installation. Cross-tenant impact without other orgs' consent.
Suggestions:
- Confirm this cross-org blast radius is intended; consider restricting uninstall to a single-org installation or requiring explicit confirmation.
- Surface the number of affected orgs to the caller before uninstalling.
| if err != nil { | ||
| log.Printf("[GitHubApp] installation %q event failed for connector %d: %v", env.Action, rec.ID, err) | ||
| return c.JSON(http.StatusInternalServerError, map[string]string{"error": "internal_error"}) | ||
| if env.Issue != nil && strings.Contains(env.Issue.HTMLURL, "/pull/") { |
There was a problem hiding this comment.
Severity: warning
Detecting a PR via strings.Contains(htmlURL, "/pull/") is fragile. A repo literally named "pull" yields an issue URL like /owner/pull/issues/1 that matches, misrouting the event to the wrong org.
Suggestions:
- Prefer the explicit pull_request field when present, and only fall back to the issue URL.
- If parsing the URL, match the path segment structure (e.g. /pull/<number>) instead of a substring.
| } | ||
| // The other orgs only need their copy of the PR list updated. | ||
| if event == "pull_request" && len(targets) > 1 { | ||
| if err := s.queueSharedPullRequestState(c, body, targets[1:]); err != nil { |
There was a problem hiding this comment.
Severity: warning
A failure queuing shared PR state for secondary orgs returns 500 before the primary org's event is processed. One bad secondary connector blocks the main pipeline.
Suggestions:
- Process the primary event first, then queue shared state for the others.
- Log and continue on secondary-org queue failures instead of aborting the whole webhook.
| case "unsuspend": | ||
| err = store.SetAppConnectorFlags(rec.ID, nil, &no) | ||
| } | ||
| if err != nil { |
There was a problem hiding this comment.
Severity: warning
handleGitHubAppInstallationEvent returns 500 on the first connector error, leaving later connectors unprocessed. State becomes partially applied until GitHub retries.
Suggestions:
- Attempt all connectors, collect errors, and return 500 only if any failed.
- Ensure the operation is idempotent so retries converge cleanly.
| if meta.RepoScope == storagegithub.RepoScopeUser { | ||
| scope = storagegithub.RepoScopeUser | ||
| } | ||
| others, err := store.CountOtherOrgsUsingInstallation(cfg.BaseURL, meta.InstallationID, orgID) |
There was a problem hiding this comment.
Severity: warning
Two DB queries per connector (CountOtherOrgsUsingInstallation and OtherOrgOwnerEmailsForInstallation) inside the loop create an N+1 pattern on the demo endpoint.
Suggestions:
- Batch the counts and owner emails for all installation IDs in one query.
- Or fetch other-org data once per installation rather than per connector.
|
|
||
| // One org handles the event through the normal pipeline: the oldest connector, or for comments the org | ||
| // that reviewed this PR most recently (so replies come from that org and its AI provider). | ||
| target := targets[0] |
There was a problem hiding this comment.
Severity: warning
target := targets[0] relies on ListAppConnectorsByInstallation returning oldest-first. If the query does not guarantee ordering, the wrong org handles the event.
Suggestions:
- Confirm the storage query orders by created_at/ID ascending.
- If ordering is not guaranteed, sort targets explicitly before selecting.
| var ids []int64 | ||
| cursor := "" | ||
| for page := 0; ; page++ { | ||
| if page == 100 { |
There was a problem hiding this comment.
Severity: info
The 100-page cap maps to a repo count that depends on the page size used by ListInstallationRepositories; the error text asserts 10000 repos.
Suggestions:
- Derive the limit from the actual page size, or reword the error to state the page cap.
- Extract the magic number into a named constant.
|
|
||
| // maskEmail keeps the first two characters of the name and the domain: "athreyac4@gmail.com" -> "at•••@gmail.com" | ||
| // (one character when the name is that short). | ||
| func maskEmail(email string) string { |
There was a problem hiding this comment.
Severity: warning
maskEmail is defined but not referenced anywhere in this diff. Unused helpers become dead code and untested surface.
Suggestions:
- Confirm maskEmail is called from the email display/log path; if not, remove it.
- If it is used, add a unit test covering short local parts and missing '@'.
| if err == sql.ErrNoRows { | ||
| // Orgs sharing a GitHub App installation each keep their own copy of the PR, and the link names one of | ||
| // them: open the copy in an org the user belongs to. | ||
| orgID, resolvedID, err = pullrequests.NewStore(s.db).SiblingPullRequestForUser(prID, user.ID) |
There was a problem hiding this comment.
Severity: warning
This fallback resolves a PR to another org's copy. SiblingPullRequestForUser must verify the user is a member of that org; matching only by shared PR identity would leak another org's PR.
Suggestions:
- Confirm SiblingPullRequestForUser filters by user membership/role, not just PR identity.
- Add a test where the user is not a member of the sibling org and expect 404.
| return c.JSON(http.StatusInternalServerError, ErrorResponse{Error: "failed to look up pull request"}) | ||
| } | ||
| return JSONWithEnvelope(c, http.StatusOK, map[string]interface{}{"org_id": orgID}) | ||
| return JSONWithEnvelope(c, http.StatusOK, map[string]interface{}{"org_id": orgID, "pull_request_id": resolvedID}) |
There was a problem hiding this comment.
Severity: warning
Response now returns pull_request_id. Clients that only read org_id will keep using the original PR id, which may belong to a different org.
Suggestions:
- Update the frontend to consume pull_request_id from this response.
- Document the new field in the API contract to prevent stale client usage.
| } | ||
|
|
||
| if action, _ := event.Metadata["action"].(string); action == "opened" || action == "open" { | ||
| if action, _ := event.Metadata["action"].(string); !args.SkipLinks && (action == "opened" || action == "open") { |
There was a problem hiding this comment.
Severity: warning
SkipLinks silently drops the PR description links comment. If the caller ever queues all connectors for a shared installation with SkipLinks=true, no links are posted and nothing errors.
Suggestions:
- Guarantee the caller always enqueues exactly one non-skipping job per PR (e.g. the lowest-ID/primary connector).
- Add a test asserting a shared-installation PR still gets exactly one links comment.
| t.Fatalf("first org's connector changed: %+v", rec) | ||
| } | ||
| recs, err := store.ListAppConnectorsByInstallation("https://ghes.test", inst) | ||
| if err != nil || len(recs) != 2 || recs[0].ID != id || recs[1].ID != idB { |
There was a problem hiding this comment.
Severity: warning
Test asserts recs[0]/recs[1] order from ListAppConnectorsByInstallation. If the query has no deterministic ORDER BY, this becomes flaky.
Suggestions:
- Assert the returned set by ID membership rather than positional order, or document/guarantee ORDER BY id in the store method.
| var id int64 | ||
| err := s.db.QueryRow(` | ||
| SELECT connector_id FROM reviews | ||
| WHERE connector_id = ANY($1) AND (pr_mr_url = $2 OR pr_mr_url LIKE $2 || '/%') |
There was a problem hiding this comment.
Severity: warning
pr_mr_url LIKE $2 || '/%' treats % and _ in prURL as wildcards. A pasted URL containing these characters over-matches unrelated PRs and can select the wrong connector.
Suggestions:
- Escape LIKE metacharacters in prURL before building the pattern
- Or use
LIKE ... ESCAPE '\'with an escaped pattern
| err := s.db.QueryRow(` | ||
| SELECT COUNT(*) FROM integration_tokens | ||
| WHERE token_type = $1 AND provider_url = $2 AND provider_app_id = $3 AND org_id <> $4 | ||
| AND COALESCE(metadata->>'removed', 'false') <> 'true' |
There was a problem hiding this comment.
Severity: warning
Counts suspended connectors as active users. HasAppConnectorForOwnerAllRepos excludes suspended; this query only excludes removed, so the two disagree.
Suggestions:
- Add
AND COALESCE(metadata->>'suspended','false') <> 'true'to match the live-connector definition used elsewhere
| // for the installation, sorted and without duplicates. Callers show them masked only. | ||
| func (s *TokenStore) OtherOrgOwnerEmailsForInstallation(baseURL string, installationID, orgID int64) ([]string, error) { | ||
| rows, err := s.db.Query(` | ||
| SELECT DISTINCT LOWER(u.email) |
There was a problem hiding this comment.
Severity: warning
Returns raw owner emails of other orgs. A caller that forgets to mask leaks PII across org boundaries.
Suggestions:
- Mask emails in SQL (e.g. keep only domain or first char) instead of relying on callers
- Or return a non-PII identifier and resolve display separately
| Disconnected bool `json:"disconnected"` | ||
| } | ||
| _ = json.Unmarshal(existingMeta, &prev) | ||
| if in.KeepDisconnected && prev.Removed && prev.Disconnected { |
There was a problem hiding this comment.
Severity: warning
KeepDisconnected defaults to false, so any caller that omits it reconnects a disconnected connector. Webhook or other callers may silently resurrect it.
Suggestions:
- Invert the flag to an explicit Reconnect so the zero value keeps the disconnected state
- Or require every non-install caller to set KeepDisconnected explicitly
| // DeleteRepositoriesByConnectorExcept removes the connector's repositories whose provider id isn't in keep (a | ||
| // user-scoped GitHub App connector whose user lost access to some). | ||
| func (s *Store) DeleteRepositoriesByConnectorExcept(connectorID int64, keep []string) error { | ||
| if keep == nil { |
There was a problem hiding this comment.
Severity: warning
Converting nil keep to an empty slice makes an empty/nil keep delete every repository for the connector. A caller passing an empty slice after a failed fetch silently wipes all repos.
Suggestions:
- Guard against an empty keep list (return an error) unless delete-all is explicitly intended.
- Document the destructive empty-input behavior on the method and at call sites.
| FROM pull_requests pr | ||
| JOIN repositories r ON r.id = pr.repository_id | ||
| JOIN integration_tokens t ON t.id = r.connector_id | ||
| JOIN pull_requests sib ON sib.provider = pr.provider AND sib.provider_pr_id = pr.provider_pr_id AND sib.id <> pr.id |
There was a problem hiding this comment.
Severity: warning
The sibling join does not exclude a sibling in the same org as the source PR. If an org has two connectors syncing the same PR, this returns a same-org copy, violating the 'another org' contract.
Suggestions:
- Add
AND sib.org_id <> pr.org_idto the join or WHERE clause. - Add a test covering two connectors in the same org syncing the same PR.
| */ | ||
| 'og:title': title, | ||
| 'og:description': description, | ||
| 'og:url': homepage, |
There was a problem hiding this comment.
Severity: warning
module.exports changed from an object to a function. Any caller still doing require('./meta.config.js') and spreading it will get undefined title/meta and emit empty tags.
Suggestions:
- Update the webpack config to call buildMetaConfig(isCloud) and pass siteURL into html-webpack-plugin options.
- Verify the build emits title, og:*, and canonical tags for both cloud and self-hosted.
| */ | ||
| export const disconnectGitHubInstallation = (connectorId: number): Promise<{ status: string }> => | ||
| apiClient.delete<{ status: string }>(`/api/v1/github-app/installations/${connectorId}`); | ||
| export const disconnectGitHubInstallation = (connectorId: number, uninstall = false): Promise<{ status: string }> => |
There was a problem hiding this comment.
Severity: warning
uninstall=true uninstalls the App from GitHub for every account using the installation, not just the current org. Callers must confirm this cross-tenant destructive action before invoking.
Suggestions:
- Require an explicit confirmation dialog before sending uninstall=true.
- Surface other_orgs/other_accounts in the confirmation so users see the blast radius.
| const OtherAccountsModal: React.FC<{ inst: GitHubAppInstallation; onClose: () => void }> = ({ inst, onClose }) => { | ||
| const unnamed = inst.other_orgs - inst.other_accounts.length; | ||
| return createPortal( | ||
| <div className="fixed inset-0 z-[9999] flex items-center justify-center p-4"> |
There was a problem hiding this comment.
Severity: warning
The modal has no dialog semantics or keyboard handling. Screen readers won't announce it and Escape won't close it.
Suggestions:
- Add role="dialog" and aria-modal="true" to the container
- Close on Escape via a keydown listener
- Move focus into the modal on open and restore it on close
| </p> | ||
| <ul className="mt-4 space-y-2"> | ||
| {inst.other_accounts.map((email) => ( | ||
| <li key={email} className="flex items-center gap-2 text-sm text-slate-200"> |
There was a problem hiding this comment.
Severity: warning
Using the masked email as the React key can collide when two accounts share the same masked address, causing reconciliation bugs.
Suggestions:
- Use a stable unique id from the API if available
- Otherwise combine the email with a discriminator (e.g. index) to guarantee uniqueness
| if (!uninstall) { | ||
| return `LiveReview stops reviewing ${inst.account_login}'s pull requests for your account and removes its repositories here. ` + | ||
| 'The app stays installed on GitHub' + | ||
| (inst.other_orgs > 0 ? `, so the ${accountsLabel(inst.other_orgs)} using it keep working` : '') + |
There was a problem hiding this comment.
Severity: info
Singular case reads 'the 1 other LiveReview account ... keep working'. The verb should agree with the singular subject.
Suggestions:
- Branch on inst.other_orgs === 1 to emit 'keeps working' vs 'keep working'
There was a problem hiding this comment.
❌ LiveReview policy: Not approved (blocked at Risk score) · Review #13212
| Stage | Result | Detail |
|---|---|---|
| Eligibility | ✅ | author level 1 >= required 1 |
| Static rules | ✅ | no blocking rulesets configured |
| Risk score | ❌ | Risk score 1.00 (P100) is above the cutoff of 0.75 (P20) |
| LLM classification | ✅ | class "logic-change", confidence 8 |
| Final decision | ❌ | risk: Risk score 1.00 (P100) is above the cutoff of 0.75 (P20) |
- Repo sync: an unreadable allowed_repo_ids list on a user-scoped connector stops the sync instead of counting as "no repositories", which would have dropped all of them. - Comment routing: identify the PR from issue.pull_request instead of "/pull/" in the URL (a repo named pull matched), and match review URLs with starts_with so % and _ aren't wildcards. - Webhooks: a failure updating other orgs' copy of a PR no longer blocks the org handling the event; installation events and Uninstall process every org's connector before reporting a failure. - Other-accounts popup: dialog semantics, Escape closes it, focus on Close, index keys (masked emails can repeat); singular wording fix.
Summary
Follow-up to #137 from the launch dry run: several people from the same GitHub organization can now sign up with GitHub on their own, plus fixes to the
/gh-loginpage and to how LiveReview links look when shared.1. One GitHub App installation, several LiveReview accounts
Before: once someone connected the App on a GitHub organization (e.g. HexmosTech), anyone else from that organization who signed up got "Your GitHub installations are already connected to other LiveReview accounts" and couldn't sign in.
Now each LiveReview org gets its own connection to the same installation, with no invite needed:
at•••@gmail.com; the server masks them).20261023090000replaces the one-org-per-installation unique index with one per (installation, org).2.
/gh-loginfixeshexmos.com/signin?app=livereview, using the same sign-in URL helper as the Cloud login page.3. Title, description and link previews
Shared links (e.g.
livereview.hexmos.com/gh-login?src=launch-trial) previewed as "A powerful tool for automated code reviews" with the React boilerplate'sog:urland a map-collage image.og-image.png.property=, which Facebook and LinkedIn need.https://livereview.hexmos.com/, indexable.noindex, no canonical.Linked Issue
No linked issue; found while testing the Product Hunt launch flow (#137) with two HexmosTech accounts.
Validation
internal/api,internal/githubapp,internal/jobqueue,storage/providers/...andnetwork/providers/githubpass. New tests cover:installation_idtsc --noEmitandmake check-status-doc. The webpack config loads, and theindex.htmltemplate renders the new tags.Checklist
SECURITY.mdand updated related documentation if needed.Notes For Reviewers
Behavior changes worth a look
connectGitHubAppInstallationininternal/api/github_app_auth.go): an org that only sees the installation is limited to its user's repositories. Repositories added to the installation later appear for that org after its next GitHub sign-in.Before / after deploy
20261023090000.🔍 LiveReview
Auto-approval decisions
⚙️ Customize risk-based approval policy for this repository or your organization.