Skip to content

fix(db): give the anomaly-alert dedup claims their own alert_dedup_claims table - #8985

Merged
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
bitfathers94:fix/issue-8901
Jul 26, 2026
Merged

fix(db): give the anomaly-alert dedup claims their own alert_dedup_claims table#8985
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
bitfathers94:fix/issue-8901

Conversation

@bitfathers94

Copy link
Copy Markdown
Contributor

fix(db): give the anomaly-alert dedup claims their own alert_dedup_claims table

runAnomalyAlerts in src/review/alerts.ts writes per-hour dedup claims shaped
(id, project, target_id, notification_key, status) with
ON CONFLICT(project, target_id, notification_key) DO NOTHING, but pointed those
raw INSERTs at notification_deliveries -- the migrated badge read-model (0031),
whose columns are dedup_key/channel/recipient_login/... with a UNIQUE(dedup_key,
channel) index. None of the claim columns nor the ON CONFLICT target exist there.
runAnomalyAlerts has no callers yet, so this hasn't fired, but the moment it is
wired to a cron path every Discord-notify invocation throws at the first INSERT.

Add migration 0181 creating a distinctly-named alert_dedup_claims table with the
(project, target_id, notification_key) unique index the port actually needs, and
point both claim inserts at it. Allowlist the table in check-schema-drift as a
raw-SQL-only feature table (alerts.ts accesses it via env.DB.prepare, not Drizzle).

A new test drives runAnomalyAlerts against the real migrated D1 (createTestEnv)
and asserts both claims land in alert_dedup_claims and a same-hour repeat is
throttled by the unique constraint -- proving the write path no longer collides.

Closes #8901

Validation

Verified locally on this branch before opening:

  • npm run typecheck
  • npx turbo run build:tsc build:verify
  • npm run test:coverage — patch coverage 100.0% of changed lines

@bitfathers94
bitfathers94 requested a review from JSONbored as a code owner July 26, 2026 15:05
@superagent-security

Copy link
Copy Markdown
Contributor

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

…aims table

runAnomalyAlerts in src/review/alerts.ts writes per-hour dedup claims shaped
(id, project, target_id, notification_key, status) with
ON CONFLICT(project, target_id, notification_key) DO NOTHING, but pointed those
raw INSERTs at notification_deliveries -- the migrated badge read-model (0031),
whose columns are dedup_key/channel/recipient_login/... with a UNIQUE(dedup_key,
channel) index. None of the claim columns nor the ON CONFLICT target exist there.
runAnomalyAlerts has no callers yet, so this hasn't fired, but the moment it is
wired to a cron path every Discord-notify invocation throws at the first INSERT.

Add migration 0181 creating a distinctly-named alert_dedup_claims table with the
(project, target_id, notification_key) unique index the port actually needs, and
point both claim inserts at it. Allowlist the table in check-schema-drift as a
raw-SQL-only feature table (alerts.ts accesses it via env.DB.prepare, not Drizzle).

A new test drives runAnomalyAlerts against the real migrated D1 (createTestEnv)
and asserts both claims land in alert_dedup_claims and a same-hour repeat is
throttled by the unique constraint -- proving the write path no longer collides.

Closes JSONbored#8901
@codecov

codecov Bot commented Jul 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.88%. Comparing base (f5e457e) to head (0e85040).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8985      +/-   ##
==========================================
+ Coverage   90.56%   93.88%   +3.32%     
==========================================
  Files          96      807     +711     
  Lines       22490    80485   +57995     
  Branches     3884    24408   +20524     
==========================================
+ Hits        20367    75561   +55194     
- Misses       1945     3560    +1615     
- Partials      178     1364    +1186     
Flag Coverage Δ
backend 95.17% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/review/alerts.ts 100.00% <ø> (ø)

... and 710 files with indirect coverage changes

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 26, 2026
@loopover-orb

loopover-orb Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Tip

✅ LoopOver review result - approve/merge recommended

Review updated: 2026-07-26 15:51:01 UTC

4 files · 1 AI reviewer · no blockers · readiness 98/100 · CI green · clean

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This PR fixes a real latent bug: runAnomalyAlerts in src/review/alerts.ts was writing (project, target_id, notification_key) claims via ON CONFLICT into notification_deliveries, a table whose actual migrated schema (0031) is a completely different badge-read-model shape with no matching unique index — the first real INSERT would have thrown SQLITE_ERROR. Migration 0181 adds a correctly-shaped alert_dedup_claims table with the exact unique index the ON CONFLICT target needs, both raw INSERTs in alerts.ts are repointed at it, the table is properly allowlisted in check-schema-drift.ts as raw-SQL-only, and a new test drives the real INSERT path against a fully migrated D1 via createTestEnv, proving both claims land and the same-hour repeat is throttled by the unique constraint. This closes issue #8901 and is a narrow, well-targeted correctness fix with no scope creep.

Nits — 3 non-blocking
  • migrations/0181_alert_dedup_claims.sql:9 uses `created_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP` — this repo's own convention flags static-default timestamps as a drizzle-injection hazard, but since this table is raw-SQL-only (no Drizzle mapping declared), it's likely fine; worth a one-line comment confirming that's intentional.
  • No test exercises resolveWebhook/webhookUrl edge cases changed by this PR (there are none — this is purely additive test coverage, so no concern here).
  • Consider adding a short comment in alerts.ts near the two INSERTs noting the new table name so a future contributor scanning the file doesn't need to cross-reference the migration to know why alert_dedup_claims exists.

Decision drivers

  • ✅ Code review — No blockers (1 reviewer)
  • ✅ Gate result — Passing (No configured blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #8901
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 45 registered-repo PR(s), 37 merged, 0 issue(s).
Contributor context ✅ Confirmed Gittensor contributor bitfathers94; Gittensor profile; 45 PR(s), 0 issue(s).
Improvement ✅ Minor risk: clean · value: minor · LLM: significant
Linked issue satisfaction

Addressed
The PR adds migration 0181 creating a distinctly-named alert_dedup_claims table with the exact (project, target_id, notification_key) unique index, updates alerts.ts's two INSERTs to target it instead of notification_deliveries, and adds a test that runs runAnomalyAlerts against a real migrated D1 env verifying claims land correctly and the throttle works.

Review context
  • Author: bitfathers94
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: not available
  • Official Gittensor activity: 45 PR(s), 0 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Keep the PR focused and include validation evidence before maintainer review.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
🧪 Chat with LoopOver

Ask LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

  • @loopover ask &lt;question&gt; answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat &lt;question&gt; answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://loopover.ai/docs/loopover-commands

🧪 Experimental — new and may change.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


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

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

  • Re-run LoopOver review

@loopover-orb loopover-orb Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LoopOver approves — the gate is satisfied and CI is green.

@loopover-orb
loopover-orb Bot merged commit 330fdd6 into JSONbored:main Jul 26, 2026
8 checks passed
JSONbored added a commit that referenced this pull request Jul 26, 2026
Two PRs merged within 18 minutes of each other and both grabbed migration
number 0181 (0181_alert_dedup_claims.sql from #8985, 0181_decision_replay_inputs.sql
from #9014), breaking db:migrations:check on main for every subsequent PR.

Renumbers the later-merged file (decision_replay_inputs) to 0182, the next free
number. No code or test references either file by name/number, so this is a
pure rename.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(db): notification_deliveries table name collides with an unrelated schema used by the currently-unwired src/review/alerts.ts anomaly-alert port

1 participant