Skip to content

fix(registry): resolve repo casing case-insensitively on sync - #604

Merged
JSONbored merged 6 commits into
JSONbored:mainfrom
galuis116:fix/registry-sync-case-insensitive-fullname
Jun 13, 2026
Merged

fix(registry): resolve repo casing case-insensitively on sync#604
JSONbored merged 6 commits into
JSONbored:mainfrom
galuis116:fix/registry-sync-case-insensitive-fullname

Conversation

@galuis116

Copy link
Copy Markdown
Contributor

Closes #603.

persistRegistrySnapshot keyed the upsert conflict target (onConflictDoUpdate({ target: fullName })) and the de-registration (notInArray(fullName, …)) on the case-sensitive repositories.fullName primary key. But repo names arrive from multiple sources (the upstream registry vs GitHub-canonical webhook/API casing) and the rest of the system resolves repos case-insensitively (getRepository has a lower(fullName) fallback). On a casing mismatch the sync inserted a duplicate primary-key row instead of updating the existing one, and the case-sensitive de-registration nulled the registration on the other casing — splitting a repo into two rows so a registered repo could read back as unregistered. Same class as the already-fixed #223/#225 (case-sensitive repoFullName comparison), at an unfixed site.

Change

  • Fetch existing repo names once and resolve each snapshot repo to its existing row by lowercased name, so a casing variant updates that row (no duplicate primary key).
  • De-register by lowercased comparison, so a casing variant of a still-registered repo is never wrongly de-registered.
  • No stored-case change for existing rows; behavior is identical when casing already matches.

Verification

  • Two regression tests (both fail on the old code): a GitHub-canonical row + a differently-cased registry entry yields a single registered row; a casing change between snapshots does not de-register the repo.
  • registry.test.ts 7/7; tsc --noEmit clean; full suite green; sync.ts 98% branch; global branch coverage holds (>= 97% gate).

persistRegistrySnapshot keyed the upsert conflict target and the de-registration on the case-sensitive fullName primary key, while repo names arrive from multiple sources (registry vs GitHub-canonical) and the rest of the system resolves repos case-insensitively. A casing mismatch inserted a duplicate row and de-registered the wrong one (same class as JSONbored#223/JSONbored#225). Resolve each snapshot repo to an existing row by lowercased name before upsert, and de-register by lowercased comparison.
@ghost

ghost commented Jun 11, 2026

Copy link
Copy Markdown

Note

Gittensory Gate skipped

PR closed before full evaluation. No late first comment was created.

Signal Result Evidence Action
Gate result ⚠️ Skipped #604 is no longer open. No action.

💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

@ghost ghost added the gittensory:reviewed label Jun 11, 2026
@ghost

ghost commented Jun 11, 2026

Copy link
Copy Markdown

reviewbot · advisory review

Reviewed 2 changed file(s) — two independent AI reviewers.

Suggested action: 🛠️ Request changes. Address the suggestions below before merging.

Reviewer A · gpt-oss-120b — recommends 🛠️ request changes
The PR adds case‑insensitive handling for repository names during registry sync and adds tests for the new behavior. The intent is clear, but the implementation inserts rows that may already exist, which can violate the primary‑key constraint on repositories.fullName.

Suggestions

  • Change the repository insert to an upsert (ON CONFLICT DO UPDATE) so that a case‑variant row updates the existing record instead of causing a duplicate‑key error.
  • Import and use the appropriate conflict‑resolution helper from drizzle‑orm (e.g., .onConflictDoUpdate) and ensure all relevant columns are updated.
  • Consider refreshing the list of existing full names after the insert/upsert if further logic depends on the latest state.

Worth double-checking

  • Primary‑key violation when a snapshot contains a repo with different casing that already exists.
  • Potential stale‑repo de‑registration if the list of existing names isn’t refreshed after inserts.

Reviewer B · nemotron-3-120b-a12breview unavailable

@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@ghost

ghost commented Jun 12, 2026

Copy link
Copy Markdown

gittensory · advisory review

Reviewed 2 changed file(s) — two independent AI reviewers.

Suggested action:Safe to merge — both reviewers found no blocking issues.

Reviewer A · gpt-oss-120b — recommends ✅ merge
The PR updates registry synchronization to handle repository full names case‑insensitively, preventing duplicate primary‑key rows and avoiding accidental de‑registration on empty snapshots. New tests cover the added behavior and all existing functionality remains unchanged.

Suggestions

  • Consider adding a database index on LOWER(full_name) to keep the case‑insensitive lookups performant as the repository table grows.
  • Document the new case‑insensitive de‑registration rule in the sync module comments or README for future maintainers.

Worth double-checking

  • The in‑memory map assumes the initial list of existingFullNames is up‑to‑date; concurrent sync runs could cause race conditions, though DB constraints will still protect against duplicates.
  • Large numbers of stale repositories could generate a very long IN clause; monitor query size if the repo list becomes huge.

Reviewer B · nemotron-3-120b-a12b — recommends ✅ merge
The PR fixes a case-sensitivity issue in registry sync by resolving repository names case-insensitively when upserting and de-registering. It prevents duplicate rows from casing variants and avoids mass de-registration on empty snapshots. The change is well-tested with four new unit tests covering the key scenarios.

Worth double-checking

  • Performance: Fetching all repository names on every sync could become expensive as the table grows; consider indexing or optimizing if scale increases.
  • Ensure the case-resolution logic aligns with getRepository elsewhere in the codebase (not shown in diff) to maintain consistency.

@ghost ghost added the gittensory-review label Jun 12, 2026

@JSONbored JSONbored left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Please see latest review for requested changes and ensure CI is green when done.

JSONbored and others added 2 commits June 12, 2026 07:53
Address review: (1) restore the empty-snapshot guard so a failed/empty registry fetch never de-registers every repo; (2) record each resolved canonical name within the sync loop so a case-variant duplicate in the same snapshot upserts the existing row instead of inserting a second case-only-different primary key. Add tests for both.
@galuis116

Copy link
Copy Markdown
Contributor Author

Thanks for the review — addressed in 7a8d110:

  • Upsert / duplicate concern (Reviewer A): the persist already uses onConflictDoUpdate({ target: fullName }), so a case-variant of an existing row updates it. The one remaining gap was a snapshot containing two case-variants of the same repo: canonicalByLower was built once and not updated in the loop, so the second still inserted a duplicate. Now each resolved canonical name is recorded inside the loop, so an intra-snapshot case-variant upserts the same row.
  • Empty-snapshot safety: while addressing the "add an empty-snapshot test" suggestion I found my rewrite had dropped the original registeredFullNames.length > 0 guard — an empty/failed registry fetch would have de-registered every repo. Restored as snapshot.repositories.length > 0 && staleFullNames.length > 0.

Added regression tests for both (empty snapshot preserves registrations; two case-variant entries collapse to one row). tsc clean, full suite green, sync.ts 100% branch.

@JSONbored
JSONbored self-requested a review June 13, 2026 05:52
@dosubot dosubot Bot added the lgtm label Jun 13, 2026
@JSONbored
JSONbored merged commit f71c28e into JSONbored:main Jun 13, 2026
7 checks passed
@github-project-automation github-project-automation Bot moved this from Todo to Done in gittensory - v1 roadmap Jun 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

No open projects
Status: Done

2 participants