-
Notifications
You must be signed in to change notification settings - Fork 4.5k
feat(search): scope Search and Ask to selected notebooks #1331
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
62fc88f
163283c
6809d91
997d47a
a6745e3
f05c153
91603ae
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,13 +1,17 @@ | ||
| import json | ||
| from typing import AsyncGenerator | ||
| from typing import AsyncGenerator, List | ||
|
|
||
| from fastapi import APIRouter, HTTPException | ||
| from fastapi.responses import StreamingResponse | ||
| from loguru import logger | ||
|
|
||
| from api.models import AskRequest, AskResponse, SearchRequest, SearchResponse | ||
| from open_notebook.ai.models import Model, model_manager | ||
| from open_notebook.domain.notebook import text_search, vector_search | ||
| from open_notebook.domain.notebook import ( | ||
| resolve_notebook_scope, | ||
| text_search, | ||
| vector_search, | ||
| ) | ||
| from open_notebook.exceptions import ( | ||
| DatabaseOperationError, | ||
| InvalidInputError, | ||
|
|
@@ -22,6 +26,8 @@ | |
| async def search_knowledge_base(search_request: SearchRequest): | ||
| """Search the knowledge base using text or vector search.""" | ||
| try: | ||
| notebook_ids = await resolve_notebook_scope(search_request.scope_notebook_ids) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: Custom agent: Security & testability When scope validation hits a database or driver error, Prompt for AI agents |
||
|
|
||
| if search_request.type == "vector": | ||
| # Check if embedding model is available for vector search | ||
| if not await model_manager.get_embedding_model(): | ||
|
|
@@ -36,6 +42,7 @@ async def search_knowledge_base(search_request: SearchRequest): | |
| source=search_request.search_sources, | ||
| note=search_request.search_notes, | ||
| minimum_score=search_request.minimum_score, | ||
| notebook_ids=notebook_ids, | ||
| ) | ||
| else: | ||
| # Text search | ||
|
|
@@ -44,6 +51,7 @@ async def search_knowledge_base(search_request: SearchRequest): | |
| results=search_request.limit, | ||
| source=search_request.search_sources, | ||
| note=search_request.search_notes, | ||
| notebook_ids=notebook_ids, | ||
| ) | ||
|
|
||
| return SearchResponse( | ||
|
|
@@ -67,7 +75,11 @@ async def search_knowledge_base(search_request: SearchRequest): | |
|
|
||
|
|
||
| async def stream_ask_response( | ||
| question: str, strategy_model: Model, answer_model: Model, final_answer_model: Model | ||
| question: str, | ||
| strategy_model: Model, | ||
| answer_model: Model, | ||
| final_answer_model: Model, | ||
| notebook_ids: List[str], | ||
| ) -> AsyncGenerator[str, None]: | ||
| """Stream the ask response as Server-Sent Events.""" | ||
| try: | ||
|
|
@@ -76,7 +88,7 @@ async def stream_ask_response( | |
| # LangGraph accepts a partial state dict at runtime, but its typed | ||
| # overloads require the full state type (langgraph typing limitation). | ||
| async for chunk in ask_graph.astream( # type: ignore[call-overload] | ||
| input=dict(question=question), | ||
| input=dict(question=question, notebook_ids=notebook_ids), | ||
| config=dict( | ||
| configurable=dict( | ||
| strategy_model=strategy_model.id, | ||
|
|
@@ -124,6 +136,10 @@ async def stream_ask_response( | |
| async def ask_knowledge_base(ask_request: AskRequest): | ||
| """Ask the knowledge base a question using AI models.""" | ||
| try: | ||
| # Cheapest check first: a malformed or unknown scope fails before any | ||
| # model lookup or embedding check can mask it. | ||
| notebook_ids = await resolve_notebook_scope(ask_request.scope_notebook_ids) | ||
|
|
||
| # Validate models exist | ||
| strategy_model = await Model.get(ask_request.strategy_model) | ||
| answer_model = await Model.get(ask_request.answer_model) | ||
|
|
@@ -155,7 +171,11 @@ async def ask_knowledge_base(ask_request: AskRequest): | |
| # For streaming response | ||
| return StreamingResponse( | ||
| stream_ask_response( | ||
| ask_request.question, strategy_model, answer_model, final_answer_model | ||
| ask_request.question, | ||
| strategy_model, | ||
| answer_model, | ||
| final_answer_model, | ||
| notebook_ids, | ||
| ), | ||
| media_type="text/event-stream", | ||
| headers={ | ||
|
|
@@ -178,6 +198,10 @@ async def ask_knowledge_base(ask_request: AskRequest): | |
| async def ask_knowledge_base_simple(ask_request: AskRequest): | ||
| """Ask the knowledge base a question and return a simple response (non-streaming).""" | ||
| try: | ||
| # Cheapest check first: a malformed or unknown scope fails before any | ||
| # model lookup or embedding check can mask it. | ||
| notebook_ids = await resolve_notebook_scope(ask_request.scope_notebook_ids) | ||
|
|
||
| # Validate models exist | ||
| strategy_model = await Model.get(ask_request.strategy_model) | ||
| answer_model = await Model.get(ask_request.answer_model) | ||
|
|
@@ -211,7 +235,7 @@ async def ask_knowledge_base_simple(ask_request: AskRequest): | |
| # LangGraph accepts a partial state dict at runtime, but its typed | ||
| # overloads require the full state type (langgraph typing limitation). | ||
| async for chunk in ask_graph.astream( # type: ignore[call-overload] | ||
| input=dict(question=ask_request.question), | ||
| input=dict(question=ask_request.question, notebook_ids=notebook_ids), | ||
| config=dict( | ||
| configurable=dict( | ||
| strategy_model=strategy_model.id, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| # ADR-008: Notebook scope is an optional filter on the existing search functions | ||
|
|
||
| - **Status**: Accepted | ||
| - **Date**: 2026-09 | ||
| - **Related**: #574 and #87 (the two requests this resolves), [#1315](https://github.com/lfnovo/open-notebook/discussions/1315) (Evidence-Centered Research — the larger retrieval contract), [ADR-006](ADR-006-migration-granularity.md) (migration granularity) | ||
|
|
||
| ## Context | ||
|
|
||
| Search and Ask always ran against the whole knowledge base, and `notebook_id` was accepted by `POST /api/search` but silently ignored because the request model never declared it. Two long-standing requests (#574, API; #87, UI) asked for the same thing: limit Search and Ask to one or more notebooks. Discussion #1315 places scoped retrieval inside a broader design — a retrieval contract shared by Search, Ask and chat, an evidence bundle, validated citations. That design is deliberated until October 2026 and will land in stages, so the question was whether to wait for it or ship the filter now. | ||
|
|
||
| ## Decision | ||
|
|
||
| **Ship notebook scope as an optional, backward-compatible filter on the existing SurrealQL search functions, exposed on both endpoints and the Search/Ask page. Treat it as the first, smallest block of #1315, not a competing design.** | ||
|
|
||
| Semantics: | ||
|
|
||
| - The scope is a **set of notebook ids**. Empty or absent means the whole knowledge base — the historical behavior, unchanged. | ||
| - A source matches when it is linked to **any** selected notebook (`reference` edge); a note when linked via the `artifact` edge. Source chunks and insights follow their parent source. Membership is resolved once per call, not per row. | ||
| - The scope lives **in the database functions** (`fn::text_search`, `fn::vector_search`) as a trailing `option<array<record<notebook>>>` parameter, so text search, vector search and every Ask fan-out apply the same rule. The previous call shapes still work. | ||
| - The API accepts both `notebook_ids` (list, at most 50) and `notebook_id` (single — the shape #574 proposed and clients already send); they are merged. Ids are validated in the domain layer before the query: malformed → 400, unknown → 404, so a typo never looks like "no matches". | ||
| - Search and Ask on the page share one selector and one scope state. | ||
|
|
||
| ## Alternatives considered | ||
|
|
||
| - **Wait for the #1315 retrieval contract** — rejected: the need is confirmed and years old, and the filter is orthogonal to how evidence is bundled or cited. The contract will consume the same `notebook_ids` parameter. | ||
| - **Filter in Python after the query** — rejected: the SurrealQL functions apply `LIMIT` before returning, so post-filtering would starve scoped results and waste the FTS/vector work. | ||
| - **Single `notebook_id` only** — rejected: #87 asked for multi-select, and a list subsumes the single case. The single field stays as a convenience for existing clients. | ||
| - **Per-notebook embedding tables** (proposed on #87) — rejected: an index-layout change for a filter; out of scope, and #1315 may revisit indexing on its own terms. | ||
|
|
||
| ## Consequences | ||
|
|
||
| - Adding "notebook plus specific sources" or other scope dimensions later means extending the parameter list of the same functions (new migration), not a new engine. | ||
| - Any new caller of the search functions can ignore the scope and keep global behavior; callers that need scope pass the same list. Chat context (#1315 "Auto" option) can reuse `resolve_notebook_scope` and the functions as-is. | ||
| - The migration follows ADR-006: one migration (24), with a `_down` restoring the previous definitions. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| import { render, screen, fireEvent } from '@testing-library/react' | ||
| import { describe, it, expect, vi, beforeEach } from 'vitest' | ||
| import { NotebookScopeSelector } from './NotebookScopeSelector' | ||
| import { useNotebooks } from '@/lib/hooks/use-notebooks' | ||
|
|
||
| // useTranslation is mocked globally in setup.ts (t returns the key string) | ||
|
|
||
| vi.mock('@/lib/hooks/use-notebooks', () => ({ | ||
| useNotebooks: vi.fn(), | ||
| })) | ||
|
|
||
| const mockUseNotebooks = vi.mocked(useNotebooks) | ||
|
|
||
| const notebooks = [ | ||
| { id: 'notebook:a', name: 'Alpha', description: '' }, | ||
| { id: 'notebook:b', name: 'Beta', description: 'second' }, | ||
| ] | ||
|
|
||
| describe('NotebookScopeSelector', () => { | ||
| beforeEach(() => { | ||
| vi.clearAllMocks() | ||
| mockUseNotebooks.mockReturnValue({ data: notebooks, isLoading: false } as ReturnType<typeof useNotebooks>) | ||
| }) | ||
|
|
||
| it('reads as "all notebooks" when nothing is selected and hides the clear action', () => { | ||
| render(<NotebookScopeSelector selectedIds={[]} onChange={vi.fn()} />) | ||
| expect(screen.getByText('searchPage.scopeAllNotebooks')).toBeInTheDocument() | ||
| expect(screen.queryByText('searchPage.scopeClear')).not.toBeInTheDocument() | ||
| }) | ||
|
|
||
| it('adds a notebook to the scope when its checkbox is toggled', () => { | ||
| const onChange = vi.fn() | ||
| render(<NotebookScopeSelector selectedIds={['notebook:a']} onChange={onChange} />) | ||
|
|
||
| fireEvent.click(screen.getByRole('button', { name: /searchPage.scopeNotebooks/ })) | ||
| fireEvent.click(screen.getByRole('checkbox', { name: /Beta/ })) | ||
|
|
||
| expect(onChange).toHaveBeenCalledWith(['notebook:a', 'notebook:b']) | ||
| }) | ||
|
|
||
| it('removes an already selected notebook when toggled again', () => { | ||
| const onChange = vi.fn() | ||
| render(<NotebookScopeSelector selectedIds={['notebook:a', 'notebook:b']} onChange={onChange} />) | ||
|
|
||
| fireEvent.click(screen.getByRole('button', { name: /searchPage.scopeNotebooks/ })) | ||
| fireEvent.click(screen.getByRole('checkbox', { name: /Alpha/ })) | ||
|
|
||
| expect(onChange).toHaveBeenCalledWith(['notebook:b']) | ||
| }) | ||
|
|
||
| it('clears the whole scope from the clear action', () => { | ||
| const onChange = vi.fn() | ||
| render(<NotebookScopeSelector selectedIds={['notebook:a']} onChange={onChange} />) | ||
|
|
||
| fireEvent.click(screen.getByText('searchPage.scopeClear')) | ||
|
|
||
| expect(onChange).toHaveBeenCalledWith([]) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: Custom agent: Vision & principles alignment
This PR introduces a new shared API and data-layer scoping contract without the required decision record. Add a short ADR or PDR in
docs/7-DEVELOPMENT/decisions/covering the notebook-scope semantics, compatibility choice, and consequences.Prompt for AI agents