Skip to content

feat: resolve embedding credentials natively from the OS keyring - #220

Merged
hannesrudolph merged 1 commit into
mainfrom
feat/native-embedding-keyring
Sep 11, 2026
Merged

hannesrudolph merged 1 commit into
mainfrom
feat/native-embedding-keyring

Conversation

@hannesrudolph

Copy link
Copy Markdown
Member
Additional instructions

Keep Allow edits from maintainers enabled.

What Problem This Solves

Fixes an issue where users running Discrawl under a service manager need an external credential-loading process for embeddings even though Discord bot credentials already support the OS keyring. Requiring an environment export makes an otherwise native capture/embedding service harder to operate.

Why This Change Was Made

Add explicit api_key_source = "keyring" selection with service/account fields, using the existing go-keyring dependency and Crawlkit's existing per-provider API-key option. The embedding commands, semantic queries, live worker, and doctor use the selected source. Absent fields preserve environment-based behavior without unexpected keyring prompts or fallback.

Live provider initialization shares one pending lookup. Locked keyring prompts cannot block capture or worker cancellation, and unavailable credentials pause work without consuming attempts. Initialization retries after one minute; successful providers retain the key until restart. Keyring API keys do not undergo Discord bot-token normalization.

User Impact

Operators can run Discrawl directly under launchd or another service manager with both credentials resolved natively. Existing environment-based and credential-free provider configurations continue working. Keyring values are not written to configuration, process arguments, status, or the environment. Raw credential lookup errors and probe failures are not exposed in diagnostics.

Evidence

  • Focused tests cover legacy/env selection, explicit keyring selection without environment fallback, service/account normalization, empty/missing/locked credentials, credential-free providers, HTTP authorization, safe diagnostics, and config round trips.
  • A real background-worker test over a temporary archive keeps capture running through a missing key, restores the stub keyring item, and verifies both messages embed after the normal retry timer.
  • A blocked-lookup test proves cancellation returns promptly, repeated attempts share one outstanding lookup, timeout does not exhaust retry attempts, and later completion recovers.
  • Full Go race suite, lint, module tidy, and unchanged dependency-file checks passed. Tests stub OS keyring access and never use a live archive.
  • Final structured autoreview completed with no actionable findings.

@clawsweeper

clawsweeper Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. 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. labels Sep 11, 2026
@clawsweeper

clawsweeper Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed September 11, 2026, 5:59 PM ET / 21:59 UTC.

ClawSweeper review

What this changes

Adds explicit OS keyring credentials for embedding commands, semantic search, and diagnostics, with cancellable background initialization and unchanged environment defaults.

Merge readiness

Ready for maintainer review

This PR provides a distinct capability absent from main and v0.14.1. No blocking correctness or security defect was found; the MEMBER-authored contribution also remains protected from automated closure.

Priority: P2
Reviewed head: 2e342278faf669b785030e5d93f8f8251a0f293f

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused additive implementation with meaningful compatibility and recovery coverage and no identified blocking defect.
Proof confidence 🌊 off-meta tidepool Not applicable: The MEMBER-authored PR is exempt from the external proof gate. Supplied tests exercise the provider HTTP path and worker recovery with stubbed keyring access; they do not establish native OS-keyring behavior.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: The MEMBER-authored PR is exempt from the external proof gate. Supplied tests exercise the provider HTTP path and worker recovery with stubbed keyring access; they do not establish native OS-keyring behavior.
Evidence reviewed 7 items Introduced change and scope: Reviewed the pinned base-to-head diff and recovered the truncated configuration and test content from local files. The change touches 11 files; dependency manifests, workflows, and archive schemas are unchanged.
Explicit selection preserves existing defaults: Empty or env selection retains the existing provider constructor. Keyring selection requires a successful lookup and passes the credential through CrawlKit's existing WithAPIKey option. This direct call establishes the dependency contract relevant to this review.
Published dependency supports per-provider credentials: At v0.16.1, WithAPIKey overrides environment resolution without modifying the process environment. CheckProvider lacks a credential option; the PR's adapter preserves its provider-specific probe selection. The tag resolves to commit 0bb18e9865a2b8ecbd8e94924a8f6c1dbf7233f8.
Findings None None.
Security None None.

How this fits together

Discrawl turns archived Discord messages and search queries into vectors through configurable embedding providers. This change supplies provider credentials and lets background capture continue while credential initialization waits or retries.

flowchart TD
  A[Embedding configuration] --> B{Credential source}
  B --> C[Environment variable]
  B --> D[Selected OS keyring item]
  C --> E[Embedding provider]
  D --> E
  F[Archived messages and search queries] --> E
  E --> G[Stored vectors and semantic search]
  D --> H[Background initialization wait or retry]
Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test growth Production +123 net lines; tests +235 lines Production growth implements the stated credential adapter and cancellable initialization; tests cover selection, compatibility, and recovery.

Technical review

Best possible solution:

Keep keyring selection explicit, preserve environment and credential-free configurations, and reuse the existing provider transport with bounded background initialization.

Do we have a high-confidence way to reproduce the issue?

Not applicable: this adds a credential-source capability rather than repairing a defined existing contract; source inspection confirms the capability is absent from main.

Is this the best way to solve the issue?

Yes: the explicit opt-in adapter uses existing keyring and provider APIs while preserving defaults, and the asynchronous lookup addresses blocking initialization without changing capture ownership.

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning medium; reviewed against 66fbb8fb51b0.

Labels

Label changes:

  • add P2: Native credential selection is a bounded operational improvement for optional embeddings.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The MEMBER-authored PR is exempt from the external proof gate. Supplied tests exercise the provider HTTP path and worker recovery with stubbed keyring access; they do not establish native OS-keyring behavior.

Label justifications:

  • P2: Native credential selection is a bounded operational improvement for optional embeddings.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The MEMBER-authored PR is exempt from the external proof gate. Supplied tests exercise the provider HTTP path and worker recovery with stubbed keyring access; they do not establish native OS-keyring behavior.

Evidence

What I checked:

  • Introduced change and scope: Reviewed the pinned base-to-head diff and recovered the truncated configuration and test content from local files. The change touches 11 files; dependency manifests, workflows, and archive schemas are unchanged. (2e342278faf6)
  • Explicit selection preserves existing defaults: Empty or env selection retains the existing provider constructor. Keyring selection requires a successful lookup and passes the credential through CrawlKit's existing WithAPIKey option. This direct call establishes the dependency contract relevant to this review. (internal/cli/embedding_provider.go:14, 2e342278faf6)
  • Published dependency supports per-provider credentials: At v0.16.1, WithAPIKey overrides environment resolution without modifying the process environment. CheckProvider lacks a credential option; the PR's adapter preserves its provider-specific probe selection. The tag resolves to commit 0bb18e9865a2b8ecbd8e94924a8f6c1dbf7233f8. (embed/provider.go, 0bb18e9865a2)
  • Secret handling and cancellation: The resolver rejects missing or empty credentials with fixed diagnostic strings. Background initialization shares one pending lookup, waits outside the mutex, and returns on cancellation; credential acquisition failures produce a paused worker failure with a one-minute retry. (internal/cli/embedding_worker.go:29, 2e342278faf6)
  • Validation supplied by the contribution: The complete captured body at sourceRevision ba189b215c0404853c87865d893b4c37779b3ac55b8dbca615aa6f199a89e45e reports race-suite and lint success. Inspected tests cover environment preservation, no fallback, HTTP authorization, configuration round trips, pending-lookup cancellation, and worker recovery through a temporary archive. OS keyring access is stubbed; no native-keyring runtime transcript is supplied. This review did not execute tests or repository helpers. (internal/cli/embedding_provider_test.go:26, 2e342278faf6)
  • Current main and release still lack the capability: The live main ref matches the pinned base. Its EmbeddingsConfig and the v0.14.1 configuration expose APIKeyEnv but no embedding keyring selector. Searches for embedding keyring and embedding credentials found no replacement implementing this capability. (internal/config/config.go:100, 66fbb8fb51b0)

Likely related people:

  • Hannes Rudolph: Raw commit 8f0e93b adds internal/cli/embedding_worker.go:45 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: 8f0e93b808ae; files: internal/cli/embedding_worker.go)
  • Peter Steinberger: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

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.

@hannesrudolph
hannesrudolph merged commit 0f35f71 into main Sep 11, 2026
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant