fix(notifications): match issue-watch repo names case-insensitively - #747
Conversation
issue-watch subscriptions stored and matched repoFullName case-sensitively while the rest of the codebase treats repo names case-insensitively. A contributor who subscribed via gittensory_watch_issues with non-canonical casing (GitHub repo names are case-insensitive) was silently never matched by the webhook's canonical repository.full_name lookup, so the feature quietly delivered no notifications. Lowercase repoFullName on store and on both lookups (mirroring the existing login/label normalization), so matching is symmetric and the (login, repo) unique index dedupes across casings. Closes JSONbored#746
|
Note Gittensory Gate skippedPR closed before full evaluation. No late first comment was created.
💰 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. |
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
|
gittensory · advisory review Reviewed 2 changed file(s) — two independent AI reviewers. Changed files: Suggested action: 🛠️ Request changes. (reviewers split: request changes / merge) Address the suggestions below before merging. Reviewer A · Suggestions
Worth double-checking
Reviewer B · No blocking issues spotted. |
JSONbored
left a comment
There was a problem hiding this comment.
PR #747 — fix(notifications): match issue-watch repo names case-insensitively
Author: galuis116 | CI: Fail
ACTION: REquest changes
Highlights:
- Lowercases repoFullName on store and all three lookups (upsert/delete/listIssueWatchersForRepo), matching existing login/labels normalization (#604 convention).
- Genuine correctness fix: webhooks deliver canonical repository.full_name, so non-canonical-cased subscriptions silently never matched. Fails closed.
- Test proves store-normalization, canonical-casing delivery, idempotency across casings, and case-insensitive unwatch.
Notes / before merge:
- mergeable_state: fail, please ensure passing CI (needs test coverage updates)
…e admits fan-out The main merge brought in JSONbored#742's visibility-aware fan-out gate, which only fans out issue-watch events for a tracked, accessible repo. The case-insensitivity regression test subscribed to a repo it never upserted, so the gate (correctly) returned no events. Upsert it as a tracked PUBLIC repo, matching the other detectIssueWatchEvents tests, so the case-insensitive match is exercised end-to-end.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #747 +/- ##
=======================================
Coverage 96.72% 96.73%
=======================================
Files 94 94
Lines 13850 13851 +1
Branches 5052 5051 -1
=======================================
+ Hits 13397 13399 +2
Misses 86 86
+ Partials 367 366 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
codecov/patch flagged the upsert's ternary fallback as a partial branch: the
`: { login, repoFullName, labels }` arm is unreachable because the row always
exists immediately after the insert/onConflictDoUpdate. Mark it /* v8 ignore */
(the established convention for defensive type-safety fallbacks) so the diff is
fully covered.
JSONbored
left a comment
There was a problem hiding this comment.
PR #747 — fix(notifications): match issue-watch repo names case-insensitively
Verdict: MERGE
Resolves issue #746: FULLY
CI: All green (test 1/2, lint, workers, ui, security, codecov). gittensory/Context neutral.
Highlights:
- Exactly the prescribed fix: lowercases repoFullName on store (upsertIssueWatchSubscription) and on both lookups (listIssueWatchersForRepo, deleteIssueWatchSubscription), mirroring existing login/labels normalization; select-back where-clause corrected to the normalized value.
- Restores the codebase-wide case-insensitive repo invariant (getRepository, #604) and lets UNIQUE(login, repo_full_name) dedupe across casings.
- Strong regression test: store normalization, canonical-casing delivery via detectIssueWatchEvents, cross-casing idempotency (no dup row), case-insensitive unwatch — exercises every path the bug touched.
Concerns / required changes:
- None.
Completeness vs issue: Implements the issue's recommended unconditional-lowercase fix on all three functions plus the exact regression test requested; fully resolves the silent-missed-notification and duplicate-row problems.
Summary
The issue-watch feature (#735,
gittensory_watch_issues) stored and matched the watchedrepoFullNamecase-sensitively, while the rest of the codebase treats repo full names case-insensitively (getRepositoryresolves vialower(fullName) = lower(input); the registry sync was fixed for this in #604). The watch functions deliberately lowercaseloginandlabels"for case-insensitive matching" — but notrepoFullName.Impact: the MCP tool stores the user's raw casing (schema is just
z.string(), no canonicalization), but the webhook looks up watchers with GitHub's canonicalpayload.repository.full_namevia an exacteq:So a contributor who subscribes with non-canonical casing (e.g.
jsonbored/gittensory— trivially common, since GitHub URLs are case-insensitive) gets a subscription the webhook can never match: they see it inlist, but silently never receive any issue-watch notification. It also permitted duplicate rows for the same(login, repo)across casings (the unique index is case-sensitive). Fails closed — a correctness/usability bug, not a security issue.Fix
Lowercase
repoFullNameon store and on both lookups (upsertIssueWatchSubscription,listIssueWatchersForRepo,deleteIssueWatchSubscription), mirroring the existinglogin/labelsnormalization in the same functions. Matching is now symmetric, and theUNIQUE(login, repo_full_name)index dedupes across casings since the stored value is always lowercase.Tests
Added a regression test in
issue-watch.test.ts: subscribing withOwner/Repois found by the canonicalowner/repolookup and firesdetectIssueWatchEvents; the stored/listed name is normalized; re-subscribing under another casing is idempotent; andunwatchis case-insensitive. (The existing CRUD test only ever used the already-lowercaseowner/repo, so the asymmetry was invisible.)Full coverage suite green locally (only the known CRLF-local
gittensory-focus-manifesttest fails locally; passes in CI).Closes #746