Skip to content

fix: complete + deterministic dedup candidate set on sync (GIT-70) - #22

Merged
nTEG-dev merged 1 commit into
mainfrom
feature/GIT-70-sync-completeness
Aug 7, 2026
Merged

nTEG-dev merged 1 commit into
mainfrom
feature/GIT-70-sync-completeness

Conversation

@nTEG-dev

@nTEG-dev nTEG-dev commented Aug 6, 2026 •

Copy link
Copy Markdown
Member

Base-retarget trail. This PR briefly pointed at main and was then retargeted back to feature/GIT-51-session-recovery.

Why: ci.yml triggers on pull_request: branches: [main], so a PR based on a stack branch matches no trigger and gets no CI at all — gh pr checks reported "no checks reported" for both #21 and #22. The stack tip contains every commit, so pointing #22 at main for one pass matrix-verified GIT-51 and GIT-70 on Node 18/20/22 together, then it was pointed back so the review diff stays the GIT-70 delta only.

Also worth recording: retargeting alone did not trigger CI. A base change fires pull_request: edited, and the workflow declares no types:, so it defaults to [opened, synchronize, reopened]. A close/reopen was needed to fire the run.

The durable fix — adding feature/** to the push triggers — is filed as a line item on GIT-45, not executed here: ci.yml is the release-path file and does not get edited mid-sprint.

Draft — verification only. Do not merge. Base is feature/GIT-51-session-recovery (stack tip), not main.

Third in the stack: PR #20 (GIT-69 + GIT-67/63) → PR #21 (GIT-51) → this. Based on the stack tip so the diff shows only the GIT-70 delta. Retarget as the stack merges.

The defect

An unordered limit: 200 single page feeding the text-dedup map in syncThreadsToSupabase, against a table already at 256 rows.

Every thread still synced — the per-thread lookup is an exact thread_id match, unbounded by the window. What truncated was dedup coverage. A text-matching row outside the loaded 200 meant the map missed it and a duplicate row was created, silently, with the dedup log never firing.

A write that fires because a read was incomplete. And because there was no ORDER BY, which 200 rows loaded was arbitrary — so whether a given close duplicated was nondeterministic between identical runs.

session_close now awaits this sync (GIT-69 item 3), so it runs synchronously on the close path.

Fix

  1. Pagination via directQueryAll, which already existed — checked before building, same as the harness.
  2. A deterministic total order (thread_id.asc). Required, not cosmetic: PostgREST Range pagination over an unordered result set can repeat and skip rows between pages, so without it the pagination would introduce the very gaps it was added to close. thread_id is unique, so it orders totally on its own.

directQueryAll stops at its maxRows cap without saying so. Inheriting that silence would rebuild the defect one layer up, so the cap is detected and reported.

Why coverage is a separate axis from success

dedup_coverage: "complete" | "partial" | "unavailable", with dedup_candidates so the claim is auditable. session_close surfaces it as a warning, not a failure.

Every thread syncs by identity regardless of coverage. Incomplete coverage means a duplicate may exist — which all_synced structurally cannot detect, because from the sync's perspective writing a duplicate is a successful write.

detects misses
GIT-69 item 3 gate a thread that failed to sync a thread that synced as a duplicate
this PR the duplicate —

The gate makes close honest about failures. This makes it honest about correctness.

Identity matching at the exact-thread_id lookup is untouched, per R8a.

Verification

Check Result
npm run typecheck clean
npm run build clean
npm run test:unit 1093 passed
e2e GIT-67/63 acceptance 12 passed

New coverage (+5): deterministic order asserted at the call site; complete coverage past the old 200 window; cap reported as partial rather than absorbed; load failure reported as unavailable; sync still succeeds while coverage is incomplete. Four existing sync tests were updated — their mocks assumed directQuery for the candidate load and caught the change correctly.

Refs GIT-70, GIT-69, GIT-63

The last item before any tag conversation exists. session_close now awaits
this sync (GIT-69 item 3), so the window runs synchronously on the close
path.

The defect: an unordered `limit: 200` single page feeding the text-dedup
map, against a table already at 256 rows. Every thread still synced — the
per-thread lookup is an exact thread_id match, unbounded by the window — so
what truncated was DEDUP COVERAGE. A text-matching row outside the loaded
200 meant the map missed it and a DUPLICATE row was created, silently, with
the dedup log never firing. A write that fires because a read was
incomplete.

Two changes:

1. Pagination via directQueryAll, which already existed — checked before
   building, same as the harness. The candidate set is now actually
   complete rather than an arbitrary slice.

2. A deterministic total order (thread_id.asc). Required, not cosmetic:
   PostgREST Range pagination over an unordered result set can repeat and
   skip rows between pages, so without it the pagination would introduce
   the very gaps it was added to close. thread_id is unique, so it orders
   totally on its own.

directQueryAll stops at its maxRows cap WITHOUT saying so. Inheriting that
silence would rebuild the defect one layer up, so the cap is detected and
reported: dedup_coverage is "complete" | "partial" | "unavailable", with
dedup_candidates so the claim is auditable.

Coverage is deliberately a separate axis from all_synced, and session_close
surfaces it as a warning rather than a failure. Every thread syncs by
identity regardless; incomplete coverage means a duplicate MAY exist —
which all_synced structurally cannot detect, because writing a duplicate IS
a successful write. That distinction is the whole ticket: the outcome
contract makes the path honest about failures, not about correctness.

Identity matching at the exact-thread_id lookup is untouched, per R8a.

Tests: +5 (deterministic order asserted at the call; complete coverage past
the old 200 window; cap reported as partial not absorbed; load failure
reported as unavailable; sync still succeeds while coverage is incomplete).
Also updated the 4 existing sync tests whose mocks assumed directQuery for
the candidate load — they caught the change correctly.

1093 unit + 12 e2e green, typecheck and build clean.

Refs GIT-70, GIT-69, GIT-63

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@nTEG-dev
nTEG-dev changed the base branch from feature/GIT-51-session-recovery to main August 6, 2026 03:37
@nTEG-dev nTEG-dev closed this Aug 6, 2026
@nTEG-dev nTEG-dev reopened this Aug 6, 2026
@nTEG-dev
nTEG-dev changed the base branch from main to feature/GIT-51-session-recovery August 6, 2026 03:40
@nTEG-dev
nTEG-dev changed the base branch from feature/GIT-51-session-recovery to main August 7, 2026 02:26
@nTEG-dev
nTEG-dev marked this pull request as ready for review August 7, 2026 02:26
@nTEG-dev nTEG-dev closed this Aug 7, 2026
@nTEG-dev nTEG-dev reopened this Aug 7, 2026
@nTEG-dev
nTEG-dev merged commit 8fd4ae6 into main Aug 7, 2026
8 checks passed
nTEG-dev added a commit that referenced this pull request Aug 7, 2026
GIT-74 token audit + GIT-76 field-drop fix (draft, stacked on #22)
@nTEG-dev
nTEG-dev deleted the feature/GIT-70-sync-completeness branch September 23, 2026 05:49
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.

1 participant