LCORE-2664: Spike — BYOK auto-import from Confluence#2178
LCORE-2664: Spike — BYOK auto-import from Confluence#2178max-svistunov wants to merge 3 commits into
Conversation
Design spike for LCORE-788 (auto import of content for BYOK from Confluence; scope reduced 2026-06 to Confluence only) under the epic readout LCORE-2652. The spike doc records: - Settled decision S1: the importer lives in lightspeed-core/rag-content, following the byok-pdf precedent (Decision 3); the PoC required zero rag-content library changes. - Strategic decisions S2-S5 for PM confirmation: periodic full re-crawl + rebuild + rollout as the refresh baseline (no hot-reload), DB file as the primary artifact with OCI image optional, a Kubernetes CronJob reference manifest as the deployable automation, and no new lightspeed-stack configuration surface. - Technical decisions T1-T8 for tech-lead confirmation: fetch client (atlassian-python-api), v1 body.export_view rendered HTML, Cloud API-token + DC PAT auth, space/CQL selection, attachments out of scope, manifest-driven citation metadata, embedding-model pinning, and CQL-delta + version-skip + ID-enumeration-diff change detection. - PoC results from a real Confluence Cloud crawl (20-page space, read-only token): end-to-end build, correct retrieval with per-page URL citations, incremental re-run with zero re-fetches/re-embeds. - Proposed JIRAs: one epic with six tickets (e2e feature files first, step definitions, fetch stage, importer command + packaging, CronJob manifest + admin guide, lightspeed-stack docs) plus one incidental bug ticket; parses cleanly with dev-tools/file-jiras.sh. The spec doc is the permanent reference: requirements R1-R10, use cases, architecture (fetch stage -> existing pipeline -> artifact -> CronJob refresh), acceptance test surface driving the e2e kickoff ticket, and implementation suggestions with concrete rag-content insertion points.
Moderate-ambition PoC validating the spike's core mechanism against real Confluence Cloud (redhat.atlassian.net, space RHAT, 20 pages, read-only API token): - confluence_fetch.py: full + incremental crawl via v1 REST body.export_view; writes per-page HTML, manifest.json (id, title, canonical URL, version) and state.json (watermark + versions); incremental mode combines a CQL lastmodified delta with page-version comparison and detects deletions by full ID enumeration diff. - confluence_processor.py: custom_processor-pattern build driver with ConfluenceMetadataProcessor resolving citation URL/title from the manifest (hermetic_build=True for auth-gated URLs); wires the docling HTMLReader via file_extractor and scopes the corpus with required_exts. - confluence_test_page.py: helper for an editable test page; unused in the end (space creation is 403-forbidden for regular users), kept as evidence of the read-only-token constraint. - poc-results/: sanitized evidence only (crawl stats, retrieval output with titles/URLs/scores, conversion-structure sample); raw crawled content is Red Hat internal and deliberately excluded. Validated: end-to-end build (3.8 MB llamastack-faiss store), correct top-3 retrieval with per-page URL citations, incremental re-run with zero body fetches and zero re-embeds (2 API calls instead of 21). Both directories are review-time artifacts and will be removed from the branch before merge per docs/contributing/howto-organize-poc-output.md.
WalkthroughThis change documents an automated BYOK Confluence importer and adds a PoC for REST crawling, incremental synchronization, HTML-to-vector processing, citation metadata, persisted-store querying, and validation evidence. ChangesBYOK Confluence Import
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant ConfluenceAPI
participant confluence_fetch
participant confluence_processor
participant VectorStore
confluence_fetch->>ConfluenceAPI: Enumerate pages and changed IDs
ConfluenceAPI-->>confluence_fetch: Page IDs, versions, and exported HTML
confluence_fetch->>confluence_processor: HTML files and manifest.json
confluence_processor->>VectorStore: Chunk, embed, and save index
🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
I'm happy with your recommendations, just in S4 it would be great if we also provided a non-kubernetes reference. something that teams w/ podman-only deployments could leverage |
Per PM review on the spike PR (sbunciak, 2026-07-21): teams with podman-only deployments need a non-Kubernetes automation reference. Spike doc: Decision S4 gains option D (CronJob reference manifest plus a podman Quadlet reference — .container + .timer systemd units driving the same importer container, plain-cron fallback noted) and D becomes the recommendation, PM-confirmed. The scheduled-refresh JIRA stub's scope and acceptance criteria now cover both deployment shapes. Spec doc kept in sync: R9 and its acceptance-test-surface row cover the Quadlet reference, the rollout plan names systemctl restart alongside kubectl rollout restart, the key-files table adds the Quadlet example units, and the changelog records the amendment.
@sbunciak Incorporated -- S4 now recommends the k8s cronjob reference plus a podman-only quaslet reference (.container & .timer systemd units, same importer container), and as fallback we have a normal cron one-liner. PTAL: https://github.com/max-svistunov/lightspeed-stack/blob/lcore-2664-spike-byok-confluence-import/docs/design/byok-confluence-import/byok-confluence-import-spike.md?plain=1#L97-L121 The suggested jira and the requirement R9 are updated accordingly. With this update, I take it that I have your LGTM. TYVM! |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
docs/design/byok-confluence-import/byok-confluence-import.md (3)
117-120: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake incremental state filter-aware.
R3 allows CQL/label selection, but
state.jsonis described only aspage-id → version. If the configured filter changes or a page no longer matches, a space-wide inventory can leave the previously imported page inmanifest.jsonand the rebuilt artifact. Persist the effective selection/filter identity and perform deletion diffs against the filtered inventory.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/design/byok-confluence-import/byok-confluence-import.md` around lines 117 - 120, Update the state.json design to persist the effective CQL/label filter identity alongside the page-id → version map, and make incremental deletion comparisons use the inventory produced by that filter. Ensure filter changes invalidate or reconcile prior state so pages no longer selected are removed from manifest.json and the rebuilt artifact.
234-240: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFail closed when manifest metadata is missing.
ConfluenceMetadataProcessorcurrently falls back to the HTML filename when a manifest entry lacksurlortitle(seepoc/confluence_processor.py, Lines 26-71). That can produce chunks without the canonical Confluence URL required by R4 while reporting a successful build. Validate manifest coverage before processing, or make missing metadata a fatal error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/design/byok-confluence-import/byok-confluence-import.md` around lines 234 - 240, Update ConfluenceMetadataProcessor and the surrounding pipeline to fail closed when any manifest entry lacks the canonical url or title, instead of falling back to the HTML filename. Validate manifest coverage before processing, or propagate a fatal error during metadata processing, and prevent save(index, output) from reporting a successful build when required metadata is missing.
147-150: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDefine the partial-crawl failure threshold and default. The configuration section only lists CLI flags/env vars for Confluence auth, embedding, and output; it never defines the failure-ratio option, default value, or whether the crawl aborts before publishing the artifact.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/design/byok-confluence-import/byok-confluence-import.md` around lines 147 - 150, Update the Confluence import configuration section to define the per-page failure-ratio option, its default value, and the behavior when the threshold is exceeded, including that the crawl aborts before publishing the artifact. Ensure these definitions match the partial-crawl handling described in the run-summary and failure policy.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/byok-confluence-import/byok-confluence-import-spike.md`:
- Around line 108-113: Apply one atomic artifact-publication contract across
both automation references: in
docs/design/byok-confluence-import/byok-confluence-import-spike.md lines
108-113, specify that Quadlet publishes versioned artifacts and atomically
switches the active symlink or path before restarting Lightspeed; in
docs/design/byok-confluence-import/byok-confluence-import.md lines 207-212, add
the same invariant to the rollout plan and Quadlet acceptance criteria.
---
Outside diff comments:
In `@docs/design/byok-confluence-import/byok-confluence-import.md`:
- Around line 117-120: Update the state.json design to persist the effective
CQL/label filter identity alongside the page-id → version map, and make
incremental deletion comparisons use the inventory produced by that filter.
Ensure filter changes invalidate or reconcile prior state so pages no longer
selected are removed from manifest.json and the rebuilt artifact.
- Around line 234-240: Update ConfluenceMetadataProcessor and the surrounding
pipeline to fail closed when any manifest entry lacks the canonical url or
title, instead of falling back to the HTML filename. Validate manifest coverage
before processing, or propagate a fatal error during metadata processing, and
prevent save(index, output) from reporting a successful build when required
metadata is missing.
- Around line 147-150: Update the Confluence import configuration section to
define the per-page failure-ratio option, its default value, and the behavior
when the threshold is exceeded, including that the crawl aborts before
publishing the artifact. Ensure these definitions match the partial-crawl
handling described in the run-summary and failure policy.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c99b8f16-bfa7-4d14-9a15-298bcc0e9aff
📒 Files selected for processing (2)
docs/design/byok-confluence-import/byok-confluence-import-spike.mddocs/design/byok-confluence-import/byok-confluence-import.md
📜 Review details
⚠️ CI failures not shown inline (1)
GitHub Actions: E2E Tests / 0_E2E server mode _ ci _ group 2.txt: LCORE-2664: Spike — BYOK auto-import from Confluence
Conclusion: failure
the response is �[0m�[90m�[1m200�[0m�[90m # tests/e2e/features/steps/common_http.py:27�[0m
�[2A �[32mThen �[0m�[32mThe status code of the response is �[0m�[32m�[1m200�[0m�[90m # tests/e2e/features/steps/common_http.py:27 0.000s�[0m
�[90mAnd �[0m�[90mThe body of the "�[0m�[90m�[1mtool_calls�[0m�[90m" field of the response is the following�[0m�[90m # tests/e2e/features/steps/common_http.py:323�[0m
"""
[
{
"name": "list_skills",
"type": "function_call"
}
]
"""
�[10A �[31mAnd �[0m�[31mThe body of the "�[0m�[31m�[1mtool_calls�[0m�[31m" field of the response is the following�[0m�[90m # tests/e2e/features/steps/common_http.py:323 0.000s�[0m
"""
[
{
"name": "list_skills",
"type": "function_call"
}
]
"""
ASSERT FAILED: No matching element found in list for schema item {'name': 'list_skills', 'type': 'function_call'}
�[36mAnd �[0m�[36mThe body of the "tool_results" field of the response is the following�[0m�[90m # None�[0m
"""
[
{
"status": "success",
"content": "{\"echo\":\"Echo back the user's input exactly as provided. Use when a user asks to echo, repeat, or mirror text.\"}",
"type": "function_call_output"
}
]
"""
�[36mAnd �[0m�[36mThe token metrics have increased�[0m�[90m # None�[0m
----
CAPTURED STDOUT: scenario
⏱ Attempt 1/12 - waiting for lightspeed-stack...
⏱ Attempt 2/12 - waiting for lightspeed-stack...
⏱ Attempt 3/12 - waiting for lightspeed-stack...
⏱ Attempt 4/12 - waiting for lightspeed-stack...
⏱ Attempt 5/12 - waiting for lightspeed-stack...
⏱ Attempt 6/12 - waiting for lightspeed-stack...
⏱ Attempt 7/12 - waiting for lightspeed-stack...
⏱ Attempt 8/12 - waiting for lightspeed-stack...
🧰 Additional context used
📓 Path-based instructions (1)
**/*
📄 CodeRabbit inference engine (Custom checks)
**/*: Flag meaningful O(n^2)+ algorithms on non-trivial inputs, including handlers and Kubernetes list operations.
Flag N+1 patterns that list items and then query once per item, including Kubernetes API and database access.
Flag expensive work inside loops, including API calls, JSON parsing, and regex compilation.
Flag unbounded growth in caches, watchers, or buffers when eviction or limits are missing.
Flag missing pagination or limits on list operations and API endpoints.
Flag secrets or tokens logged in plaintext or hardcoded in source.
Flag API endpoints missing authentication or authorization.
Flag injection vulnerabilities, including SQL injection, command injection, and path traversal.
Flag sensitive data leaked in API responses, WebSocket messages, or logs.
Flag Kubernetes Secrets and Red Hat secrets missing OwnerReferences.Never commit secrets or keys; use environment variables for sensitive data.
Files:
docs/design/byok-confluence-import/byok-confluence-import.mddocs/design/byok-confluence-import/byok-confluence-import-spike.md
🔇 Additional comments (1)
docs/design/byok-confluence-import/byok-confluence-import-spike.md (1)
427-436: 🔒 Security & PrivacyVerify ownership and secret handling in the Kubernetes reference.
The eventual CronJob/Secret manifest should use an
OwnerReferencefor deployment-owned Kubernetes Secrets, and the Podman fallback must keep credentials out of command lines and logs.As per coding guidelines, Kubernetes Secrets must not be missing OwnerReferences and sensitive values must not be committed or exposed.
Source: Coding guidelines
| **Recommendation**: D — CronJob reference manifest + admin guide in | ||
| rag-content, documenting the DB-file flow (shared volume + rollout | ||
| trigger), **plus a podman-only reference**: Quadlet systemd units | ||
| (`.container` + `.timer`) running the same importer container on a | ||
| schedule, with a plain-cron one-liner noted as the minimal fallback. | ||
| Both references drive the identical container image and flags — the |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Apply one atomic artifact-publication contract to both automation references.
The Quadlet path must use the same versioned-output and atomic-switch behavior as the Kubernetes path before restarting Lightspeed.
docs/design/byok-confluence-import/byok-confluence-import-spike.md#L108-L113: specify versioned artifact publication and atomic symlink/rename for Quadlet.docs/design/byok-confluence-import/byok-confluence-import.md#L207-L212: add that invariant to the rollout plan and Quadlet acceptance criteria.
📍 Affects 2 files
docs/design/byok-confluence-import/byok-confluence-import-spike.md#L108-L113(this comment)docs/design/byok-confluence-import/byok-confluence-import.md#L207-L212
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/design/byok-confluence-import/byok-confluence-import-spike.md` around
lines 108 - 113, Apply one atomic artifact-publication contract across both
automation references: in
docs/design/byok-confluence-import/byok-confluence-import-spike.md lines
108-113, specify that Quadlet publishes versioned artifacts and atomically
switches the active symlink or path before restarting Lightspeed; in
docs/design/byok-confluence-import/byok-confluence-import.md lines 207-212, add
the same invariant to the rollout plan and Quadlet acceptance criteria.
| | B. llama-index-readers-confluence (MIT) | Highest-level; brings heavy attachment deps (pytesseract, pdf2image, docx2txt…); hides pagination/state | | ||
| | C. Raw REST (requests/httpx) | No new dep; we own retry/backoff/pagination/Cloud-DC differences | | ||
|
|
||
| **Recommendation**: A. B's attachment machinery is out of scope (T5) and |
There was a problem hiding this comment.
yes, agree with A. The code included as PoC seems sufficient. TY
| that affects Confluence "Export to PDF" does not apply to the HTML path. | ||
| Risk: `export_view` is not in the v2 API — Cloud requires v1 endpoints | ||
| (deprecation risk over the feature lifetime; DC keeps this surface). | ||
|
|
There was a problem hiding this comment.
yup, I tried A a while ago (against RH Confluence) at it looked ok.
| |--------|-------------| | ||
| | A. Cloud email+API-token (Basic) and DC PAT (Bearer) | Two single-secret headless modes, from env/K8s Secret | | ||
| | B. Also OAuth 2.0 3LO | Interactive consent + rotating refresh tokens; built for marketplace apps, poor fit for a CronJob | | ||
|
|
There was a problem hiding this comment.
probably both options will be required by customers, but it might be implemented via future ticket. We can stick with A today.
| PDF/Office attachments can later route through the byok-pdf pipeline | ||
| (LCORE-2090 line of work) as a follow-up; images/diagrams are dropped by | ||
| the HTML→Markdown conversion anyway. | ||
|
|
| the consuming service; same-dimension model drift fails *silently*. | ||
|
|
||
| **Recommendation**: The importer requires an explicit model name; default to | ||
| HF-id resolution (`-md '' -mn <id>` convention) so the artifact is |
There was a problem hiding this comment.
+1. IMHO the Hugging Face resolution policy is standard now (or at least semi-standard)
| re-fetched nor re-embedded. Validated in the PoC for the no-change and | ||
| new-state cases; live update/delete exercised by the e2e tickets against a | ||
| mock Confluence. | ||
|
|
3 participants
LCORE-2664: Spike — BYOK auto-import from Confluence
Spike deliverable for LCORE-788 (parent feature; scope reduced to
Confluence-only, 2026-06). Presented at the LCORE-2652 epic readout.
What's in this PR
Design docs (
docs/design/byok-confluence-import/):byok-confluence-import-spike.md(use the "Outline" button) — the spike: decisions, PoC results, proposed JIRAs, background researchbyok-confluence-import.md(use the "Outline" button) — feature spec — changeable based on final decisions, will be kept in the repo long-termPoC code (removed before merge, with
poc-results/):poc/confluence_fetch.py— full + incremental Confluence crawl (rendered HTML + citation manifest + sync state)poc/confluence_processor.py— ConfluenceMetadataProcessor + existing pipeline gluepoc-results/— sanitized evidence from a real Confluence Cloud runMain findings
lastmodified+ page-version comparison skipped all unchanged pages (2 API calls instead of 21, no re-embeds); deletions need a full ID enumeration diff (CQL cannot see them).body.export_view) converts cleanly through docling — sidestepping the known Confluence-PDF-export letter-spacing problem; the endpoint is v1-only on Cloud (risk noted in T2).llamastack-faissstore fails to open with its own generatedllama-stack.yaml(registration conflict; breaksquery_rag.pyout of the box) — proposed as an incidental JIRA.For reviewers
@sbunciak — strategic decisions S2–S5 (S1, importer placement in rag-content, is settled and recorded for context — L35-L52):
@tisnik — technical decisions T1–T8: L133-L234 — fetch client, body format, auth, selection, attachments, citation plumbing, embedding pinning, change detection; each with options table and confidence.
JIRA review (both): Proposed JIRAs — one Epic, six tickets (e2e feature files first), plus one incidental. Parses cleanly with
dev-tools/file-jiras.sh --parse-only.Doc structure note: The decision and JIRA sections of the spike doc are where your input is needed. They link to background sections later in the doc — read those if you need more context on a specific point, but it is optional.
Pre-merge cleanup
Before merge (after decisions are confirmed and JIRAs are filed):
docs/design/byok-confluence-import/poc/andpoc-results/LCORE-????placeholders with filed ticket keysTools used to create PR
Closes LCORE-2664
Related: LCORE-788, LCORE-256, LCORE-2652
Summary by CodeRabbit