Skip to content

feat: queue embeddings for live Discord events - #154

Closed
hannesrudolph wants to merge 1 commit into
openclaw:mainfrom
hannesrudolph:codex/discrawl-openai-live-embeddings
Closed

feat: queue embeddings for live Discord events#154
hannesrudolph wants to merge 1 commit into
openclaw:mainfrom
hannesrudolph:codex/discrawl-openai-live-embeddings

Conversation

@hannesrudolph

Copy link
Copy Markdown
Member

Summary

  • propagate configured embedding dimensions to Crawlkit
  • queue embeddings for live tail messages, replayed failures, and tail repair
  • add an ordered pending-job index so large embedding backlogs drain without a full sort

Validation

  • go test ./...
  • go vet ./...

@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. labels Jul 30, 2026
@clawsweeper

clawsweeper Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Codex review: found issues before merge. Reviewed August 1, 2026, 7:47 PM ET / 23:47 UTC.

ClawSweeper review

What this changes

Adds 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
Reviewed head: 7fbd1ed6f1a1aad01b8e77632d0d1b6299751a81
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The implementation has useful coverage, but the unresolved P1 upgrade-contract regression prevents it from being quality-ready.
Proof confidence 🌊 off-meta tidepool Not applicable: Not applicable as a real-behavior merge gate because the author is a repository MEMBER; the stated Go test and vet validation remains supplemental to the compatibility review.
Patch quality 🦪 silver shellfish (2/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: Not applicable as a real-behavior merge gate because the author is a repository MEMBER; the stated Go test and vet validation remains supplemental to the compatibility review.
Evidence reviewed 5 items Documented opt-in contract: Current main documents embeddings as background enrichment that must not block the hot sync path, and specifies that sync --with-embeddings is the operation that writes embedding jobs.
Existing command boundary: The sync command exposes --with-embeddings with a false default and passes that explicit flag into SyncOptions.Embeddings; tail has no equivalent opt-in on current main.
Upgrade-changing branch line: The branch sets tail enqueueing directly from the global search.embeddings.enabled setting, which makes currently enabled installations take on the new live-tail workload without selecting it through the established sync flag.
Findings 1 actionable finding [P1] Preserve existing tail behavior by default
Security None None.

How this fits together

Discrawl'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]
Loading

Decision needed

Question Recommendation
Should enabling search.embeddings.enabled also opt existing installations into embedding every live tail and repair event, or should live tail embedding require its own default-off setting? Add a tail-specific opt-in: Preserve current behavior by default and add a clearly documented setting for operators who want live tail and repair events queued.

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

  • Preserve existing tail behavior by default (P1) - search.embeddings.enabled currently enables the provider while queueing is selected explicitly with sync --with-embeddings. This line makes every existing embedding-enabled tail installation enqueue live creates, updates, failure replays, and repair rows after upgrade. Add a default-off tail opt-in (or obtain explicit approval for this new global contract) and cover both behaviors.
  • Resolve merge risk (P1) - Merging as written changes upgrades for existing search.embeddings.enabled users: live tail traffic and periodic repair work will begin accumulating embedding jobs without the prior explicit sync --with-embeddings selection.
  • Complete next step (P2) - A maintainer must choose the permanent configuration and upgrade contract before a safe repair boundary can be defined.
  • Improve patch quality - Choose and implement the live-tail opt-in contract.
  • Improve patch quality - Add regression coverage for both default and opted-in tail queueing behavior.

Findings

  • [P1] Preserve existing tail behavior by default — internal/cli/cli.go:787
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Changed surface 13 files, +96/-6 lines The change spans configuration, live ingestion, replay, repair, SQLite indexing, tests, and user documentation, so the upgrade contract needs an explicit decision.

Merge-risk options

Maintainer options:

  1. Preserve the explicit opt-in boundary (recommended)
    Add a default-off live-tail embedding setting and regression coverage showing existing embedding-enabled installations do not enqueue tail jobs until it is selected.
  2. Accept the global setting expansion
    Approve the changed upgrade contract, document that enabled embeddings include live tail and repair traffic, and add coverage for that intentional behavior.

Technical review

Best 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 sync --with-embeddings queueing boundary; a default-off tail-specific opt-in is the safer maintainable path unless the broader contract receives explicit approval.

Full review comments:

  • [P1] Preserve existing tail behavior by default — internal/cli/cli.go:787
    search.embeddings.enabled currently enables the provider while queueing is selected explicitly with sync --with-embeddings. This line makes every existing embedding-enabled tail installation enqueue live creates, updates, failure replays, and repair rows after upgrade. Add a default-off tail opt-in (or obtain explicit approval for this new global contract) and cover both behaviors.
    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 8141387a4f95.

Labels

Label justifications:

  • P2: This is a bounded feature and upgrade-contract decision with meaningful effects for embedding-enabled installations, but no demonstrated current outage.
  • merge-risk: 🚨 compatibility: The branch repurposes an existing global setting to activate new live-tail queueing for current configurations after upgrade.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🌊 off-meta tidepool and patch quality is 🦪 silver shellfish.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: Not applicable as a real-behavior merge gate because the author is a repository MEMBER; the stated Go test and vet validation remains supplemental to the compatibility review.

Evidence

What I checked:

  • Documented opt-in contract: Current main documents embeddings as background enrichment that must not block the hot sync path, and specifies that sync --with-embeddings is the operation that writes embedding jobs. (README.md:866, 8141387a4f95)
  • Existing command boundary: The sync command exposes --with-embeddings with a false default and passes that explicit flag into SyncOptions.Embeddings; tail has no equivalent opt-in on current main. (internal/cli/admin_commands.go:119, 8141387a4f95)
  • Upgrade-changing branch line: The branch sets tail enqueueing directly from the global search.embeddings.enabled setting, which makes currently enabled installations take on the new live-tail workload without selecting it through the established sync flag. (internal/cli/cli.go:787, 7fbd1ed6f1a1)
  • Review continuity and provenance: Git blame identifies the proposed configuration bridge as introduced by Hannes Rudolph in the PR commit; comparing the current head to the previously reviewed head shows no change to this concern. (internal/cli/cli.go:787, 7fbd1ed6f1a1)
  • Current-main status: The proposed head is not an ancestor of current main, so the requested live-tail behavior is not already implemented there. (internal/syncer/tail.go:1, 8141387a4f95)

Likely related people:

  • Peter Steinberger: Current main's embedding contract and tail implementation are attributed to Peter in the release baseline; he also introduced the crawlkit embedding primitives used by this path. (role: embedding and tail-area contributor; confidence: high; commits: 4f3017c8188d, 40c787c54a73; files: README.md, internal/syncer/tail.go, internal/cli/admin_commands.go)
  • Hannes Rudolph: Hannes previously merged the tail failure replay work that this branch extends, and authored the current proposed live-embedding bridge. (role: recent tail recovery contributor; confidence: high; commits: d865b5687bba, 7fbd1ed6f1a1; files: internal/syncer/tail_replay.go, internal/syncer/tail.go, internal/cli/cli.go)

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 (21 earlier review cycles; latest 8 shown)
  • reviewed 2026-08-01T07:59:37.422Z sha 7fbd1ed :: needs maintainer review before merge. :: none
  • reviewed 2026-08-01T10:19:05.426Z sha 7fbd1ed :: needs maintainer review before merge. :: none
  • reviewed 2026-08-01T11:55:23.001Z sha 7fbd1ed :: needs maintainer review before merge. :: none
  • reviewed 2026-08-01T14:08:41.805Z sha 7fbd1ed :: needs maintainer review before merge. :: none
  • reviewed 2026-08-01T16:10:54.360Z sha 7fbd1ed :: found issues before merge. :: [P1] Preserve existing tail behavior by default
  • reviewed 2026-08-01T16:18:30.198Z sha 7fbd1ed :: found issues before merge. :: [P1] Keep live tail embeddings opt-in
  • reviewed 2026-08-01T18:44:22.290Z sha 7fbd1ed :: found issues before merge. :: [P1] Keep live tail embeddings opt-in
  • reviewed 2026-08-01T22:37:49.681Z sha 7fbd1ed :: found issues before merge. :: [P1] Preserve existing tail behavior by default

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. labels Jul 30, 2026
@steipete

steipete commented Aug 2, 2026

Copy link
Copy Markdown
Collaborator

Verified this branch and carried the useful implementation forward in #159, preserving the original 7fbd1ed commit and @hannesrudolph credit.

The replacement adds the missing compatibility boundary: live event, failure replay, and repair queueing now require explicit tail --with-embeddings, matching the established sync --with-embeddings contract. Existing installations with global embeddings enabled therefore keep their current tail behavior after upgrade.

Proof on the replacement:

  • full make check passed, including lint/security/vulnerability gates, 85.1% coverage, race tests, CLI smoke, and six-platform snapshot builds
  • the built CLI exposes and accepts tail --with-embeddings
  • isolated replay-only runs succeed both with and without the opt-in
  • AutoReview is clean for the repair and the complete two-commit branch

Recommendation: close this PR as superseded by #159, then land #159 after its CI is green.

@steipete

steipete commented Aug 2, 2026

Copy link
Copy Markdown
Collaborator

Thanks @hannesrudolph for the original live-embedding work. The implementation and contributor credit are preserved in #159, which landed as 146e72773ab887d1e7a156f1f62a384fa93a5293 with the explicit default-off opt-in and existing-archive regression. Closing this PR as superseded, as described in the earlier routing note.

@steipete steipete closed this Aug 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants