Skip to content

fix(nip56): address post-merge review findings on report ingestion - #786

Open
Priyanshubhartistm wants to merge 3 commits into
cameri:mainfrom
Priyanshubhartistm:fix/nip56-report-ingestion-gaps
Open

Priyanshubhartistm wants to merge 3 commits into
cameri:mainfrom
Priyanshubhartistm:fix/nip56-report-ingestion-gaps

Conversation

@Priyanshubhartistm

Copy link
Copy Markdown
Collaborator

Description

Following up on #770 after a collaborator review on the merged PR turned up four real bugs in report ingestion: dead rows for targetless reports, silent truncation of multi-target reports, non-atomic inserts across a report's rows, and the first report after a restart blocking on a cold WoT graph rebuild. Fixed all four.

Related Issue

Closes #785

Motivation and Context

All four bugs were already live on main

Signed-off-by: Priyanshubhartistm <bhartipriyanshustm@gmail.com>
@changeset-bot

changeset-bot Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 2277916

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
nostream Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Signed-off-by: Priyanshubhartistm <bhartipriyanshustm@gmail.com>
@greptile-apps

greptile-apps Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 2/5

The PR is not yet safe to merge because hot-enabling WoT can leave its graph empty, the first event can still block on a cold build, and accepted reports can lose targets.

Findings

  1. P1 Hot-enabled WoT stays empty ▶
  2. P1 First event still waits ▶
  3. P1 Accepted reports lose targets ▶
Summary

This PR skips targetless report rows, persists multi-target reports in one transaction, expands target extraction, and starts WoT graph building when its singleton is created.

  • The new warm-up needs to account for hot-enabling WoT and does not run early enough to avoid the first event’s cold-build delay.
  • The new target cap accepts reports while discarding targets beyond twenty.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[First incoming EVENT] --> B[Create WoT singleton]
  B --> C[Start graph build]
  C --> D[WoT distance lookup awaits build]
  B --> E[WoT disabled: mark empty graph ready]
  E --> F[Hot-enable WoT]
  F --> G[Ready flag skips graph build]
Loading

Reviews (1) · Last reviewed commit: "fix(nip56): address post-merge review fi..."

Comment thread src/services/wot-graph-service.ts
Comment thread src/factories/wot-graph-service-factory.ts
Comment thread src/utils/nip56.ts
@Priyanshubhartistm Priyanshubhartistm changed the title [BUG] NIP-56 report ingestion: dead rows, WoT-rebuild blocking, target truncation, no transaction fix(nip56): address post-merge review findings on report ingestion Sep 24, 2026
@coveralls

coveralls commented Sep 24, 2026 •

Copy link
Copy Markdown
Collaborator

Coverage Status

Coverage is 72.466% — Priyanshubhartistm:fix/nip56-report-ingestion-gaps into cameri:main. No base build found for cameri:main.

Signed-off-by: Priyanshubhartistm <bhartipriyanshustm@gmail.com>
@Priyanshubhartistm Priyanshubhartistm changed the title fix(nip56): address post-merge review findings on report ingestion fix(nip56): address post-merge review findings on report ingestion Sep 24, 2026

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] NIP-56 report ingestion: dead rows, WoT-rebuild blocking, target truncation, no transaction

2 participants