[Yogesh-Kisslay]

Closes #96.

Ingests a company's entire Confluence + Jira into a pgvector knowledge base and
serves it via RAG:

• Onboarding — role-based 7-day plans + per-day content + final quiz, grounded
  in the company's own docs
• Knowledge chat — conversational Q&A over Confluence + Jira, with citations and
  people-analytics (who did what)

Highlights

• Local embeddings (bge-small, pgvector HNSW); no external embedding key needed
• Resilient large-tenant Atlassian ingestion (rate-limit + token-refresh safe; 130k+ issues)
• Source-filtered retrieval fix (exact KNN so Confluence-sparse topics still return docs)
• Onboarding/knowledge logging (startup migration no longer disables app loggers)

Tests: 1382 passing, 99% coverage.

Summary by CodeRabbit

• New Features
  • Added onboarding experiences (plans/day content/quizzes) plus public plan viewing/sharing.
  • Introduced Confluence/Jira connections with background ingest/reconcile, webhook handling, and indexed-status UX.
  • Added knowledge-base chat with persistent threads, structured responses, and source citations.
  • Enabled local text embeddings with configurable Atlassian OAuth and indexing settings.
• Bug Fixes
  • Improved citation grounding and structured block robustness (including Mermaid and dotted tech-term matching).
  • Hardened LLM parsing and retry/error handling for truncation and transient failures.
• Chores
  • Expanded Docker/local setup (pgvector), environment/docs, migrations, and significantly increased automated test coverage.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

This PR adds a company-scoped Atlassian knowledge platform with Confluence/Jira ingestion, local embeddings and retrieval, onboarding plan/day/quiz generation, persisted knowledge chat, and supporting migrations, scripts, docs, and UI surfaces. It also includes a separate syllabi/sample-app UI cleanup.

Changes

Knowledge Platform rollout

Layer / File(s)	Summary
Config, docs, and runtime setup .coveragerc, .dockerignore, .env.example, .gitignore, Dockerfile, README.md, KNOWLEDGE_PLATFORM_DESIGN.md, LARGE_INGESTION_CHALLENGE.md, config.py, requirements.txt, docker-compose.yml, docs/LOCAL_SETUP.md, scripts/*	Adds backend/container settings, Atlassian OAuth/webhook/embedding environment variables, design docs, helper scripts, database startup, and dependency updates.
Database schema migrations alembic/env.py, alembic/versions/*	Adds and updates Alembic revisions for companies, connections, onboarding, document storage, ingestion jobs, knowledge chats, and related schema fixes.
Company resolution and auth exposure services/company.py, models/user.py, controllers/auth.py, services/user.py, middleware/auth.py, main.py, models/__init__.py, ui/src/store/authStore.ts, ui/src/components/nav-user.tsx, tests/test_company.py, tests/test_user_service.py	Links users to companies, exposes company data in auth/UI, allows OAuth/webhook bypasses, and reaps stuck jobs on startup.
Onboarding and knowledge models onboarding/models/*, onboarding/schemas/*	Defines SQLModel tables and request schemas for the onboarding and knowledge features.
Ingestion, embeddings, and retrieval onboarding/services/confluence.py, onboarding/services/embeddings.py, onboarding/services/chunking.py, onboarding/services/retrieval.py, onboarding/controllers/confluence.py, onboarding/routes/confluence.py, routes/__init__.py, services/encryption.py, services/content_validator.py, services/llm.py, tests/test_chunking_embeddings.py, tests/test_retrieval.py, tests/test_content_blocks.py, tests/test_content_validator.py	Implements Atlassian connect/callback, page/issue ingestion, local embeddings, webhook freshness, reconciliation, and hybrid retrieval with people analytics.
Onboarding generation and scoring onboarding/services/generation.py, onboarding/prompts/onboarding.py, onboarding/services/onboarding.py, onboarding/controllers/onboarding.py, onboarding/routes/onboarding.py, tests/test_generation_retry.py, tests/test_onboarding.py	Generates onboarding plans, day content, and quizzes from retrieved docs, with retry/error handling and persistence/scoring.
Knowledge chat service and API onboarding/prompts/knowledge.py, onboarding/services/knowledge.py, onboarding/controllers/knowledge.py, onboarding/routes/knowledge.py, tests/test_knowledge.py	Implements doc-grounded chat responses with citations, people-analytics routing, and persisted chat/message history.
Connections, knowledge, and onboarding UI ui/src/app/plugins/connections/*, ui/src/app/plugins/knowledge/index.tsx, ui/src/app/plugins/onboarding/*, ui/src/components/app-sidebar.tsx, ui/src/routes/index.tsx	Adds pages for connecting Atlassian, monitoring ingestion, chatting with the knowledge base, and taking onboarding plans/quizzes.
Estimated code review effort: 5 (Critical)	~150 minutes

Sequence Diagram(s)

sequenceDiagram
  participant UI
  participant OnboardingRoute
  participant OnboardingService
  participant Retrieval
  participant LLM
  UI->>OnboardingRoute: POST /onboarding/generate
  OnboardingRoute->>OnboardingService: generate_plan(role, company)
  OnboardingService->>Retrieval: retrieve_context(company_id, query)
  OnboardingService->>LLM: generate_onboarding_plan(docs_text)
  LLM-->>OnboardingService: 7-day plan
  OnboardingService-->>UI: plan + days

Loading

sequenceDiagram
  participant WebhookRoute
  participant ConfluenceService
  participant DocumentPage
  participant DocumentChunk
  WebhookRoute->>ConfluenceService: verify secret + handle_webhook
  ConfluenceService->>DocumentPage: fetch/upsert page
  ConfluenceService->>DocumentChunk: re-chunk and embed content

Loading

Changes

Syllabi and sample app UI cleanup

Layer / File(s)	Summary
Sample app and syllabi UI refactors ui/src/app/plugins/sampleApp/index.tsx, ui/src/app/plugins/syllabi/block-renderer.tsx, ui/src/app/plugins/syllabi/detail.tsx, ui/src/app/plugins/syllabi/quiz.tsx	Removes unused sample-app syllabus UI, refactors inline token parsing and Mermaid sanitization, adds code highlighting, and adjusts syllabus progress and quiz wiring.

Estimated code review effort: 5 (Critical) | ~150 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	⚠️ Warning	Large parts of the PR add knowledge chat, Atlassian ingestion, and platform infrastructure beyond the onboarding-only issue.	Split the knowledge chat and Atlassian platform work into separate PRs or link the additional issue(s) covering that scope.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly captures the main change: a Confluence/Jira RAG platform with onboarding and knowledge chat.
Linked Issues check	✅ Passed	The PR delivers a curated, role-based onboarding flow grounded in company documentation, matching issue #96's requirements.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feature/issue-96-onboarding

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 14

Note

Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.

🟡 Minor comments (8)

ui/src/app/plugins/syllabi/block-renderer.tsx-27-27 (1)

27-27: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Markdown link parsing truncates URLs containing ).

The URL segment https?:\/\/[^)]+ stops at the first closing parenthesis, so links whose URL contains a literal ) (common in Confluence/Jira/Wiki page URLs) are cut short — the href becomes a broken URL and the trailing )... leaks into the text. This path renders Atlassian-sourced knowledge/onboarding content, so such URLs are realistic. Consider balancing a single level of parens or falling back to raw text when the closing ) doesn't terminate the token.

Also applies to: 39-41

🤖 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 `@ui/src/app/plugins/syllabi/block-renderer.tsx` at line 27, The Markdown link
matcher in block-renderer.tsx is too narrow because the pattern used by the
rendering logic stops URLs at the first closing parenthesis, which breaks links
containing literal parentheses. Update the link parsing in the block-rendering
flow (the regex assigned to pattern and any related handling around the markdown
tokenization) so it can safely accept balanced parentheses or otherwise avoid
splitting valid Atlassian URLs, and fall back to treating the token as plain
text when the closing parenthesis is not the actual end of the link.

alembic/versions/e2f3a4b5c6d7_drop_onboarding_docs.py-20-22 (1)

20-22: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Avoid dropping onboarding_docs without a data migration

alembic/versions/e2f3a4b5c6d7_drop_onboarding_docs.py:20-22 drops the legacy table outright, and downgrade() only recreates an empty schema. If any deployed database still has onboarding_docs rows, that data is lost on upgrade; add a backfill/export step or confirm the table is empty before removing it.

🤖 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 `@alembic/versions/e2f3a4b5c6d7_drop_onboarding_docs.py` around lines 20 - 22,
The upgrade in drop_onboarding_docs() is deleting onboarding_docs outright
without preserving existing rows, so update this migration to include a data
migration or safety check before calling op.drop_table; use the existing
upgrade() and downgrade() flow to backfill/export legacy data or verify the
table is empty, and keep the table removal only after the data path is handled.

scripts/test_webhook.sh-40-46 (1)

40-46: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win

Use mktemp instead of a PID-based temp file name.

/tmp/_wh_body.$$ is predictable; a local attacker could pre-create/symlink it (CWE-377). Trivial to harden even for a dev-only script.

🔒 Proposed fix

 post() {  # post <label> <url> <json> ; prints the HTTP status
   local label="$1" url="$2" body="$3" code
-  code="$(curl -s -o /tmp/_wh_body.$$ -w '%{http_code}' -X POST "$url" \
+  local tmpfile
+  tmpfile="$(mktemp)"
+  code="$(curl -s -o "$tmpfile" -w '%{http_code}' -X POST "$url" \
     -H 'Content-Type: application/json' -d "$body" || echo "000")"
-  printf '  %-42s -> %s  %s\n' "$label" "$code" "$(cat /tmp/_wh_body.$$ 2>/dev/null)"
-  rm -f "/tmp/_wh_body.$$"
+  printf '  %-42s -> %s  %s\n' "$label" "$code" "$(cat "$tmpfile" 2>/dev/null)"
+  rm -f "$tmpfile"
 }

🤖 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 `@scripts/test_webhook.sh` around lines 40 - 46, In the `post` helper in
`test_webhook.sh`, replace the predictable `/tmp/_wh_body.$$` temp file with a
`mktemp`-created path and use that variable consistently for both the `curl`
output and the `cat`/cleanup steps. Keep the helper’s behavior the same, but
ensure the temporary body file is created securely and removed afterward.

Source: Linters/SAST tools

scripts/test_webhook.sh-50-71 (1)

50-71: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win

Send webhook secrets in a header instead of the URL. The routes already accept X-Webhook-Secret, so these test calls can avoid leaking secrets through logs, history, and proxies.

🤖 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 `@scripts/test_webhook.sh` around lines 50 - 71, The webhook test calls are
passing secrets in the query string instead of the supported X-Webhook-Secret
header, which can leak them through logs and history. Update the post
invocations in test_webhook.sh for the Jira and Confluence cases (and the
wrong-secret auth check) to send the secret via the header expected by the
webhook routes, keeping the request bodies and endpoints the same.

ui/src/app/plugins/onboarding/detail.tsx-332-337 (1)

332-337: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Quiz attempt persistence failure is silent.

If postRequest throws or success is false, the catch does nothing — the user still sees a "submitted"/pass-fail screen (already computed locally) even though the attempt was never recorded server-side.

🤖 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 `@ui/src/app/plugins/onboarding/detail.tsx` around lines 332 - 337, The quiz
attempt save in the onboarding detail flow is failing silently because the
`postRequest` result is only handled on success and the `catch` block ignores
errors. Update the attempt submission logic in `detail.tsx` around the quiz
submit handler so failures from `postRequest` or a false `success` are surfaced
to the user and the UI does not imply the attempt was persisted. Use the
`setAttempts` and `setSubmitting` flow to keep the local state consistent, and
ensure the submit path in the quiz attempt handler clearly distinguishes between
local pass/fail computation and server persistence.

ui/src/app/plugins/onboarding/index.tsx-154-165 (1)

154-165: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Silent failure on plan delete.

Same pattern as in knowledge/index.tsx's deleteChat — a failed delete gives no user feedback; the dialog just closes as if it succeeded.

🤖 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 `@ui/src/app/plugins/onboarding/index.tsx` around lines 154 - 165, The delete
flow in handleDeleteConfirm is swallowing errors, so a failed plan deletion
closes the dialog without any user feedback. Update the catch path in
handleDeleteConfirm (and any related delete state handling in
ui/src/app/plugins/onboarding/index.tsx) to surface a visible error message or
notification instead of leaving the UI unchanged. Follow the same pattern used
by knowledge/index.tsx’s deleteChat so users can tell the delete request failed.

ui/src/app/plugins/knowledge/index.tsx-95-99 (1)

95-99: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Silent failure on chat delete.

If api.delete throws, the function just returns with no user feedback — the chat stays in the list but the user gets no indication the delete failed.

🔔 Suggested fix

-        try { await api.delete(`/py/knowledge/chats/${id}`) } catch { return }
+        try { await api.delete(`/py/knowledge/chats/${id}`) } catch {
+            toast({ variant: "destructive", title: "Error", description: "Failed to delete chat." })
+            return
+        }

🤖 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 `@ui/src/app/plugins/knowledge/index.tsx` around lines 95 - 99, The deleteChat
flow currently swallows api.delete failures and exits silently, leaving the UI
unchanged with no feedback. Update deleteChat to handle the catch path
explicitly in the knowledge plugin, using the existing api.delete call and
surrounding state updates to surface a user-visible error or retry prompt
instead of immediately returning. Keep the successful path unchanged so chats
are only removed from setChats and activeId/newChat are updated after a
confirmed delete.

ui/src/app/plugins/onboarding/detail.tsx-854-863 (1)

854-863: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Silent failure on share toggle.

catch { /* leave unchanged */ } gives no feedback if enabling/disabling the public link fails; the button just reverts state with no explanation.

🔔 Suggested fix

-        try {
-            await api.patch(`/py/onboarding/${plan.id}/share?enable=${next}`)
-            setShareEnabled(next)
-        } catch { /* leave unchanged */ }
-        finally { setTogglingShare(false) }
+        try {
+            await api.patch(`/py/onboarding/${plan.id}/share?enable=${next}`)
+            setShareEnabled(next)
+        } catch {
+            toast({ variant: "destructive", title: "Error", description: "Failed to update sharing." })
+        }
+        finally { setTogglingShare(false) }

🤖 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 `@ui/src/app/plugins/onboarding/detail.tsx` around lines 854 - 863, The share
toggle in handleToggleShare is silently swallowing API failures, so users get no
feedback when /py/onboarding/${plan.id}/share fails. Update the catch path in
handleToggleShare to surface the error through the existing UI pattern used in
this component (for example, a toast or inline error state) and keep the
shareEnabled value unchanged unless the request succeeds; use the same plan,
setTogglingShare, and setShareEnabled flow to locate the fix.

🧹 Nitpick comments (23)

ui/src/app/plugins/syllabi/detail.tsx (1)

2-2: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Use highlight.js/lib/common or highlight.js/lib/core here

import hljs from "highlight.js" pulls in all supported languages and bloats the client bundle. If you only need the usual common set, switch to highlight.js/lib/common; if you only highlight a few known languages, use highlight.js/lib/core and register just those grammars.

🤖 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 `@ui/src/app/plugins/syllabi/detail.tsx` at line 2, The import in the syllabi
detail component is pulling in the full highlight.js package and unnecessarily
increasing the client bundle. Update the import in detail.tsx from the top-level
highlight.js entry to either highlight.js/lib/common for the standard language
set or highlight.js/lib/core if only a few grammars are needed, and register
only the languages used by the highlighting logic in the relevant
component/module.

KNOWLEDGE_PLATFORM_DESIGN.md (1)

27-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Add a language hint to the fenced diagram block.

Static analysis flags this as an unlabeled fenced code block (MD040).

🤖 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 `@KNOWLEDGE_PLATFORM_DESIGN.md` around lines 27 - 33, The fenced diagram block
is unlabeled and triggers MD040; update the markdown in the knowledge design
document so the diagram fence includes an appropriate language hint, using the
diagram block itself as the reference point rather than changing its content.

Source: Linters/SAST tools

services/company.py (2)

57-72: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Return type can theoretically be None despite the -> Company annotation.

If the IntegrityError branch's re-query (Line 70) doesn't find a row (e.g., another session's transaction rolled back after the constraint conflict, or an isolation-level edge case), get_or_create_company returns None while typed as Company, and callers (e.g., services/user.py's create_user, which dereferences company.id) would hit an AttributeError rather than a clear failure.

Consider raising an explicit error if the refetch still comes back empty, to fail loudly instead of propagating None downstream.

♻️ Suggested fix

         except IntegrityError:  # concurrent create — fetch the winner
             session.rollback()
-            return session.exec(select(Company).where(Company.domain == key)).first()
+            winner = session.exec(select(Company).where(Company.domain == key)).first()
+            if winner is None:
+                raise RuntimeError(f"Company creation race for key={key!r} did not resolve.")
+            return winner

🤖 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 `@services/company.py` around lines 57 - 72, The get_or_create_company flow can
still return None from the IntegrityError retry path even though it is annotated
to return Company, which can break callers like create_user when they access
company.id. Update get_or_create_company to use the refetch result after
session.rollback() and, if no Company is found, raise an explicit error instead
of returning None; keep the change localized around the IntegrityError branch
and the final return in get_or_create_company.

---

20-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Generic-email allowlist is a maintenance surface.

_GENERIC_EMAIL_DOMAINS is a hardcoded set; providers not listed (e.g., regional webmail like qq.com, 163.com, fastmail.com) would be incorrectly treated as corporate domains, pooling unrelated users into one company. Not a bug introduced by this PR, but worth tracking as the list will need ongoing curation.

🤖 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 `@services/company.py` around lines 20 - 42, The hardcoded
_GENERIC_EMAIL_DOMAINS allowlist in services.company.Company is a maintenance
risk because missing providers will be misclassified as corporate domains.
Update the domain handling in Company to use a centralized, easily extensible
source for generic mail providers, and add the missing common webmail domains
(for example regional and provider-hosted mail services) so the list stays
accurate over time.

README.md (1)

224-229: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Document EMBEDDING_DIMENSIONS alongside EMBEDDING_MODEL.

config.py requires EMBEDDING_DIMENSIONS to stay in sync with EMBEDDING_MODEL and the document_chunks pgvector column width, but this table doesn't mention the variable at all. Add a row so operators overriding EMBEDDING_MODEL know they may also need to set EMBEDDING_DIMENSIONS (and re-run the relevant migration).

📝 Proposed addition

 | `EMBEDDING_MODEL` | No | Local embedding model (default: `BAAI/bge-small-en-v1.5`, 384-dim) |
+| `EMBEDDING_DIMENSIONS` | No | Must match the embedding model's output width and the `document_chunks` pgvector column (default: `384`) |

🤖 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 `@README.md` around lines 224 - 229, Update the configuration table in README
to include EMBEDDING_DIMENSIONS alongside EMBEDDING_MODEL, since config.py
depends on it matching the chosen model and the document_chunks pgvector width.
Add a clear row near EMBEDDING_MODEL in the docs so operators know that
overriding EMBEDDING_MODEL may also require setting EMBEDDING_DIMENSIONS and
re-running the related migration. Reference the existing EMBEDDING_MODEL entry
and the surrounding environment-variable table to place the new documentation
correctly.

config.py (2)

111-124: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Overriding ATLASSIAN_OAUTH_SCOPES can silently break refresh.

If an operator sets ATLASSIAN_OAUTH_SCOPES without including offline_access, no refresh token will be issued, and _refresh_access_token (onboarding/services/confluence.py) will fail once the initial access token expires (~1 hour). Consider validating that overridden scopes still contain offline_access, or warn/log at startup if it's missing.

💡 Proposed guard

 ATLASSIAN_OAUTH_SCOPES = _strip_opt("ATLASSIAN_OAUTH_SCOPES") or _DEFAULT_ATLASSIAN_SCOPES
+if "offline_access" not in ATLASSIAN_OAUTH_SCOPES.split():
+    raise ValueError(
+        "ATLASSIAN_OAUTH_SCOPES must include 'offline_access' or refresh tokens won't be issued."
+    )

🤖 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 `@config.py` around lines 111 - 124, The ATLASSIAN_OAUTH_SCOPES override in
config.py can remove offline_access and silently break token refresh later in
_refresh_access_token. Add startup validation where ATLASSIAN_OAUTH_SCOPES is
derived from _DEFAULT_ATLASSIAN_SCOPES to ensure any overridden value still
includes offline_access, and if it does not, either reject it with a clear error
or emit a prominent warning/log before the connector starts.

---

136-141: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Add a startup guard for embedding width drift

EMBEDDING_MODEL and EMBEDDING_DIMENSIONS are only documented as needing to match the document_chunks column; no runtime check ties the model’s output width to the configured dimension. A mismatch will fail later during inserts or similarity search, so the embedding path should assert the vector length up front.

🤖 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 `@config.py` around lines 136 - 141, Add a runtime startup guard for embedding
width drift by checking that the configured embedding model’s output size
matches EMBEDDING_DIMENSIONS before any inserts or search use it. Use the
existing EMBEDDING_MODEL and EMBEDDING_DIMENSIONS settings in config.py and
place the assertion where the embedding path is initialized so the app fails
fast with a clear error if the vector length does not match the document_chunks
schema.

alembic/versions/a5b6c7d8e9f0_add_onboarding_quiz_attempt.py (1)

24-35: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Consider indexing plan_id (and possibly user_id).

plan_id is a FK with ondelete="CASCADE" but Postgres doesn't auto-index FK columns. Without an index, cascading deletes from onboarding_plans and lookups (e.g. attempt history) will scan sequentially as the table grows.

♻️ Proposed fix

     op.create_table(
         "onboarding_quiz_attempts",
         sa.Column("id", sa.Integer(), nullable=False),
         sa.Column("plan_id", sa.Integer(), nullable=False),
         sa.Column("user_id", sa.String(), nullable=False),
         sa.Column("score", sa.Integer(), nullable=False),
         sa.Column("correct", sa.Integer(), nullable=False),
         sa.Column("total", sa.Integer(), nullable=False),
         sa.Column("answers", sa.Text(), nullable=True),
         sa.Column("created_at", sa.DateTime(), server_default=sa.func.now(), nullable=True),
         sa.ForeignKeyConstraint(["plan_id"], ["onboarding_plans.id"], ondelete="CASCADE"),
         sa.PrimaryKeyConstraint("id"),
     )
+    op.create_index("ix_onboarding_quiz_attempts_plan_id", "onboarding_quiz_attempts", ["plan_id"])
+    op.create_index("ix_onboarding_quiz_attempts_user_id", "onboarding_quiz_attempts", ["user_id"])

Remember to drop these indexes in downgrade() before drop_table.

🤖 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 `@alembic/versions/a5b6c7d8e9f0_add_onboarding_quiz_attempt.py` around lines 24
- 35, Add indexes for the onboarding_quiz_attempts foreign key columns,
especially plan_id and optionally user_id, in the migration that defines
onboarding_quiz_attempts. Update the upgrade logic in
a5b6c7d8e9f0_add_onboarding_quiz_attempt.py to create these indexes alongside
the table, and make sure downgrade drops the indexes before drop_table. Use the
onboarding_quiz_attempts table definition and the new index declarations as the
main place to apply the fix.

alembic/versions/d1e2f3a4b5c6_add_document_pages_and_chunks.py (1)

32-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

confluence_page_id naming will become source-agnostic.

Given DocumentChunk.source later distinguishes confluence/jira (per onboarding/models/document_chunk.py), this column name will be misleading once Jira issues are also stored in document_pages. Not a functional bug now, but worth a rename (or comment) in the source-generalization migration if not already addressed there.

Also applies to: 49-51

🤖 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 `@alembic/versions/d1e2f3a4b5c6_add_document_pages_and_chunks.py` at line 32,
The column name in the document page migration is source-specific and will be
misleading once Jira pages are stored alongside Confluence pages. Update the
migration around the document page table definition to use a source-agnostic
name or add an explicit comment/documentation aligned with the later
generalization in DocumentChunk.source and the document_pages schema.

alembic/versions/onb_user_company.py (2)

57-81: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoff

Row-by-row backfill loop may be slow/lock-heavy on large users tables.

Each user triggers up to 3 round-trips (lookup, optional insert, update) inside a single migration transaction. For tenants with large user tables this could hold locks for a long time during deploy. Given the PR's stated focus on scaling for "large tenants," consider batching (e.g., bulk INSERT ... ON CONFLICT keyed by domain, then a single bulk UPDATE ... FROM) if user counts are expected to be large.

🤖 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 `@alembic/versions/onb_user_company.py` around lines 57 - 81, The backfill loop
in the migration is doing per-user lookup/insert/update work, which can be slow
and lock-heavy for large `users` tables. Refactor the `op.get_bind()` backfill
logic to batch the work in `onb_user_company.py` by first collecting distinct
email domains, upserting companies in bulk (or with conflict handling), and then
applying a set-based `UPDATE ... FROM` to link `users.company_id` in one pass.
Keep `_key_and_name` for domain/name derivation, but avoid the current
row-by-row `users` iteration and repeated `bind.execute` round-trips.

---

45-50: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

Corporate company name defaults to the raw domain string.

For corporate emails, name is set equal to domain (e.g. "acme.com") rather than a human-friendly name. Acceptable for a backfill placeholder if it's editable later, but worth confirming the UI/company settings allow renaming, since this becomes the company's display name.

🤖 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 `@alembic/versions/onb_user_company.py` around lines 45 - 50, The
corporate-email branch in _key_and_name currently sets the company display name
to the raw domain, so update the backfill/default behavior to ensure that value
is either a friendlier placeholder or is explicitly editable in company settings
after creation. Verify the company creation and rename flow around _key_and_name
so the stored name does not become a permanent unreadable display name for
corporate accounts.

alembic/versions/onb_job_kind.py (1)

19-23: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Consider a CHECK constraint for kind.

The comment states kind is constrained to "ingest" | "reconcile", but the column accepts any string. Adding a CHECK (kind IN ('ingest','reconcile')) constraint would enforce this at the DB layer, consistent with how similar enum-like columns should be defended in depth (schema, service, and DB constraint).

♻️ Optional CHECK constraint

 def upgrade():
     op.add_column(
         "ingestion_jobs",
         sa.Column("kind", sa.String(length=16), nullable=False, server_default="ingest"),
     )
+    op.create_check_constraint(
+        "ck_ingestion_jobs_kind", "ingestion_jobs", "kind IN ('ingest', 'reconcile')"
+    )

Based on learnings, enum-like string columns (like skills.quiz_difficulty) should be validated consistently at the API/schema, service, and DB constraint layers with a matching CHECK constraint.

🤖 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 `@alembic/versions/onb_job_kind.py` around lines 19 - 23, The
ingestion_jobs.kind column is currently just a free-form string, so update the
migration in onb_job_kind.py to add a DB-level CHECK constraint that limits
values to the two supported kinds. Use the existing upgrade() migration that
adds kind, and add a matching named constraint so the schema enforces the same
enum-like values described by the service/API. If there is a downgrade path,
remove the constraint there as well, keeping the migration consistent with the
new kind column.

Source: Learnings

alembic/versions/onb_knowledge_chats.py (1)

32-46: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

role column also lacks a CHECK constraint.

Per the ORM docstring, role is 'user' | 'assistant' only, but the column accepts any string up to 16 chars. Consider adding a CHECK constraint for the same reason as kind/source in the sibling migrations.

♻️ Optional CHECK constraint

     op.create_index("ix_knowledge_messages_chat_id", "knowledge_messages", ["chat_id"])
+    op.create_check_constraint(
+        "ck_knowledge_messages_role", "knowledge_messages", "role IN ('user', 'assistant')"
+    )

Based on learnings, enum-like string columns should be enforced with a matching DB CHECK constraint in addition to the application-level contract.

🤖 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 `@alembic/versions/onb_knowledge_chats.py` around lines 32 - 46, The
knowledge_messages table creation in the onb_knowledge_chats migration leaves
role as a free-form string, which should be constrained like the sibling
enum-like columns. Update the op.create_table block for knowledge_messages to
add a database CHECK constraint on role in the same style used for kind/source
elsewhere, limiting values to the ORM-supported user and assistant roles. Use
the create_table definition and the role column as the anchors when applying the
constraint.

Source: Learnings

alembic/versions/onb_job_source.py (1)

19-23: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Same enum-hardening gap as kind: consider a CHECK constraint for source.

♻️ Optional CHECK constraint

 def upgrade():
     op.add_column(
         "ingestion_jobs",
         sa.Column("source", sa.String(length=32), nullable=False, server_default="confluence"),
     )
+    op.create_check_constraint(
+        "ck_ingestion_jobs_source", "ingestion_jobs", "source IN ('confluence', 'jira')"
+    )

Based on learnings, enum-like string columns should be enforced with a matching DB CHECK constraint alongside the application-level default set.

🤖 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 `@alembic/versions/onb_job_source.py` around lines 19 - 23, The new `source`
column in `upgrade()` is being added as an enum-like string without database
enforcement, so mirror the hardening used for `kind` by adding a matching CHECK
constraint alongside the `op.add_column` call. Update the `ingestion_jobs`
migration to constrain `source` to the allowed value set (starting with the
default `confluence`) so the schema enforces valid values even if the
application default is bypassed.

Source: Learnings

onboarding/services/retrieval.py (2)

345-360: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Docstring contradicts the actual implementation of "involved".

count_involvement's docstring states involved counts include "any of those, or named in the text", but _involved_clause (used for the involved count) explicitly does not match free-text mentions — only role columns (assignee/reporter/author/editor). This mismatch could mislead anyone relying on the docstring to interpret people-analytics results (or an LLM prompt referencing this behavior).

📝 Suggested docstring fix

 def count_involvement(company_id: int, name: str) -> dict:
     """Exact counts for a person by role across the whole knowledge base: assigned +
-    reported (Jira), authored (Confluence docs written/edited), and involved (any of
-    those, or named in the text). Uses word-boundary matching to avoid over-counting."""
+    reported (Jira), authored (Confluence docs written/edited), and involved (any of
+    those role-holdings — NOT mere text mentions). Uses word-boundary matching to
+    avoid over-counting."""

🤖 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 `@onboarding/services/retrieval.py` around lines 345 - 360, The docstring for
count_involvement is inconsistent with the actual involved logic implemented in
_involved_clause. Update the count_involvement docstring to describe involved as
only role-based matches (assignee, reporter, author, last editor) and remove any
mention of free-text name mentions. Keep the wording aligned with
_involved_clause and the involved count calculation so the documented behavior
matches the code.

---

413-418: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Invalid metric silently falls back instead of erroring.

leaderboard() coerces any unrecognized metric value to "involved" without signaling the caller. If this is reachable from an API endpoint with a user-supplied metric, a typo silently returns different data instead of a 400, which is easy to miss during integration/testing.

🤖 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 `@onboarding/services/retrieval.py` around lines 413 - 418, The leaderboard()
function currently hides invalid user input by silently replacing any
unsupported metric with "involved". Update leaderboard() to validate the metric
argument against _LEADERBOARD_METRICS and raise an explicit error instead of
coercing it, so callers using a user-supplied metric can detect bad values and
return a proper failure response. Keep the change localized to leaderboard() and
any nearby API-facing handling that consumes its metric parameter.

onboarding/models/document_chunk.py (1)

17-24: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Consider explicit ondelete on company_id/page_id foreign keys.

Unlike OnboardingQuizAttempt.plan_id (which sets ondelete="CASCADE"), these FKs have no cascade behavior. Deleting a DocumentPage/Company without first removing dependent chunks will hit a DB constraint violation instead of cascading cleanly. Ingestion already deletes chunks explicitly before re-embedding, but any other page/company deletion path is unprotected.

♻️ Suggested consistency fix

-    company_id: int = Field(foreign_key="companies.id", index=True)
-    page_id: int = Field(foreign_key="document_pages.id", index=True)
+    company_id: int = Field(foreign_key="companies.id", index=True, ondelete="CASCADE")
+    page_id: int = Field(foreign_key="document_pages.id", index=True, ondelete="CASCADE")

Would need a matching Alembic migration for the FK constraint.

🤖 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 `@onboarding/models/document_chunk.py` around lines 17 - 24, Add explicit
cascade behavior to the DocumentChunk foreign keys so dependent chunks are
removed automatically when a Company or DocumentPage is deleted. Update the
DocumentChunk model’s company_id and page_id relationships to use
ondelete="CASCADE" in a way consistent with OnboardingQuizAttempt.plan_id, and
make sure the ORM/schema reflects this change. Also add the matching Alembic
migration to drop and recreate the FK constraints with cascade semantics so
deletion paths remain consistent across the database.

onboarding/services/embeddings.py (1)

19-27: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Lazy singleton model load isn't thread-safe.

_get_model()'s check-then-set on the global _model has a race: concurrent first calls (e.g., parallel ingestion workers) can each see _model is None and construct/download the model redundantly. Community fastembed usage patterns guard this init with a lock.

🔒 Suggested fix

+import threading
+
 _model: Optional[TextEmbedding] = None
+_model_lock = threading.Lock()
 
 def _get_model() -> TextEmbedding:
     """Lazily load and cache the embedding model (first call downloads it)."""
     global _model
     if _model is None:
-        logger.info("Loading embedding model %s", config.EMBEDDING_MODEL)
-        _model = TextEmbedding(model_name=config.EMBEDDING_MODEL)
+        with _model_lock:
+            if _model is None:
+                logger.info("Loading embedding model %s", config.EMBEDDING_MODEL)
+                _model = TextEmbedding(model_name=config.EMBEDDING_MODEL)
     return _model

🤖 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 `@onboarding/services/embeddings.py` around lines 19 - 27, _get_model currently
uses a racy check-then-set on the global _model, so concurrent first callers can
initialize TextEmbedding multiple times. Update
onboarding/services/embeddings.py to guard the lazy singleton initialization
with a thread lock around the _model is None check and assignment, keeping the
existing _get_model and _model symbols as the entry point and cached instance.

tests/test_retrieval.py (1)

465-476: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Mocked-session test doesn't verify the sources filter is actually applied to the query.

test_sources_filter_applied mocks session.exec to return fixed rows regardless of what _base_select builds, so it can't catch a regression where the sources .where(...) clause (or the SET LOCAL enable_indexscan/bitmapscan calls in the if sources: branch) is dropped or broken. Based on learnings, mocked-session tests can't exercise DB-side WHERE-clause filtering — this needs either an integration test against a real DB (SQLite/Postgres) or an assertion on the generated statement/conn.exec_driver_sql calls to actually cover this path.

🤖 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 `@tests/test_retrieval.py` around lines 465 - 476, The current
`test_sources_filter_applied` only stubs `session.exec` with fixed rows, so it
does not prove that `retrieve()` and its `_base_select` query actually apply the
`sources` filter or the `SET LOCAL enable_indexscan/bitmapscan` calls in the `if
sources:` branch. Update this test to either assert against the generated
statement/SQL execution path (for example by inspecting the statement passed
through `retrieve()`/`session.exec`) or replace it with an integration-style
test against a real database so the `sources` `.where(...)` clause is genuinely
exercised.

Source: Learnings

onboarding/services/confluence.py (1)

841-861: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Add strict=True to zip(chunks, vectors) to fail loudly on a length mismatch.

If embedding_service.embed_texts ever returns fewer/more vectors than input chunks (partial failure, upstream bug), zip() silently truncates and pairs the wrong vector with the wrong chunk content — corrupting embeddings without any error. _flush_embed_batch already relies on vectors[offset:offset+n] slicing lining up exactly with chunks; strict=True turns a silent corruption into a loud, debuggable failure.

🛡️ Proposed fix

     with Session(engine) as session:
-        for i, (content, vector) in enumerate(zip(chunks, vectors)):
+        for i, (content, vector) in enumerate(zip(chunks, vectors, strict=True)):
             session.add(

Also applies to: 983-996

🤖 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 `@onboarding/services/confluence.py` around lines 841 - 861, The _store_chunks
helper silently truncates when pairing chunks with vectors, which can corrupt
embeddings if their lengths differ. Update the zip(chunks, vectors) iteration in
_store_chunks to use strict mode so a mismatch fails immediately, and ensure the
same alignment assumption still holds for the _flush_embed_batch flow that
passes sliced chunk/vector batches into _store_chunks.

Source: Linters/SAST tools

tests/test_knowledge.py (1)

797-807: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Mocked session doesn't verify tenant/user WHERE-clause scoping.

list_chats filters by company_id AND user_id at the SQL layer, but this test fully mocks session.exec(...).all(), so the WHERE predicates never execute — a regression that dropped or swapped the company_id/user_id filter would still pass this test. Given this scopes cross-tenant data access, consider an integration test against a real/SQLite-backed session (or assert on the compiled statement's filter clauses) to actually exercise the isolation logic.

Based on learnings, for query functions where filtering happens purely in the SQL WHERE clause, mocked-session tests can't simulate database-side filtering — recommend an integration-style test with a real DB/SQLite or inspecting the generated statement's predicates.

🤖 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 `@tests/test_knowledge.py` around lines 797 - 807, The test for list_chats is
only mocking session.exec().all(), so it never verifies the company_id/user_id
SQL filtering and can miss cross-tenant scoping regressions. Update
test_list_chats to use a real SQLite-backed session or assert the compiled
statement built by onboarding.services.knowledge.list_chats includes the correct
WHERE predicates for both company_id and user_id, so the isolation logic is
actually exercised.

Source: Learnings

onboarding/services/knowledge.py (1)

354-420: 🧹 Nitpick | 🔵 Trivial

Consider access controls for people-analytics.

The leaderboard/count/list "people" answers surface individual work-item counts and rankings to any authenticated company user. If some roles shouldn't see comparative performance data across teammates, consider gating this path (e.g., manager-only) rather than exposing it to all knowledge-chat users.

🤖 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 `@onboarding/services/knowledge.py` around lines 354 - 420, The
people-analytics path in _maybe_person_answer currently returns leaderboard,
count, and list results to any authenticated user, so add an access-control
check before calling _leaderboard_answer, _count_answer, or _list_answer. Use
the existing user/role context available to this codepath to gate comparative
teammate performance data (for example, manager-only) and fall through to normal
RAG when the caller is not authorized.

services/content_validator.py (1)

202-214: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract duplicated keyword-extraction loop into a helper.

The extraction logic (strip/normalize/filter stopwords) is copy-pasted for keyword_source and the task_description fallback. Consolidating avoids future divergence between the two blocks.

♻️ Proposed refactor

+def _extract_keywords(source: str) -> set:
+    keywords: set = set()
+    for raw_w in source.split():
+        w = re.sub(r"[^\w.-]", "", raw_w).strip(".-").lower()
+        if len(w) > 3 and w not in _STOPWORDS:
+            keywords.add(w)
+    return keywords
+
     keyword_source = topic if topic else task_description
-    task_keywords: set = set()
-    for raw_w in keyword_source.split():
-        w = re.sub(r"[^\w.-]", "", raw_w).strip(".-").lower()
-        if len(w) > 3 and w not in _STOPWORDS:
-            task_keywords.add(w)
+    task_keywords: set = _extract_keywords(keyword_source)
     # If topic yielded no usable keywords, fall back to task_description so
     # check 3 is never silently disabled.
     if not task_keywords and topic:
-        for raw_w in task_description.split():
-            w = re.sub(r"[^\w.-]", "", raw_w).strip(".-").lower()
-            if len(w) > 3 and w not in _STOPWORDS:
-                task_keywords.add(w)
+        task_keywords = _extract_keywords(task_description)

🤖 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 `@services/content_validator.py` around lines 202 - 214, The keyword extraction
logic is duplicated between the main `keyword_source` loop and the
`task_description` fallback in `content_validator.py`. Extract the
normalization/filtering steps into a small helper used by `task_keywords`
population so both paths share the same behavior. Keep the fallback in the
existing `check 3` flow, but have it call the helper instead of repeating the
`re.sub`, strip, lowercase, length, and stopword checks.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c098b4a3-30a5-467c-83c2-7c13c3219259

📥 Commits

Reviewing files that changed from the base of the PR and between 6b36845 and 8823b21.

⛔ Files ignored due to path filters (1)

• .DS_Store is excluded by !**/.DS_Store

📒 Files selected for processing (102)

• .coveragerc
• .dockerignore
• .env.example
• .gitignore
• Dockerfile
• KNOWLEDGE_PLATFORM_DESIGN.md
• LARGE_INGESTION_CHALLENGE.md
• README.md
• alembic/env.py
• alembic/versions/a5b6c7d8e9f0_add_onboarding_quiz_attempt.py
• alembic/versions/adaptive_topic_canon_fix_topic_canonicalization.py
• alembic/versions/b6c7d8e9f0a1_add_company_and_confluence_connection.py
• alembic/versions/c7d8e9f0a1b2_add_onboarding_docs_and_ingestion_jobs.py
• alembic/versions/d1e2f3a4b5c6_add_document_pages_and_chunks.py
• alembic/versions/e2f3a4b5c6d7_drop_onboarding_docs.py
• alembic/versions/f3a4b5c6d7e8_embedding_dim_768_to_384.py
• alembic/versions/onb_doc_source.py
• alembic/versions/onb_drop_space_keys.py
• alembic/versions/onb_ingest_phase_spaces.py
• alembic/versions/onb_jira_fields.py
• alembic/versions/onb_job_kind.py
• alembic/versions/onb_job_source.py
• alembic/versions/onb_knowledge_chats.py
• alembic/versions/onb_knowledge_msg_blocks.py
• alembic/versions/onb_page_meta.py
• alembic/versions/onb_user_company.py
• alembic/versions/y3z4a5b6c7d8_add_onboarding_tables.py
• alembic/versions/z4a5b6c7d8e9_add_share_enabled_to_onboarding_plans.py
• config.py
• controllers/auth.py
• main.py
• middleware/auth.py
• models/__init__.py
• models/user.py
• onboarding/__init__.py
• onboarding/controllers/__init__.py
• onboarding/controllers/confluence.py
• onboarding/controllers/knowledge.py
• onboarding/controllers/onboarding.py
• onboarding/models/__init__.py
• onboarding/models/company.py
• onboarding/models/confluence_connection.py
• onboarding/models/document_chunk.py
• onboarding/models/document_page.py
• onboarding/models/ingestion_job.py
• onboarding/models/knowledge_chat.py
• onboarding/models/knowledge_message.py
• onboarding/models/onboarding_day.py
• onboarding/models/onboarding_plan.py
• onboarding/models/onboarding_quiz_attempt.py
• onboarding/prompts/__init__.py
• onboarding/prompts/knowledge.py
• onboarding/prompts/onboarding.py
• onboarding/routes/__init__.py
• onboarding/routes/confluence.py
• onboarding/routes/knowledge.py
• onboarding/routes/onboarding.py
• onboarding/schemas/__init__.py
• onboarding/schemas/knowledge.py
• onboarding/schemas/onboarding.py
• onboarding/services/__init__.py
• onboarding/services/chunking.py
• onboarding/services/confluence.py
• onboarding/services/embeddings.py
• onboarding/services/generation.py
• onboarding/services/knowledge.py
• onboarding/services/onboarding.py
• onboarding/services/retrieval.py
• requirements.txt
• routes/__init__.py
• scripts/setup.sh
• scripts/test_webhook.sh
• scripts/warm_embeddings.py
• services/company.py
• services/content_validator.py
• services/encryption.py
• services/llm.py
• services/user.py
• tests/test_chunking_embeddings.py
• tests/test_company.py
• tests/test_confluence.py
• tests/test_content_blocks.py
• tests/test_content_validator.py
• tests/test_generation_retry.py
• tests/test_knowledge.py
• tests/test_onboarding.py
• tests/test_retrieval.py
• tests/test_user_service.py
• ui/src/app/plugins/connections/banner.tsx
• ui/src/app/plugins/connections/index.tsx
• ui/src/app/plugins/knowledge/index.tsx
• ui/src/app/plugins/onboarding/detail.tsx
• ui/src/app/plugins/onboarding/index.tsx
• ui/src/app/plugins/onboarding/public.tsx
• ui/src/app/plugins/sampleApp/index.tsx
• ui/src/app/plugins/syllabi/block-renderer.tsx
• ui/src/app/plugins/syllabi/detail.tsx
• ui/src/app/plugins/syllabi/quiz.tsx
• ui/src/components/app-sidebar.tsx
• ui/src/components/nav-user.tsx
• ui/src/routes/index.tsx
• ui/src/store/authStore.ts

💤 Files with no reviewable changes (1)

• ui/src/app/plugins/syllabi/quiz.tsx

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

alembic/versions/ingest_single_flight_one_running_job.py (1)

33-39: 🚀 Performance & Scalability | 🔵 Trivial

Consider CREATE INDEX CONCURRENTLY for large ingestion_jobs tables.

op.create_index here takes a full table lock while building. If ingestion_jobs grows large in multi-tenant deployments, this could briefly block reads/writes during deploy. Alembic runs migrations in a transaction by default, so CONCURRENTLY would require wrapping this in op.get_context().autocommit_block().

Given the row count is likely low (one row per ingestion run), this is a low-priority note rather than a blocker.

🤖 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 `@alembic/versions/ingest_single_flight_one_running_job.py` around lines 33 -
39, The `op.create_index` call in the migration that creates the
`ingestion_jobs` partial unique index can take a blocking table lock during
deployment. Update the migration around `op.create_index` to use a concurrent
index build for large deployments by wrapping it in
`op.get_context().autocommit_block()` and performing the index creation there,
keeping the existing index name, columns, and `postgresql_where` condition
intact.

🤖 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 `@onboarding/services/onboarding.py`:
- Around line 277-282: The quiz payload in get_quiz is still exposing the answer
rationale by keeping explanation in each question object. Update the question
sanitization in get_quiz so it removes explanation along with correct_answer
before returning questions, and mirror the same filtering in save_quiz_attempt
or any shared quiz serialization path used for the same payload. Use the
existing get_quiz and save_quiz_attempt logic as the place to apply the
exclusion, and only include explanations in a post-submission response path if
needed.
- Around line 340-343: The quiz scoring logic in
onboarding/services/onboarding.py should fail closed when a question is missing
correct_answer instead of defaulting to "a". Update the question evaluation in
the quiz loop (the block using enumerate(questions), correct_key, selected, and
is_correct) to treat a missing or empty correct_answer as invalid and mark that
question incorrect rather than comparing against a fallback. Use the existing
quiz-processing code path to locate the fix and preserve current behavior only
for questions with an explicit stored answer key.

---

Nitpick comments:
In `@alembic/versions/ingest_single_flight_one_running_job.py`:
- Around line 33-39: The `op.create_index` call in the migration that creates
the `ingestion_jobs` partial unique index can take a blocking table lock during
deployment. Update the migration around `op.create_index` to use a concurrent
index build for large deployments by wrapping it in
`op.get_context().autocommit_block()` and performing the index creation there,
keeping the existing index name, columns, and `postgresql_where` condition
intact.

🪄 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: defaults

Review profile: CHILL

Plan: Pro

Run ID: 06ff8190-5075-4072-b825-f7b64632ccfb

📥 Commits

Reviewing files that changed from the base of the PR and between 8823b21 and 402ecca.

📒 Files selected for processing (23)

• .dockerignore
• Dockerfile
• alembic/versions/company_name_full_email_generic.py
• alembic/versions/ingest_single_flight_one_running_job.py
• onboarding/controllers/onboarding.py
• onboarding/models/onboarding_plan.py
• onboarding/prompts/knowledge.py
• onboarding/prompts/onboarding.py
• onboarding/schemas/onboarding.py
• onboarding/services/confluence.py
• onboarding/services/generation.py
• onboarding/services/knowledge.py
• onboarding/services/onboarding.py
• onboarding/services/retrieval.py
• services/company.py
• services/llm.py
• tests/test_company.py
• tests/test_confluence.py
• tests/test_knowledge.py
• tests/test_onboarding.py
• ui/src/app/plugins/knowledge/index.tsx
• ui/src/app/plugins/onboarding/detail.tsx
• ui/src/app/plugins/onboarding/index.tsx

🚧 Files skipped from review as they are similar to previous changes (16)

• .dockerignore
• onboarding/models/onboarding_plan.py
• tests/test_company.py
• services/company.py
• services/llm.py
• onboarding/prompts/knowledge.py
• onboarding/controllers/onboarding.py
• tests/test_onboarding.py
• onboarding/services/knowledge.py
• ui/src/app/plugins/onboarding/index.tsx
• ui/src/app/plugins/knowledge/index.tsx
• ui/src/app/plugins/onboarding/detail.tsx
• tests/test_knowledge.py
• onboarding/services/generation.py
• onboarding/services/retrieval.py
• onboarding/services/confluence.py

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (1)

docker-compose.yml (1)

18-19: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Bind the published port to localhost.

"5432:5432" publishes on 0.0.0.0, exposing a DB with trivial openship/openship credentials to the whole local network. For a local-dev DB, bind to loopback. This also makes the collision with a host-local Postgres explicit rather than silently competing for the interface.

🔒 Proposed change

     ports:
-      - "5432:5432"
+      - "127.0.0.1:5432:5432"

🤖 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 `@docker-compose.yml` around lines 18 - 19, The postgres port mapping is
currently published on all interfaces, which exposes the local-dev database
beyond loopback and can silently compete with a host Postgres. Update the ports
entry in docker-compose.yml to bind the existing 5432 mapping to localhost only,
keeping the same service port while using a loopback host binding so it is
explicit and not reachable from the wider network.

🤖 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 `@scripts/dev.cmd`:
- Around line 1-3: The Windows launcher wrapper in the dev.cmd script needs to
be saved with CRLF line endings instead of LF-only. Update the script file
encoding/line endings so cmd.exe can parse the wrapper reliably before handing
off to dev.ps1, and verify the wrapper still invokes the PowerShell entrypoint
correctly.

In `@scripts/setup.ps1`:
- Around line 29-89: The step labels in setup.ps1 are inconsistent with the
mirrored setup.sh flow and the total count, since the script starts at “Step
0/6” and skips “Step 2”. Update the Info calls in the setup sequence (the
Python, Database, and subsequent secret/.env sections) so the numbering matches
setup.sh’s 1/6 through 6/6 progression and uses a consistent label order
throughout.
- Around line 44-52: The Python version check in setup.ps1 is happening
immediately after winget installs Python, but the current PowerShell session may
not have the updated PATH yet. Update the flow around the winget install/Ok path
so the script either reloads PATH or tells the user to reopen the shell before
running the final `py` version check, matching the existing Node setup guidance.
Keep the fix localized to the `winget` branch and the `Ok ("Python: " + (&
$PyExe `@PyArgs` --version))` call site.

---

Nitpick comments:
In `@docker-compose.yml`:
- Around line 18-19: The postgres port mapping is currently published on all
interfaces, which exposes the local-dev database beyond loopback and can
silently compete with a host Postgres. Update the ports entry in
docker-compose.yml to bind the existing 5432 mapping to localhost only, keeping
the same service port while using a loopback host binding so it is explicit and
not reachable from the wider network.

🪄 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: defaults

Review profile: CHILL

Plan: Pro

Run ID: 8ef223ed-9080-495b-9be4-2b9ed85de146

📥 Commits

Reviewing files that changed from the base of the PR and between abb3a19 and 425f019.

📒 Files selected for processing (9)

• Makefile
• alembic/versions/d1e2f3a4b5c6_add_document_pages_and_chunks.py
• docker-compose.yml
• docs/LOCAL_SETUP.md
• scripts/dev.cmd
• scripts/dev.ps1
• scripts/setup.cmd
• scripts/setup.ps1
• scripts/setup.sh

✅ Files skipped from review due to trivial changes (1)

• docs/LOCAL_SETUP.md

🚧 Files skipped from review as they are similar to previous changes (1)

• alembic/versions/d1e2f3a4b5c6_add_document_pages_and_chunks.py

[coderabbitai]

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Save this wrapper with CRLF line endings.

LF-only .cmd files can misparse under cmd.exe and break the launcher before PowerShell starts.

🧰 Tools

🪛 Blinter (1.0.113)

[error] 1-1: Unix line endings detected. Explanation: Batch file uses Unix line endings (LF-only) which can cause GOTO/CALL label parsing failures and script malfunction due to Windows batch parser 512-byte boundary bugs. Recommendation: Convert file to Windows line endings (CRLF). Use tools like dos2unix, notepad++, or configure git with 'git config core.autocrlf true'. Context: File uses Unix line endings (LF-only) - 3 LF sequences found

(E018)

---

[error] 3-3: PowerShell execution policy bypass. Explanation: Bypassing PowerShell execution policy can allow malicious scripts to run. Recommendation: Avoid using -ExecutionPolicy Bypass unless absolutely necessary. Context: PowerShell execution policy bypass detected

(SEC009)

🤖 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 `@scripts/dev.cmd` around lines 1 - 3, The Windows launcher wrapper in the
dev.cmd script needs to be saved with CRLF line endings instead of LF-only.
Update the script file encoding/line endings so cmd.exe can parse the wrapper
reliably before handing off to dev.ps1, and verify the wrapper still invokes the
PowerShell entrypoint correctly.

Source: Linters/SAST tools