feat(chatbot): add in-page-nav attribute for cauldron testing

[yodem]

Summary

• Adds in-page-nav boolean attribute to <lc-chatbot> that forces sefaria:bootstrap-url dispatch for all Sefaria links, even when the host page is not on sefaria.org
• Applies to response body links, appetizer topic chips, and context chip navigation
• Enables testing topic and source in-page navigation on cauldron/staging without a production deployment

Usage

Add in-page-nav="true" to the chatbot embed in the Sefaria-Project cauldron branch:

<lc-chatbot in-page-nav="true" api-base-url="..." user-id="..."></lc-chatbot>

⚠️ Do not merge to main

This branch is for cauldron testing only. The in-page-nav attribute bypasses the production hostname guard. Delete after testing.

🤖 Generated with Claude Code

[coolify-sefaria-github]

The preview deployment for sefaria/ai-chatbot:server is ready. 🟢

Open app | Open Build Logs | Open Application Logs

Last updated at: 2026-06-03 16:21:07 CET

[gitvelocity-reviewer]

I'll analyze this PR systematically, reviewing the code changes, architecture, test coverage, and quality.

PR Overview

This PR implements a "Topic Appetizer" feature — a parallel pipeline that shows relevant Sefaria topic links within 5-8 seconds while the main AI agent processes the user's query. It also adds a "Progress Trail" component that shows streaming tool call progress.

Key Changes

1. Backend: New appetizer_service.py with multi-topic LLM extraction + Sefaria API search
2. Backend: sefaria_client.py — new search_topics() method with slug fallback
3. Backend: views.py — parallel appetizer thread, SSE event emission
4. Frontend: New TopicAppetizer.svelte, ProgressTrail.svelte, SourceSuggestion.svelte
5. Frontend: LCChatbot.svelte — wires new components, handles appetizer SSE events
6. AI Artifact: src/CLAUDE.md — documents new in-page-nav attribute

---

Issues Found

🔴 Critical Issues

1. Hardcoded Secret in Load Test Script
server/loadtest/curl_loadtest.sh:32

CHATBOT_USER_TOKEN_SECRET=Mg57y8NKXyaZCQHFbFvXECUEoWhjGoUXJbbVt3H
USER_ID="cLmwgp3ikvK5B0883-1U6JIhm1ZkQFTuiX19h0PGzRdqfuo9rYCuSd9Fob6oG7Yx5tA4fuhH6SZ18PaE1S13-u8DD0oK7sZfn96Zo8INN0PI8Bfhxco4bA2d66dEJ8ePTCp2NMkbmrRd9Dny9sijiY-eKOAJcDZlNb4JPaSHlO2t9ckKbdn_IhQqPboINA=="

A secret key and what appears to be an encrypted user token are hardcoded in the script. Even if this is a test/staging secret, it should not be committed to source control. The comment says "Requires: CHATBOT_USER_TOKEN_SECRET" as an env var, but then hardcodes it anyway.

2. XSS Risk in linkifyRefs
src/components/ProgressTrail.svelte:44-52

function linkifyRefs(text) {
    const escaped = text.replace(/&/g, '&amp;').replace(/</g, '&lt;').replace(/>/g, '&gt;');
    // ...
    return escaped.replace(/(['"])([^'"]+)\1/g, (match, quote, ref) => {
      const url = refToUrl(ref);
      if (!url) return match;
      return `${quote}${icon}<a class="trail-ref-link" href="${url}" ...>${ref}</a>${quote}`;
    });
}

The ref variable is inserted directly into the HTML string without escaping. While escaped is the outer text, the captured ref group from the regex is not HTML-escaped before being placed in the href and link text. A malicious tool description containing '"><script>alert(1)</script>' could bypass the outer escaping. The refToUrl function would return null for this, but the ref in the link text is still unescaped.

Actually, looking more carefully: the outer escaped string is what's being regex-replaced, so ref comes from the already-escaped string. However, the url from refToUrl is constructed from ref which could contain encoded HTML entities — this could produce malformed URLs. More importantly, the ref in the link text is inserted as-is from the escaped string, which is fine. But the url construction in refToUrl doesn't encode the ref properly for use in an href.

3. asyncio.run() in Thread — Potential Event Loop Conflicts
server/chat/V2/views.py:310

result = asyncio.run(appetizer_service.find_appetizer(data["text"]))

Using asyncio.run() inside a thread executor is generally fine (it creates a new event loop), but if Django is running with an async server (ASGI), this could conflict. The existing code uses asyncio.to_thread inside the service, which is correct for the inner async calls. This pattern is consistent with the rest of the codebase, so it's likely acceptable.

🟡 Medium Issues

4. stream_closed Race Condition
server/chat/V2/views.py

stream_closed = False
# ...
def run_appetizer():
    if result and not stream_closed:
        progress_queue.put(update, timeout=0.5)

stream_closed is a plain Python bool shared between threads without synchronization. While CPython's GIL makes simple reads/writes atomic, this is still technically a data race. Using threading.Event would be more correct.

5. Appetizer Executor Not Awaited on Error Paths
server/chat/V2/views.py:748

executor.shutdown(wait=False)
appetizer_executor.shutdown(wait=False)

The appetizer_executor is shut down in the finally block, but if an exception occurs before generate_sse() is called (e.g., during the is_load_test setup), the executor may leak. The executor is created at line ~343 before generate_sse() is defined, so it starts running immediately. If generate_sse() never runs, the executor is never shut down.

6. Inconsistent Timeout Exception Handling
server/chat/V2/appetizer/appetizer_service.py:47

except TimeoutError:

Should be asyncio.TimeoutError (or both, since Python 3.11+ made them the same). The plan documents use asyncio.TimeoutError but the implementation uses bare TimeoutError. In Python 3.11+, these are the same, but for older versions this could miss the timeout.

7. Playwright MCP Snapshot Files Committed
.playwright-mcp/page-2026-05-27T06-45-15-253Z.yml etc.
These appear to be debug/test artifacts from Playwright MCP sessions. They shouldn't be committed to the repository — they're ephemeral test snapshots with no long-term value.

8. Duplicate PNG Files
appetizer-final-state.png and appetizer-hebrew-test.png are identical (same hash 71d7812f). These appear to be test screenshots that shouldn't be in the repo root.

9. tool_input Added to tool_end Progress Update
server/chat/V2/agent/tool_runtime.py:81

tool_input=tool_input,

tool_input is now included in tool_end events. This could expose sensitive tool inputs (e.g., search queries, user data) in SSE events that are visible to the client. Verify this is intentional and that no sensitive data leaks.

10. normalizeAppetizerData — Backward Compatibility Shim
src/components/LCChatbot.svelte:816-820

function normalizeAppetizerData(raw) {
    if (!raw) return null;
    if (raw.topics) return raw;
    return { topics: [{ topicSlug: raw.topicSlug, topicTitle: raw.topicTitle, topicUrl: raw.topicUrl }] };
}

This shim handles old single-topic format. If this is for backward compatibility with persisted messages, it's fine. But it suggests the data model changed mid-development without a clean migration. Consider documenting when this shim can be removed.

🟢 Minor Issues

11. appetizer-header Missing cursor: pointer
src/components/LCChatbot.svelte (CSS section)
The .appetizer-header CSS doesn't include cursor: pointer, making it unclear to users that it's clickable. The plan documents included this but it was dropped.

12. TopicAppetizer.svelte — Non-collapsible
The final implementation of TopicAppetizer doesn't have a collapse/expand toggle (unlike the plan). The feature-tests.json shows TA-3 (collapsibility) as passing, but the current component has no toggle button. This is inconsistent.

Looking at the component more carefully — the header is a <div> not a <button>, and there's no toggle logic. The test result notes say it works, but the code doesn't show it. Either the test was run against a different version, or the feature was removed.

13. Plan Documents in docs/plans/
Multiple detailed implementation plan documents are committed. These are valuable for context but contain local file paths (/Users/yotamfromm/dev/sefaria/ai-chatbot) which are developer-specific and shouldn't be in committed plans.

14. index.html — Encrypted Token Committed

user-id="VrEKrbLVo8iV0AgQDGNStkXu0fWsD08bM7iYQ2AwZKRkiTrPuoBxYTPWumaIdMLw_..."

An encrypted user token is committed to index.html. Even if this is a demo/dev token, it's a credential that should be in a .env file or generated at runtime.

---

Test Coverage

The test file test_appetizer_service.py is comprehensive:

• ✅ search_topics — 4 tests covering happy path, slug fallback, empty results, type filtering
• ✅ Multi-topic flow — 8 tests covering multiple topics, partial hits, deduplication, fallback candidates, Hebrew, timeout
• ✅ Edge cases — LLM returning empty, all candidates missing, timeout

Coverage estimate: ~85% of the new backend code is tested. The views.py appetizer thread logic is not unit tested (integration-level only via Playwright). Frontend components have no automated tests.

The test for test_search_topics_returns_first_match checks mock.assert_called_once_with("api/name/shabbat", {"limit": "3"}) — this is correct for the new implementation (no type filter), matching the actual code.

---

Architecture Assessment

The two-tier approach (direct API → LLM fallback) from the plan was replaced with a single LLM-first approach using Sonnet (not Haiku) with tool-forcing. This is a significant architectural change from the plan:

• Plan: Regex strip → direct API → Haiku fallback
• Implementation: Sonnet with tool-forcing → API search for each candidate

This is actually a better design for quality but uses a more expensive model (Sonnet vs Haiku). The 8-second timeout (vs 5-second in the plan) reflects this tradeoff.

The parallel thread architecture is sound. The asyncio.run() in a thread pattern is consistent with the codebase.

---

Summary

This is a well-executed feature with good test coverage on the backend. The main concerns are:

1. Security: Hardcoded secrets in the load test script and demo HTML
2. XSS: Minor risk in linkifyRefs URL construction
3. Artifacts: Playwright snapshots and PNG screenshots shouldn't be committed
4. Inconsistency: TopicAppetizer collapsibility in tests vs. code

---

Code Quality Score

Sub-scores:

S (Scope) = 16/20

• Touches 8+ files across frontend (Svelte), backend (Python/Django), config, and docs
• New SSE event type, new parallel thread architecture, new UI components
• Cross-cutting: frontend ↔ backend ↔ external API integration
• New public-facing feature with analytics event

A (Architecture) = 15/20

• New parallel thread pattern for appetizer alongside main agent
• New component hierarchy (TopicAppetizer, ProgressTrail, SourceSuggestion)
• New SSE event type extending existing protocol
• Slug fallback pattern in search_topics is a good defensive design
• Progress trail replaces single-bubble thinking indicator (architectural improvement)

I (Implementation) = 14/20

• Async/await with asyncio.wait_for timeout wrapping
• Multi-candidate LLM extraction with deduplication
• Tool-forcing pattern for structured LLM output
• linkifyRefs with regex-based ref detection
• findLastIndex for correct tool_end matching (improvement over previous naive last-index approach)
• normalizeAppetizerData backward compat shim

R (Risk) = 10/20

• New parallel thread adds concurrency complexity
• SSE protocol extension is backward compatible
• stream_closed race condition (minor)
• Hardcoded secrets in committed files (significant risk)
• No feature flag for the appetizer (can't disable without deploy)
• tool_input exposure in tool_end events

Q (Quality) = 11/15

• Good backend test coverage (~85% of new service code)
• Tests cover edge cases: deduplication, partial hits, timeout, Hebrew
• No frontend component tests
• Plan documents provide good context
• feature-tests.json serves as integration test record
• Playwright snapshots committed (noise)

P (Performance/Security) = 2/5

• 8-second timeout on appetizer pipeline
• Proxy buffer flush with 4KB SSE comment (good operational thinking)
• Braintrust metrics logging for appetizer performance
• But: hardcoded secrets, XSS risk in linkifyRefs, no rate limiting on appetizer thread

Base Score = 16 + 15 + 14 + 10 + 11 + 2 = 68

Effort Scale:

• Effective Lines: 1316 → Extra Large tier (ESF: 1.0x)
• File Count: 19 → Large tier
• Gap: Large - Extra Large = -1 (no bump needed)
• Final ESF: 1.0x

Final Score: 68 × 1.0 = 68

Code Quality Data (JSON)

{
  "_schema": "code_quality_v5",
  "total_score": 68,
  "total_factors": "68 × 1.0 (Extra Large ESF) = 68",
  "scope_score": 16,
  "scope_factors": "8+ files across frontend/backend/config; new SSE event type; parallel thread architecture; new UI components; cross-cutting frontend-backend-external API integration; analytics event dispatch",
  "architecture_score": 15,
  "architecture_factors": "New parallel appetizer thread pattern; new component hierarchy (TopicAppetizer, ProgressTrail, SourceSuggestion); SSE protocol extension; slug fallback in search_topics; progress trail replaces single-bubble thinking indicator; tool-forcing pattern for structured LLM output",
  "implementation_score": 14,
  "implementation_factors": "asyncio.wait_for timeout wrapping; multi-candidate LLM extraction with deduplication; tool-forcing for structured output; linkifyRefs regex ref detection; findLastIndex for correct tool_end matching; normalizeAppetizerData backward compat shim; in-page navigation logic with hostname checks",
  "risk_score": 10,
  "risk_factors": "New parallel thread adds concurrency complexity (+3); hardcoded secrets in committed files (+4); no feature flag for appetizer (+2); tool_input exposure in tool_end events (+2); stream_closed race condition (+1); SSE protocol extension is backward compatible (-2)",
  "quality_score": 11,
  "quality_factors": "Good backend test coverage ~85% of new service code (+4); edge case tests: deduplication, partial hits, timeout, Hebrew (+3); feature-tests.json as integration record (+2); plan documents provide context (+1); no frontend