fix(wiretap): retry unresolved cache entries - #190
Open
Nicolas0315 wants to merge 2 commits into
Open
Conversation
Contributor
|
🦞👀 Pull request received. I will update this pull request when review starts. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
clawsweeper
Bot
added
merge-risk: 🚨 message-delivery
Contributor
|
Codex review: needs real behavior proof before merge. Reviewed September 3, 2026, 9:39 AM ET / 13:39 UTC. ClawSweeper reviewWhat this changesThis PR makes unresolved Discord Desktop cache entries retryable and adds a v1-to-v2 file-index migration intended to preserve archive event history during upgrades. Merge readiness⛔ Blocked before merge - 4 items remain Keep open: the prior event-replay finding is addressed, but the v1 migration still treats legacy unresolved checkpoints as imported, so the central recovery behavior remains absent. Current main does not contain this PR’s changes. Priority: P1 Review scores
Verification
How this fits togetherWiretap reads Discord Desktop cache files into DisCrawl’s local SQLite archive. Its persisted file index determines which unchanged cache entries are skipped, retried, and allowed to append archive events. flowchart LR
A[Discord Desktop cache] --> B[Wiretap importer]
B --> C[Persisted file index]
C --> D{Checkpoint status}
D -->|Imported| E[Skip unchanged file]
D -->|Unresolved| F[Retry cache entry]
B --> G[SQLite messages]
B --> H[Event history]
Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Use an explicit legacy-migration state that retries v1 checkpointed entries without replaying existing events, then prove both legacy recovery and event preservation against a real Desktop cache import. Do we have a high-confidence way to reproduce the issue? Yes, from source: seed a v1 index entry produced by the old unresolved checkpoint path, add later channel metadata, then import on the PR head; the migration marks that entry imported and the unchanged cache file is skipped. Is this the best way to solve the issue? No. The patch correctly avoids replaying events for known imported entries, but it cannot distinguish those from the unresolved entries that v1 also wrote as imported, so it does not deliver the requested legacy recovery. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: not found in the target repository. Codex review notes: model internal, reasoning high; reviewed against 0853d40e0525. LabelsLabel changes:
Label justifications:
EvidenceAcceptance criteria:
What I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (11 earlier review cycles; latest 8 shown)
|
clawsweeper
Bot
added
rating: 🧂 unranked krab
レビュー指摘 [P1] Preserve event history during the v2 migration への対応。 ## 問題 `wiretapFileIndexScope` を v1 から v2 へ変えると、`loadScanState` が読むキーが 変わるため v1 の checkpoint が全て消えたように見え、取り込み済みキャッシュ ファイルまで再走査される。解析済みメッセージは `AppendEvent: true` を立て、 store はそのミューテーションごとに無条件でイベントを挿入するため、message は upsert されて件数が変わらない一方 `message_events` だけが重複追記される。 既存アーカイブのイベント履歴がアップグレードで複製される。 ## 対応 イベント追記を抑止するのではなく、索引を引き継ぐ形にした。v2 の索引が未作成の ときだけ v1 を読み、**imported の checkpoint だけを** v2 へ移す (`migrateFileIndexFromV1`)。 - 取り込み済みファイルは checkpoint が残るので再走査されない → イベント複製なし - v1 は解決できなかった項目も imported として記録していたため区別できないが、 本PRの `checkpointScannedCandidates` が今後は skipped で記録するので、 以降の未解決項目は正しく再試行される - v1 に無いファイルは通常どおり走査される ## 回帰テスト `TestImportMigratesV1FileIndexWithoutReplayingEvents` を追加。v1 相当の取り込みを 行い、索引を v1 キーへ移して v2 を空にした「アップグレード直前」を作り、再走査後に `FilesScanned == 0` / `message_events` の件数が不変であることを固定する。 ミューテーション検証: 移行を無効化して `state.previous` を空マップへ戻すと `FilesScanned` が 0 ではなく 1 になり本テストが落ちることを確認済み。 検証: go build ./... 成功、gofmt 差分なし、go test ./... = 11 パッケージ全 ok。 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Author
|
@clawsweeper re-review [P1] Preserve event history during the v2 migration を修正し、シード付きアップグレード回帰テストと実挙動トレースを追加しました（PR本文の
head: |
Contributor
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
rating: 🦪 silver shellfish
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Why
checkpointScannedCandidatesmarked 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/discorddesktoppragma integrity_check=okThe full Windows package run still hits the pre-existing
TestDesktopPathAndImportHelpersAPPDATA expectation failure on both machines; the three tests touched by this change pass on both.Update 2026-09-03 — ClawSweeper review addressed
[P1] Preserve event history during the v2 migration — fixed
The blocking defect was real. Bumping
wiretapFileIndexScopeto v2 changes the keyloadScanStatereads, so a database holding only the v1 index looks like it has nocheckpoints at all and rescans already-imported cache files. Parsed messages set
AppendEvent: trueand the store inserts an event per mutation without dedup, so theupgrade left message rows unchanged (they upsert) while duplicating
message_events.Rather than suppressing event appends, the index is now carried forward. When the v2
index is absent,
migrateFileIndexFromV1reads the v1 key and copies over only theimportedcheckpoints:checkpointScannedCandidatesnow records those asskipped, so unresolved entriesare retried from here on — which is the whole point of the PR
internal/discorddesktop/import.go—wiretapFileIndexScopeV1added,loadScanStatefalls back to the migration,
migrateFileIndexFromV1added.Seeded upgrade regression test — added
TestImportMigratesV1FileIndexWithoutReplayingEventsininternal/discorddesktop/import_pipeline_test.go:message_eventscountpre-upgrade shape
Importand assertsFilesScanned == 0,FilesUnchanged == 1, and that themessage_eventscount is unchangedReal behavior proof
Mutation check — the new test does catch the defect. Reverting only the migration
(
state.previous = map[string]fileFingerprint{}instead of the migrated index) makes it fail:So the assertion is bound to the migration, not to incidental state.
Not verified
This is a synthetic-fixture proof over a real SQLite archive created by
store.Open, not acapture from a Windows production copy. The upgrade path, the recovered-entry retry, and the
unchanged prior event count are all exercised against the real store and real import
pipeline; what is not covered is a live Discord Desktop cache directory.
🤖 Generated with Claude Code