feat: queue embeddings for live Discord events - #154
Conversation
|
Codex review: found issues before merge. Reviewed August 1, 2026, 7:47 PM ET / 23:47 UTC. ClawSweeper reviewWhat this changesAdds optional OpenAI embedding dimensions and queues embedding jobs from live Discord tail events, replayed failures, and periodic tail repair work. Merge readiness⛔ Blocked by patch quality or review findings - 6 items remain Keep open. The branch adds useful live-tail embedding support, but it changes the established opt-in contract: installations with embeddings enabled will begin enqueueing every live tail create, update, replay, and repair message after upgrade without an explicit tail opt-in. Priority: P2 Review scores
Verification
How this fits togetherDiscrawl's Discord tail continuously archives live message events into SQLite, while the embedding pipeline separately queues archive changes and later drains them in background batches. This PR connects the live-tail and repair paths to that embedding-job queue using the global embedding-enabled setting. flowchart LR
A[Discord live events] --> B[Live tail handler]
F[Embedding settings] --> B
B --> C[SQLite message archive]
C --> D[Embedding job queue]
D --> E[Background embed command]
B --> G[Periodic repair and failure replay]
Decision needed
Why: The branch changes a documented explicit-command opt-in into an upgrade-time global behavior change, so the permanent configuration contract requires product ownership rather than a mechanical review decision. Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Keep tail embedding queueing behind a dedicated, default-off tail opt-in, document the new scope, and cover both the existing default and the enabled path; alternatively, explicitly approve and document the broader global-setting contract. Do we have a high-confidence way to reproduce the issue? Not applicable: this is a feature PR, not a report of a failing current-main path. Source inspection and the added unit test establish the proposed queueing behavior; the unresolved issue is its upgrade contract. Is this the best way to solve the issue? No. Reusing the global enabled setting conflicts with the documented explicit 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 8141387a4f95. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (21 earlier review cycles; latest 8 shown)
|
|
Verified this branch and carried the useful implementation forward in #159, preserving the original The replacement adds the missing compatibility boundary: live event, failure replay, and repair queueing now require explicit Proof on the replacement:
Recommendation: close this PR as superseded by #159, then land #159 after its CI is green. |
|
Thanks @hannesrudolph for the original live-embedding work. The implementation and contributor credit are preserved in #159, which landed as |
Summary
Validation
go test ./...go vet ./...