fix: large archive listings fail at SQLite parameter limit - #238
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
clawsweeper
Bot
added
P2
|
Codex review: needs real behavior proof before merge. Reviewed September 15, 2026, 3:17 AM ET / 07:17 UTC. ClawSweeper reviewWhat this changesThe PR passes reply and mention lookup IDs as JSON arrays so large archive listings retain their rows and display names without exceeding SQLite’s parameter limit. Merge readiness⛔ Blocked before merge - 2 items remain This remains necessary: current main and v0.15.1 still expand hydration IDs into individual SQL parameters. The focused patch has no identified correctness defect. Priority: P2 Review scores
Verification
How this fits togetherDiscrawl reads archived Discord messages from local SQLite storage for CLI listings and terminal browsing. Its hydration queries add reply context and readable mention names before displaying those messages. flowchart TD
A[CLI listing or terminal browser] --> B[Select archived messages]
C[Local SQLite archive] --> B
B --> D[Collect reply and mention IDs]
D --> E[Bind IDs as JSON arrays]
E --> F[Look up context and display names]
C --> F
F --> G[Display requested messages]
Before merge
Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Keep hydration as parameterized, read-only queries that preserve complete ID sets and existing ordering without adding limits or migrations. Do we have a high-confidence way to reproduce the issue? Yes, from source: a listing of 32,767 messages creates that many bound parameters in current-main mention hydration. This review did not execute the reproduction. Is this the best way to solve the issue? Yes. A single bound JSON array removes the parameter-count failure across all four lookups while preserving string IDs, query predicates, and existing display behavior. AGENTS.md: not found in the target repository. Codex review notes: model internal, reasoning medium; reviewed against 0d365ac80195. LabelsLabel changes:
Label justifications:
EvidenceWhat 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
|
|
Merged as b1ea76e after all checks on head 8ec8e02 settled: 19 passed, two expected skips, no failures. Validation: The built baseline failed on the synthetic 32,767-message archive. The fixed binary returned every row through |
1 participant
What Problem This Solves
Large archive reads fail with
SQL logic error: too many SQL variablesonce a message, reply, or mention lookup contains more than 32,766 IDs. This breaksmessages --all, large--lastslices, and large TUI loads even when the initial message query succeeds.User Impact
Large local listings return every requested row with mention display names and reply context intact. Existing ordering, limits, and read-only access remain unchanged.
Why This Change Was Made
Bind each hydration ID set as one JSON array and expand it with SQLite's existing JSON support. This removes the parameter-count failure in all four hydration queries without truncating results or changing database schemas.
Evidence
too many SQL variables; the patched CLI returns all 32,767 rows throughmessages --all,messages --last 32767, andtui --limit 32767.make check.Proof uses synthetic local data; no Discord credentials or private archive content are required.
Built CLI output from the synthetic archive
Built
discrawl-beforefrom base0d365acanddiscrawl-afterfrom the reviewed patch. Each command used--config <synthetic-config> --json; the fixture has 32,767 matching messages plus one reply root outside the selected channel. The JSON below records actual exit codes and parsed output, with assertions checking counts, ordering, labels, and root presence.[ { "binary": "discrawl-before", "args": [ "messages", "--channel", "100000000000000001", "--all" ], "exit_code": 1, "stderr": "SQL logic error: too many SQL variables (1)" }, { "binary": "discrawl-after", "args": [ "messages", "--channel", "100000000000000001", "--all" ], "exit_code": 0, "rows": 32767, "first": "m00001", "last": "m32767", "display_content": "ping @Synthetic Reader #synthetic-channel @Readers", "reply_to_message_id": "root" }, { "binary": "discrawl-after", "args": [ "messages", "--channel", "100000000000000001", "--last", "32767" ], "exit_code": 0, "rows": 32767, "first": "m00001", "last": "m32767", "display_content": "ping @Synthetic Reader #synthetic-channel @Readers", "reply_to_message_id": "root" }, { "binary": "discrawl-after", "args": [ "tui", "--channel", "100000000000000001", "--limit", "32767" ], "exit_code": 0, "rows": 32768, "resolved_mentions": true, "reply_root_present": true } ]The regression command
GOWORK=off go test ./internal/store -run '^TestMessageHydrationBeyondSQLiteVariableLimit$' -count=1 -vfails in all four subtests on the base withtoo many SQL variables; after the fix it reports: