fix(sync): PR hydration fails on overlapping check-run pages

[vincentkoc]

What Problem This Solves

Fixes PR hydration failing when overlapping GitHub check-run pages contain the same provider ID more than once.

User Impact

PR hydration retains each observed check-run identity once, in first-seen API order, using its latest fetched observation. No schema migration, rebuild, configuration change, or portable-profile change is required.

Distinct provider IDs sharing the same stored name/details URL can still violate the existing SQL key. They are not silently collapsed: the item rolls back and remains retryable. Supporting that separate case requires a reviewed storage-contract migration.

Why This Change Was Made

Consolidation belongs at check ingestion, where the lossless provider ID is available. Replacing the entire repeated observation avoids mixing old and later fetched fields; missing IDs remain separate. Generic pagination and database conflict behavior are unchanged.

The name/details-URL uniqueness key dates to the May 5, 2026 PR-cache implementation. This is storage-contract context, not proof of the original incident's cause.

Evidence

• The new real HTTP-client -> syncer -> SQLite regression fails on base b132e5b7816ae622646ba4745682510f854d26b0 with SQLite 2067 on pull_request_checks(thread_id, name, details_url), then passes with this change.
• Eight focused new/existing tests passed across syncer, GitHub, and store. The two new tests also passed with -race.
• Coverage includes overlap within/across pages, IDs above 2^53, complete later-observation replacement, missing IDs, repeat refresh across close/reopen, metadata-only preservation, healthy neighboring PRs, distinct-ID collision rollback, reservations, failure-ledger recovery, and unchanged success clocks after page failure/cancellation.
• A fresh complete 226-check census found no duplicate provider IDs or stored-key collisions. The original failed 224-row payload was not retained, so this fixes a demonstrated duplicate-list invariant but does not prove that pagination overlap caused the original incident.
• Production: +13/-2 lines (net +11 for identity consolidation); tests: +264; changelog: +1. Schema, SQL keys, and the semantic-v1 digest algorithm are untouched.
• CI attempt 1 passed Linux/macOS full validation, Windows portable/build checks, and Docs. Total coverage was 85.7% on both Go platforms. Docker attempt 1 also passed.
• CI and Docker checked out 0763805b97f8c90780498029e437a8e0701943cb; its tree ba588163befa415c0edfb60543c3ef7e627d10dc exactly matches head 1ceae73a8b9fa33224108ed1858872f446a329e9.
• Native branch review through P3 and independent source review completed without findings. No runtime datastore or publication settings were changed.

[clawsweeper]

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

[clawsweeper]

Codex review: blocked before merge. Reviewed September 13, 2026, 4:50 PM ET / 20:50 UTC.

ClawSweeper review

What this changes

Consolidate repeated GitHub check-run IDs before saving PR details, retaining first-seen order and the latest fetched observation, with regression coverage and a changelog entry.

Merge readiness

⛔ Blocked before merge - 1 item remains

This repair remains useful: current main and v0.9.6 still pass repeated check-run observations to storage. No blocking defect was found in the introduced patch.

Priority: P2
Reviewed head: 1ceae73a8b9fa33224108ed1858872f446a329e9

Review scores

Measure	Result	What it means
Overall readiness	🐚 platinum hermit (4/6)	A focused repair with substantial regression coverage and no identified blocking defect.
Proof confidence	🌊 off-meta tidepool	Not applicable: The author is a MEMBER, so the external-contributor proof gate does not apply. The supplied regression exercises real HTTP-client-to-syncer-to-SQLite ingestion and reports after-fix success; this review inspected its assertions without rerunning it.
Patch quality	🐚 platinum hermit (4/6)	No actionable review findings were identified.

Verification

Check	Result	Evidence
Real behavior	Not applicable	Not applicable: The author is a MEMBER, so the external-contributor proof gate does not apply. The supplied regression exercises real HTTP-client-to-syncer-to-SQLite ingestion and reports after-fix success; this review inspected its assertions without rerunning it.
Evidence reviewed	7 items	Verified patch scope: The checkout matches the pinned PR head. The introduced delta contains only check-run consolidation, two regression tests, and one changelog entry; git status was clean. Still necessary on main and latest release: Both fetched main and v0.9.6 append every named check observation without provider-ID consolidation. Live GitHub REST reads confirmed main remains b132e5b and the latest release is v0.9.6. Failure mechanism and repair boundary: Pagination appends overlapping rows; JSON decoding preserves numeric IDs. Check persistence inserts rows under the existing unique(thread_id, name, details_url) constraint, so repeated named checks with the same nonempty URL collide. Consolidating equal provider IDs before persistence fixes that trigger without changing SQL conflict handling.
Findings	None	None.
Security	None	None.

How this fits together

Gitcrawl fetches GitHub pull-request details into a local SQLite archive for search and review. Check-run ingestion converts paginated API responses into the check snapshots saved with each PR.

flowchart LR
  A[PR detail sync] --> B[GitHub check-run pages]
  B --> C[Lossless ID decoding]
  C --> D[Consolidate repeated IDs]
  D --> E[Atomic SQLite write]
  E --> F[Local PR review and search]

Loading

Before merge

• Resolve merge risk (P1) - The original incident may have had a different cause; distinct provider IDs sharing the legacy storage key remain a separate, explicitly acknowledged limitation.

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Production versus test growth	Production +13/-2 (net +11); tests +264; changelog +1	Production growth is narrowly justified by provider-ID consolidation, with broader regression coverage around persistence and recovery.

Merge-risk options

Maintainer options:

1. Decide the mitigation before merge
   Keep identity consolidation at check ingestion and preserve atomic failure handling; any broader storage-identity migration should remain separate.
2. Pause or close
   Do not merge this PR until maintainers decide whether the risk is worth taking.

Technical review

Best possible solution:

Keep identity consolidation at check ingestion and preserve atomic failure handling; any broader storage-identity migration should remain separate.

Do we have a high-confidence way to reproduce the issue?

Yes, for the duplicate-observation mechanism: advancing pages containing the same named check ID and nonempty URL reach conflicting SQLite inserts on main. Source and the supplied regression establish that path; this review did not execute it or establish the original incident's cause.

Is this the best way to solve the issue?

Yes. Consolidating by the existing lossless provider ID is a narrow repair that avoids weakening database constraints or changing unrelated pagination behavior.

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning medium; reviewed against b132e5b7816a.

Labels

Label changes:

• add P2: This is a bounded repair for PR-detail hydration failures caused by repeated check-run observations.
• add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
• add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The author is a MEMBER, so the external-contributor proof gate does not apply. The supplied regression exercises real HTTP-client-to-syncer-to-SQLite ingestion and reports after-fix success; this review inspected its assertions without rerunning it.

Label justifications:

• P2: This is a bounded repair for PR-detail hydration failures caused by repeated check-run observations.
• rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
• status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The author is a MEMBER, so the external-contributor proof gate does not apply. The supplied regression exercises real HTTP-client-to-syncer-to-SQLite ingestion and reports after-fix success; this review inspected its assertions without rerunning it.

Evidence

What I checked:

• Verified patch scope: The checkout matches the pinned PR head. The introduced delta contains only check-run consolidation, two regression tests, and one changelog entry; git status was clean. (internal/syncer/pull_details_more.go:12, 1ceae73a8b9f)
• Still necessary on main and latest release: Both fetched main and v0.9.6 append every named check observation without provider-ID consolidation. Live GitHub REST reads confirmed main remains b132e5b and the latest release is v0.9.6. (internal/syncer/pull_details_more.go:11, b72d4ae715d6)
• Failure mechanism and repair boundary: Pagination appends overlapping rows; JSON decoding preserves numeric IDs. Check persistence inserts rows under the existing unique(thread_id, name, details_url) constraint, so repeated named checks with the same nonempty URL collide. Consolidating equal provider IDs before persistence fixes that trigger without changing SQL conflict handling. (internal/store/schema.go:265, 1ceae73a8b9f)
• Regression coverage and reported execution: The complete new test file exercises the production HTTP client, syncer, and SQLite store across overlapping pages, large IDs, complete observation replacement, reopen/refresh, metadata-only preservation, distinct-ID rollback, neighboring PRs, cancellation, and recovery. The captured PR body reports failure on base and success after the fix, plus race-test success; tests were inspected but not executed during this read-only review. (internal/syncer/check_runs_test.go:48, 1ceae73a8b9f)
• Discussion limits the incident claim: The captured PR body explicitly says the original failed 224-row payload was not retained and a fresh 226-check census contained no duplicates or stored-key collisions. The patch establishes protection against repeated observations, not the historical incident's cause. No human review comments were supplied. (1ceae73a8b9f)
• Related merged work is complementary: fix(sync): preserve completed items when another hydration fails #200 provides per-item failure isolation; fix: stop repeated REST pagination links #194 rejects cyclic pagination links. Their bodies and current source establish different failure mechanisms, neither of which consolidates repeated IDs across otherwise advancing pages. (internal/github/client.go:404, b132e5b7816a)

Likely related people:

• vincentkoc: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
• Peter Steinberger: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rating scale

Score	Internal tier	Crab rank	Meaning
6/6	S	🦀 challenger crab	Exceptional readiness
5/6	A	🦞 diamond lobster	Very strong readiness
4/6	B	🐚 platinum hermit	Good normal PR; ordinary maintainer review
3/6	C	🦐 gold shrimp	Useful, but confidence is limited
2/6	D	🦪 silver shellfish	Proof or implementation needs work
1/6	F	🧂 unranked krab	Not merge-ready
N/A	NA	🌊 off-meta tidepool	Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

• ClawSweeper keeps one durable marker-backed review comment per issue or PR.
• Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
• A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
• PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
• Maintainers can also comment @clawsweeper review to request a fresh review only.
• Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
• Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
• Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

[vincentkoc]

Maintainer decision for ClawSweeper's merge-risk item, at unchanged head 1ceae73a8b9fa33224108ed1858872f446a329e9: accept the bounded repeated-provider-ID repair.

The native regression proves that repeated observations cause the SQLite collision on the base and that this ingestion repair fixes that trigger. The original failed payload was not retained, so its historical cause remains unproven. This PR does not claim to fix distinct provider IDs sharing the legacy name/details-URL key. Those collisions still roll back atomically and retain failure-ledger evidence, as the regression verifies.

Named follow-up: check-run storage identity for distinct provider IDs sharing the legacy key. Any broader storage-contract migration requires separate review and acceptance; it is not being folded into this repair.

This is the mitigation recommended by the review: consolidate identity at ingestion and preserve atomic failure handling. Exact-tree CI and Docker passed, as did native branch and independent source reviews. The risk decision is explicit; no check or review is waived.