fix(wiretap): avoid creating archives during dry runs - #195
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
clawsweeper
Bot
added
P2
|
Codex review: needs maintainer review before merge. Reviewed September 4, 2026, 7:39 AM ET / 11:39 UTC. ClawSweeper reviewWhat this changesThe PR defers Wiretap archive setup until after flag parsing so dry-run previews do not create archives or runtime directories, while optional statistics use read-only archive access. Merge readiness✅ Ready for maintainer review The focused fix is correct and remains necessary because current main still initializes writable storage before Wiretap parses dry-run flags. Submitted built-CLI evidence and the new regression matrix support normal maintainer review. Priority: P2 Review scores
Verification
How this fits togetherWiretap scans local Discord desktop cache artifacts and imports discovered message metadata into a local archive. The CLI layer parses options, selects read-only or writable storage, then returns import and coverage results as terminal or JSON output. flowchart LR
A[Desktop cache files] --> B[Wiretap command]
C[Command flags] --> B
B --> D{Requested mode}
D -->|Dry run| E[Preview import]
D -->|Dry run with statistics| F[Read-only archive coverage]
D -->|Import| G[Writable archive]
E --> H[Terminal or JSON output]
F --> H
G --> H
Before mergeNone. Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Merge the deferred storage-selection path so all Wiretap aliases preserve dry-run preview semantics while retaining explicit read-only coverage for existing archives. Do we have a high-confidence way to reproduce the issue? Yes. The previous dispatcher unconditionally opened writable storage before flag parsing, and the submitted built-CLI comparison exercises the affected dry-run path against current main and the candidate. Is this the best way to solve the issue? Yes. Moving storage selection after parsing is the narrowest solution: ordinary previews use no archive, statistics use the existing read-only path, and real imports retain the established writer lock. AGENTS.md: not found in the target repository. Codex review notes: model internal, reasoning high; reviewed against b262f754180d. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (1 earlier review cycle)
|
clawsweeper
Bot
added
rating: 🦞 diamond lobster
|
ClawSweeper status: review started. I am starting a fresh review of this pull request: fix(wiretap): avoid creating archives during dry runs This is item 1/1 in the current shard. Shard 0/1. This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking. Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted. |
1 participant
Wiretap opened and migrated a writable archive before parsing
--dry-run. A preview consequently created a database, lock files and runtime directories; even a preview without coverage failed if an unrelated archive was malformed.Parse flags after loading configuration, then select storage for the requested operation. Dry runs without statistics do not open the archive.
--dry-run --statsreads an existing archive without migrations and reports empty coverage when absent. Actual imports keep the existing writer lock, and explicit--dry-run=falsestill imports. This coverswiretap,tap,cache-import, and watched previews.SQLite read-only coverage can create WAL/SHM sidecars for an existing archive. Existing database contents remain unchanged, and normal SQLite locking keeps current WAL data visible. The command documentation records this limit.
Validation: full CLI package tests, focused Wiretap race tests, golangci-lint, module tidy/verification, vet, staticcheck, gosec, deadcode, govulncheck and docs build passed. Full independent P0–P2 review found no actionable issue. The latest main is integrated by a normal merge; the final tree exactly matches the combined source already tested with PR #190. New-head CI is pending at
bf1599ecb1877f47991a9cb2d29fc2d6d69e9d7c.Actual built CLI proof used synthetic cache files, isolated configuration, and real SQLite; no Discord credentials or network access. Eighteen invocations compared main with this candidate:
wiretap,tap,cache-import, with and without--stats--dry-run --statsping sqlite: file is not a database (26)--dry-run --dry-run=false--dry-run --stats --watch-every 1sThe new regression tests first failed on the old dispatch path for all six alias/stat combinations and for malformed-archive previews, then passed with the fix. No schema, dependency, runtime floor or release change.
Curated actual CLI output at
eecbab545664bea6a95d7ef47ec927f7aae53b8ffordiscrawl --config fixture.toml --json wiretap --path fixture-cache --dry-run --stats(selected fields; synthetic paths and timestamps omitted):{ "exit": 0, "import": { "files_scanned": 1, "messages": 1, "dry_run": true }, "coverage": { "guilds": [], "totals": { "message_count": 0 } } }Before: the same command creates
archive/archive.db, writer-lock files,runtime-cache/andruntime-logs/. After: a full recursive file/hash snapshot contains only the original fixture config and cache payload; all three runtime/archive directories remain absent. The watched invocation prints three such samples, with zero deltas after the first, and exits 0 on SIGTERM.Integration with the landed retry fix preserves the original five-file dry-run change. The combined tree passes the CLI, importer and store suites plus the same 18-command actual dry-run proof. A fresh full P0–P2 review of the integrated candidate found no actionable issues.