Skip to content

fix(installation): case-insensitive repo removal and normalize PK#127

Open
Yurii214 wants to merge 1 commit into
entrius:testfrom
Yurii214:fix/120-case-insensitive-removal
Open

fix(installation): case-insensitive repo removal and normalize PK#127
Yurii214 wants to merge 1 commit into
entrius:testfrom
Yurii214:fix/120-case-insensitive-removal

Conversation

@Yurii214
Copy link
Copy Markdown

Fixes #120

Summary

  • Add shared normalizeRepoFullName() / validateRepoFullName() utility
  • Webhook installation_repositories.added stores lowercase canonical PK
  • Webhook installation_repositories.removed clears rows case-insensitively
  • Admin register/backfill paths normalize via shared validator
  • Log a warning when removal updates zero rows (previously silent no-op)

Why

GitHub repo identity is case-insensitive, but PostgreSQL PK lookups are not.
Admin registration could create owner/repo while webhooks stored Owner/Repo,
so uninstall events returned HTTP 202 while leaving registered=true.

Test plan

  • npm run lint passes
  • npm run build passes

@xiao-xiao-mao xiao-xiao-mao Bot added the bug Something isn't working label May 24, 2026
@Yurii214 Yurii214 changed the title ix(installation): case-insensitive repo removal and normalize PK fix(installation): case-insensitive repo removal and normalize PK May 24, 2026
@Yurii214
Copy link
Copy Markdown
Author

This updates both the webhook install/remove path and the shared repo-name normalization path so casing stays consistent across registration and uninstall flows.
Lint/build are passing locally; happy to adjust if you’d prefer a narrower fix.

@Yurii214
Copy link
Copy Markdown
Author

This fix normalizes repo names across both admin and webhook paths, and also makes uninstall cleanup case-insensitive so legacy mixed-case rows are cleared correctly.
Happy to narrow or adjust the approach if you’d prefer a smaller patch.

@anderdc anderdc added enhancement New feature or request and removed bug Something isn't working labels May 29, 2026
Copy link
Copy Markdown
Collaborator

@anderdc anderdc left a comment

Choose a reason for hiding this comment

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

The case-insensitive removal (LOWER(repo_full_name) = :repoFullName) and the zero-row warning are good — keep both.

Blocker: normalizeRepoFullName() lowercases what gets stored in the repos table (installation.handler.ts added path). But the repos table is the canonical-case source of truth (see the comment at pulls.service.ts:50-51), and three sites match it exact-case against GitHub's canonical casing: webhook.service.ts:85 (findOneBy({ repoFullName }) — the registration gate), miners.service.ts:183/232 (LEFT JOIN repos r ON r.repo_full_name = p.repo_full_name), and pulls.service.ts:50. The PR/issue/comment/review handlers all still store raw canonical casing, so lowercasing only the repos row means any mixed-case repo would fail the webhook.service.ts:85 lookup and have all its events skipped as 'not registered' — the same silent-failure this PR is trying to fix.

Drop the stored-value lowercasing (the normalizeRepoFullName on insert, and don't have validateRepoFullName mutate casing for stored/keyed values). The case-insensitive removal alone fixes #120. Normalizing stored casing would have to be applied consistently across every write path and every exact-match consumer — a much larger change than #120 needs.

…trius#120)

Centralize repo full-name validation without mutating canonical casing.
Installation removal events now clear rows via case-insensitive match and
log when no row is updated.

Fixes entrius#120

Co-authored-by: Cursor <cursoragent@cursor.com>
@Yurii214 Yurii214 force-pushed the fix/120-case-insensitive-removal branch from 8232fa8 to cdbbb50 Compare May 29, 2026 21:45
@Yurii214
Copy link
Copy Markdown
Author

Updated. I removed stored-value lowercasing so repo rows keep canonical GitHub casing. The fix now only applies case-insensitive matching on removal and keeps the zero-row warning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Critical] Silent uninstall failure — data ingestion continues after app removal due to case-sensitive PK match

3 participants