fix(wiretap): retry unresolved cache entries

[Nicolas0315]

Summary

• keep unresolved cache fingerprints in the skipped state so later imports can retry them when channel metadata becomes available
• bump the private wiretap file-index scope to v2 so entries checkpointed as imported by older builds receive one migration rescan
• cover repeated retry, later resolution, and no duplicate message events

Why

checkpointScannedCandidates marked permanently unresolved candidates as imported. An unchanged cache file was therefore never reconsidered after bot sync or later cache artifacts supplied its channel metadata.

Verification

• go test ./...
• go vet ./...
• go test -race ./internal/discorddesktop
• targeted tests passed on Windows amd64 / Go 1.26.6 on two independent machines
• production-copy canary: cache/DB gap 1,055 -> 870, 185 previously missing rows recovered, duplicate message IDs 0, pragma integrity_check = ok

The full Windows package run still hits the pre-existing TestDesktopPathAndImportHelpers APPDATA expectation failure on both machines; the three tests touched by this change pass on both.

[clawsweeper]

🦞👀
ClawSweeper picked this up.

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

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[clawsweeper]

Codex review: needs real behavior proof before merge. Reviewed August 30, 2026, 6:06 PM ET / 22:06 UTC.

ClawSweeper review

What this changes

The branch keeps unresolved Discord Desktop cache files retryable and bumps Wiretap’s private file-index key so entries checkpointed by v1 are rescanned once.

Merge readiness

⛔ Blocked until stronger real behavior proof is added - 7 items remain

This PR addresses a real Wiretap retry gap, but its v2 index migration will replay existing cache messages and append duplicate local event-history rows. It also needs inspectable after-fix proof for the migration path before merge.

Priority: P1
Reviewed head: 73e18c08291affd3bb32d6c6554e3850020f4159

Review scores

Measure	Result	What it means
Overall readiness	🧂 unranked krab (1/6)	The retry fix is focused, but an upgrade-time event-history replay remains a blocking correctness defect and the claimed production proof is not inspectable.
Proof confidence	🦪 silver shellfish (2/6)	Needs stronger real behavior proof before merge: The changed production owner is Wiretap’s persisted file index and SQLite archive writer. The body reports a Windows production-copy canary, but the supplied review snapshot has no inspectable after-fix trace or artifact showing a v1-to-v2 upgrade rescan, recovered message, and unchanged prior event history; post a redacted terminal trace or log after correcting the migration. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality	🧂 unranked krab (1/6)	1 actionable review finding remain.

Verification

Check	Result	Evidence
Real behavior	Needs proof	Needs stronger real behavior proof before merge: The changed production owner is Wiretap’s persisted file index and SQLite archive writer. The body reports a Windows production-copy canary, but the supplied review snapshot has no inspectable after-fix trace or artifact showing a v1-to-v2 upgrade rescan, recovered message, and unchanged prior event history; post a redacted terminal trace or log after correcting the migration. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed	6 items	Introduced migration trigger: The introduced v2 scope makes loadScanState read a different sync-state key, so a database with only the v1 index has no prior fingerprints and rescans its cache files. Duplicate-event path: Parsed Wiretap messages set AppendEvent: true, and the store unconditionally inserts an event for each such mutation; an upgrade rescan therefore adds another event for already persisted messages. Regression coverage gap: The added pipeline test proves a newly resolvable skipped entry, but it does not seed a v1 file index and existing event, run the v2 migration, and assert that the event count remains unchanged.
Findings	1 actionable finding	[P1] Preserve event history during the v2 migration
Security	None	None.

How this fits together

Wiretap reads Discord Desktop cache files and imports classifiable messages into Discrawl’s local SQLite archive. The file index decides which cache files are rescanned, while imported message mutations may also append archive event-history rows.

flowchart LR
  A[Discord Desktop cache] --> B[Wiretap file index]
  B --> C{Fingerprint needs scan?}
  C -->|yes| D[Message classification]
  D --> E[SQLite messages]
  D --> F[Message event history]
  C -->|unresolved| G[Skipped fingerprint]
  G --> B

Loading

Before merge

• Add real behavior proof - Needs stronger real behavior proof before merge: The changed production owner is Wiretap’s persisted file index and SQLite archive writer. The body reports a Windows production-copy canary, but the supplied review snapshot has no inspectable after-fix trace or artifact showing a v1-to-v2 upgrade rescan, recovered message, and unchanged prior event history; post a redacted terminal trace or log after correcting the migration. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
• Preserve event history during the v2 migration (P1) - Changing the scope makes every v1 checkpoint appear absent, so previously imported cache files are scanned again. Parsed messages request event appends, and the store inserts those events without deduplication; upgrading therefore duplicates message_events even though message IDs are upserted. Keep the retry behavior but suppress event replay for the v1-to-v2 rescan, and add a seeded upgrade regression test.
• Resolve merge risk (P1) - Existing Wiretap archives with v1 checkpoints can gain a second message_events row for every rescanned cached message, changing local event-history consumers despite message IDs remaining unique.
• Resolve merge risk (P1) - The claimed production-copy result is not accompanied by inspectable evidence for the upgrade path or event-history preservation.
• Complete next step (P2) - The migration replay defect has a narrow source-defined repair and focused regression boundary; contributor-provided real-run proof remains separately required before merge.
• Improve patch quality - Prevent v1-to-v2 rescans from appending duplicate existing Wiretap events and add a seeded upgrade regression test.
• Improve patch quality - Post a redacted real-run trace showing recovered rows and unchanged pre-existing event count; updating the PR body should trigger re-review, or a maintainer can comment @clawsweeper re-review if it does not.

Findings

• [P1] Preserve event history during the v2 migration — internal/discorddesktop/import.go:121

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Patch footprint	production +2/-2, tests +19/-4 across 3 files	A small migration-key change has archive-wide replay consequences, so upgrade coverage matters more than patch size.

Merge-risk options

Maintainer options:

1. Preserve history during the v2 rescan (recommended)
   Add a narrowly scoped migration path that imports recovered rows without replaying existing Wiretap events, and cover it with a seeded v1-index upgrade test.
2. Pause the migration change
   Hold the v2 scope bump until the project intentionally accepts event replay as an upgrade behavior and documents that contract.

Copy recommended automerge instruction

@clawsweeper automerge

Special instructions:
Preserve prior Wiretap message-event history during the v1-to-v2 file-index migration and add focused upgrade regression coverage.

Technical review

Best possible solution:

Retain retryable skipped fingerprints, but rescan entries inherited from the v1 index without appending new events for already archived messages, backed by an upgrade regression test and a redacted real archive trace.

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

Yes—source-reproducible: seed an archive with a v1 index, a cached message, and its existing Wiretap event, then run the v2 importer. The new scope forces a rescan and the current write path appends another event.

Is this the best way to solve the issue?

No—the retry behavior is appropriate, but the unconditional scope migration is not safe until it preserves event history for existing archives.

Full review comments:

• [P1] Preserve event history during the v2 migration — internal/discorddesktop/import.go:121
  Changing the scope makes every v1 checkpoint appear absent, so previously imported cache files are scanned again. Parsed messages request event appends, and the store inserts those events without deduplication; upgrading therefore duplicates message_events even though message IDs are upserted. Keep the retry behavior but suppress event replay for the v1-to-v2 rescan, and add a seeded upgrade regression test.
  Confidence: 0.98

Overall correctness: patch is incorrect
Overall confidence: 0.98

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning high; reviewed against 4c29bb75ef7f.

Labels

Label justifications:

• P1: The introduced migration can duplicate existing local event history for active Wiretap users on upgrade.
• merge-risk: 🚨 compatibility: Changing the persisted file-index scope alters upgrade behavior for every existing v1 checkpoint.
• merge-risk: 🚨 message-delivery: The rescan can append duplicate message-event records even though message rows are upserted by ID.
• rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🦪 silver shellfish and patch quality is 🧂 unranked krab.
• status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The changed production owner is Wiretap’s persisted file index and SQLite archive writer. The body reports a Windows production-copy canary, but the supplied review snapshot has no inspectable after-fix trace or artifact showing a v1-to-v2 upgrade rescan, recovered message, and unchanged prior event history; post a redacted terminal trace or log after correcting the migration. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

Acceptance criteria:

• [P1] go test ./internal/discorddesktop.
• [P1] go test ./internal/store.
• [P1] go test ./...

What I checked:

• Introduced migration trigger: The introduced v2 scope makes loadScanState read a different sync-state key, so a database with only the v1 index has no prior fingerprints and rescans its cache files. (internal/discorddesktop/import.go:121, 73e18c08291a)
• Duplicate-event path: Parsed Wiretap messages set AppendEvent: true, and the store unconditionally inserts an event for each such mutation; an upgrade rescan therefore adds another event for already persisted messages. (internal/store/write.go:300, 73e18c08291a)
• Regression coverage gap: The added pipeline test proves a newly resolvable skipped entry, but it does not seed a v1 file index and existing event, run the v2 migration, and assert that the event count remains unchanged. (internal/discorddesktop/import_pipeline_test.go:207, 73e18c08291a)
• Current-main necessity: Fetched current main still uses the v1 scope and marks unresolved scanned candidates imported, so the central retry fix is not already implemented on the default branch. (internal/discorddesktop/import.go:121, 4c29bb75ef7f)
• Feature-history routing: Recent history identifies Peter Steinberger as the author of the earlier skipped-fingerprint recheck and subsequent Wiretap durability work, making him the strongest current-history routing candidate. (internal/discorddesktop/import.go:249, 055095aadfc2)
• Supplied proof is not inspectable: The PR body claims a Windows production-copy canary, but the reviewed snapshot provides no terminal trace, redacted log, or artifact demonstrating a v1-to-v2 rescan while preserving existing event history. (73e18c08291a)

Likely related people:

• 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.

History

Review history (10 earlier review cycles; latest 8 shown)

• reviewed 2026-08-28T05:39:06.228Z sha 73e18c0 :: needs real behavior proof before merge. :: [P1] Prevent event replay during the v2 rescan
• reviewed 2026-08-28T23:02:16.238Z sha 73e18c0 :: needs real behavior proof before merge. :: [P1] Prevent event replay during the v2 rescan
• reviewed 2026-08-29T07:02:34.528Z sha 73e18c0 :: needs real behavior proof before merge. :: [P1] Prevent event replay during the v2 rescan
• reviewed 2026-08-29T09:01:28.956Z sha 73e18c0 :: needs real behavior proof before merge. :: [P1] Prevent event replay during the v2 rescan
• reviewed 2026-08-29T14:59:23.767Z sha 73e18c0 :: needs real behavior proof before merge. :: [P1] Prevent event replay during the v2 rescan
• reviewed 2026-08-29T22:04:25.920Z sha 73e18c0 :: needs real behavior proof before merge. :: [P1] Avoid appending duplicate events during the v2 rescan
• reviewed 2026-08-30T04:52:08.308Z sha 73e18c0 :: needs real behavior proof before merge. :: [P1] Preserve event history during the v2 migration
• reviewed 2026-08-30T13:11:07.200Z sha 73e18c0 :: needs real behavior proof before merge. :: [P1] Preserve event history during the v2 migration