Skip to content

Fix/120 case insensitive repo removal#121

Closed
enjoyandlove wants to merge 4 commits into
entrius:testfrom
enjoyandlove:fix/120-case-insensitive-repo-removal
Closed

Fix/120 case insensitive repo removal#121
enjoyandlove wants to merge 4 commits into
entrius:testfrom
enjoyandlove:fix/120-case-insensitive-repo-removal

Conversation

@enjoyandlove
Copy link
Copy Markdown
Contributor

Summary

Fixes a bug where removing a GitHub repository via webhook could silently fail if the repo name was stored with different casing than what the webhook payload sent. Two changes:

  1. On add: repoFullName is now normalized to lowercase before insert, so all new rows are stored consistently.
  2. On remove: the UPDATE query now uses LOWER(repo_full_name) = LOWER(:name) via createQueryBuilder, making the lookup case-insensitive and matching rows regardless of how they were originally stored.

Related Issues

Fixes #120

Type of Change

  • Bug fix

Testing

  • Manually verified that removing a repo whose name is stored in mixed case (e.g. Org/Repo) is correctly matched and cleared when the webhook sends org/repo (all lowercase) or any other casing variant.
  • Existing tests pass.

Checklist

  • I have read the Contributing Guide
  • Code builds without errors
  • New and existing tests pass (if applicable)
  • Documentation updated (if applicable)
  • No unnecessary dependencies added

@xiao-xiao-mao xiao-xiao-mao Bot added the bug Something isn't working label May 21, 2026
@enjoyandlove
Copy link
Copy Markdown
Contributor Author

Hi. @anderdc
All checks have passed!
When you have a moment, could you please review this PR?

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.

Keep the remove-path change — LOWER(repo_full_name) = LOWER(:name) is correct and matches the convention used by installation.deleted and the admin controller.

Drop the add-path change (repoFullName: repo.full_name.toLowerCase() in installation.handler.ts). 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: 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. Lowercasing only the install-insert path, while pull_requests/issues keep GitHub's canonical casing, means any mixed-case repo would fail the webhook.service.ts:85 lookup and get silently skipped as 'not registered' — the same silent-ingestion-failure this PR is trying to prevent.

The case-insensitive removal alone captures the win. Normalizing stored casing would have to be done consistently across every write path and lookup, which is a much larger change and not what #120 needs.

@anderdc
Copy link
Copy Markdown
Collaborator

anderdc commented May 29, 2026

Duplicate of #127 which is better scoped/implemented. Closing.

@anderdc anderdc closed this May 29, 2026
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

2 participants