diff --git a/docsgpt/api/user/sources/routes.py b/docsgpt/api/user/sources/routes.py index 0930d689..9043e566 100644 --- a/docsgpt/api/user/sources/routes.py +++ b/docsgpt/api/user/sources/routes.py @@ -46,6 +46,9 @@ from docsgpt.vectorstore.vector_creator import VectorCreator WIKI_INDEX_PATH = "/index.md" +# Longest graph-node search term forwarded to the store; longer input is truncated. +_GRAPH_SEARCH_MAX_LEN = 200 + sources_ns = Namespace( "sources", description="Source document management operations", path="/api" @@ -1442,7 +1445,7 @@ class SourceGraphNodes(Resource): GRAPH_NODE_LIST_MAX_LIMIT, ), ) - query = (request.args.get("q") or "").strip() or None + query = (request.args.get("q") or "").strip()[:_GRAPH_SEARCH_MAX_LEN] or None type_key = request.args.get("type") try: with db_readonly() as conn: diff --git a/docsgpt/core/url_validation.py b/docsgpt/core/url_validation.py index eaa4cf56..974bdb6d 100644 --- a/docsgpt/core/url_validation.py +++ b/docsgpt/core/url_validation.py @@ -112,17 +112,22 @@ def validate_url(url: str, allow_localhost: bool = False) -> str: """ if not url or not isinstance(url, str): raise SSRFError("No URL was provided.") - # Ensure URL has a scheme - if not urlparse(url).scheme: - url = "http://" + url + # ``urlparse`` and ``.hostname`` raise ValueError on malformed input such + # as an unclosed IPv6 bracket (``http://[bad``); callers only catch SSRFError. + try: + # Ensure URL has a scheme + if not urlparse(url).scheme: + url = "http://" + url - parsed = urlparse(url) + parsed = urlparse(url) + hostname = parsed.hostname + except ValueError as e: + raise SSRFError(f"Invalid URL: {e}") from e # Check scheme if parsed.scheme not in ALLOWED_SCHEMES: raise SSRFError(f"URL scheme '{parsed.scheme}' is not allowed. Only HTTP(S) is permitted.") - hostname = parsed.hostname if not hostname: raise SSRFError("URL must have a valid hostname.") diff --git a/docsgpt/parser/remote/base.py b/docsgpt/parser/remote/base.py index 2cbfaf37..3f2a8e7e 100644 --- a/docsgpt/parser/remote/base.py +++ b/docsgpt/parser/remote/base.py @@ -4,7 +4,7 @@ import os import re from abc import abstractmethod from typing import Any, Dict, List -from urllib.parse import parse_qsl, urldefrag, urlencode, urlparse +from urllib.parse import parse_qsl, urlencode, urlparse, urlsplit, urlunsplit from docsgpt.parser.schema.base import Document from docsgpt.vectorstore.document_class import Document as VectorDocument @@ -99,13 +99,44 @@ def url_to_virtual_path(url: str, include_host: bool = False) -> str: def spans_multiple_hosts(urls: List[str]) -> bool: """Return whether ``urls`` point at more than one host. + A URL ``urlparse`` rejects (an unclosed IPv6 bracket, say) is left out: + it fails validation and is never fetched, so it names no host here. + Args: urls: Page URLs about to be ingested together. Returns: True when paths alone could collide and need a host prefix. """ - return len({urlparse(u).netloc for u in urls}) > 1 + hosts = set() + for url in urls: + try: + hosts.add(urlparse(url).netloc) + except ValueError: + continue + return len(hosts) > 1 + + +def normalize_page_url(url: str) -> str: + """Return the identity of the page ``url`` names, for deduplication. + + The fragment is dropped (it names a spot on the same page) and the query + parameters are sorted, as ``_query_segment`` sorts them, so + ``/p?a=1&b=2`` and ``/p?b=2&a=1#top`` are one page. The result is a key + only; fetch the URL as it was written. + + Args: + url: Page URL. + + Returns: + The normalized URL. + + Raises: + ValueError: If ``urlsplit`` cannot parse ``url``. + """ + parts = urlsplit(url) + query = urlencode(sorted(parse_qsl(parts.query, keep_blank_values=True))) + return urlunsplit(parts._replace(query=query, fragment="")) def dedupe_virtual_paths(documents: List[Document]) -> List[Document]: @@ -114,11 +145,13 @@ def dedupe_virtual_paths(documents: List[Document]) -> List[Document]: ``url_to_virtual_path`` folds some different URLs onto one path (``/a`` and ``/a.html`` are both ``a.md``), and the worker would then merge those pages into one tree entry. Within a path, the pages are ordered by URL (fragment - dropped): the first keeps the path and each later one gets ``-2``, ``-3`` - and so on before ``.md``, skipping any path another page already has. + dropped, query parameters sorted; see ``normalize_page_url``): the first + keeps the path and each later one gets ``-2``, ``-3`` and so on before + ``.md``, skipping any path another page already has. Ordering by URL rather than by fetch order keeps the names stable across re-syncs of the same pages. Documents for the same URL (one page reached - via two fragments) are one page and keep one path. + via two fragments, or with its query parameters in another order) are one + page and keep one path. Args: documents: Loaded documents; each ``extra_info`` carries ``source`` @@ -133,7 +166,7 @@ def dedupe_virtual_paths(documents: List[Document]) -> List[Document]: path = info.get("file_path") if not path: continue - page = urldefrag(str(info.get("source") or path))[0] + page = normalize_page_url(str(info.get("source") or path)) pages_by_path.setdefault(path, {}).setdefault(page, []).append(doc) taken = set(pages_by_path) diff --git a/docsgpt/parser/remote/crawler_loader.py b/docsgpt/parser/remote/crawler_loader.py index e6d2b8d5..e1e68858 100644 --- a/docsgpt/parser/remote/crawler_loader.py +++ b/docsgpt/parser/remote/crawler_loader.py @@ -2,7 +2,7 @@ import logging from bs4 import BeautifulSoup from urllib.parse import urljoin, urlparse -from docsgpt.parser.remote.base import BaseRemote, dedupe_virtual_paths, url_to_virtual_path +from docsgpt.parser.remote.base import BaseRemote, dedupe_virtual_paths, normalize_page_url, url_to_virtual_path from docsgpt.parser.schema.base import Document from docsgpt.core.url_validation import validate_url, SSRFError from docsgpt.security.safe_url import pinned_request @@ -24,6 +24,8 @@ class CrawlerLoader(BaseRemote): logging.error(f"URL validation failed: {e}") return [] + # Keyed by normalize_page_url, so fragment and query-order variants of + # one page are fetched once. visited_urls = set() base_url = urlparse(url).scheme + "://" + urlparse(url).hostname urls_to_visit = [url] @@ -31,7 +33,10 @@ class CrawlerLoader(BaseRemote): while urls_to_visit: current_url = urls_to_visit.pop(0) - visited_urls.add(current_url) + page_key = normalize_page_url(current_url) + if page_key in visited_urls: + continue + visited_urls.add(page_key) try: response = pinned_request("GET", current_url, timeout=30) @@ -57,15 +62,21 @@ class CrawlerLoader(BaseRemote): logging.error(f"Error processing URL {current_url}: {e}", exc_info=True) continue - # Parse the HTML content to extract all links - all_links = [ - urljoin(current_url, a['href']) - for a in soup.find_all('a', href=True) - if base_url in urljoin(current_url, a['href']) - ] + # Parse the HTML content to extract all links. A malformed href + # (``http://[bad``) makes urljoin or the key parse raise; skip it + # rather than abort the crawl. + all_links = [] + for a in soup.find_all('a', href=True): + try: + link = urljoin(current_url, a['href']) + link_key = normalize_page_url(link) + except ValueError: + continue + if base_url in link and link_key not in visited_urls: + all_links.append(link) # Add new links to the list of URLs to visit if they haven't been visited yet - urls_to_visit.extend([link for link in all_links if link not in visited_urls]) + urls_to_visit.extend(all_links) urls_to_visit = list(set(urls_to_visit)) # Stop crawling if the limit of pages to scrape is reached diff --git a/docsgpt/parser/remote/crawler_markdown.py b/docsgpt/parser/remote/crawler_markdown.py index db8693cc..5812391e 100644 --- a/docsgpt/parser/remote/crawler_markdown.py +++ b/docsgpt/parser/remote/crawler_markdown.py @@ -1,6 +1,6 @@ from urllib.parse import urlparse, urljoin from bs4 import BeautifulSoup -from docsgpt.parser.remote.base import BaseRemote, dedupe_virtual_paths, url_to_virtual_path +from docsgpt.parser.remote.base import BaseRemote, dedupe_virtual_paths, normalize_page_url, url_to_virtual_path from docsgpt.core.url_validation import validate_url, SSRFError from docsgpt.security.safe_url import UnsafeUserUrlError, pinned_request import re @@ -37,7 +37,8 @@ class CrawlerLoader(BaseRemote): print(f"URL validation failed: {e}") return [] - # Keep track of visited URLs to avoid revisiting the same page + # Keep track of visited pages to avoid revisiting the same page. Keyed + # by normalize_page_url, so fragment and query-order variants are one. visited_urls = set() # Determine the base domain for link filtering using tldextract @@ -49,9 +50,10 @@ class CrawlerLoader(BaseRemote): current_url = urls_to_visit.pop() # Skip if already visited - if current_url in visited_urls: + page_key = normalize_page_url(current_url) + if page_key in visited_urls: continue - visited_urls.add(current_url) + visited_urls.add(page_key) # Fetch the page content html_content = self._fetch_page(current_url) @@ -84,7 +86,9 @@ class CrawlerLoader(BaseRemote): filtered_links = self._filter_links(new_links, base_domain) # Add any new, not-yet-visited links to the queue - urls_to_visit.update(link for link in filtered_links if link not in visited_urls) + urls_to_visit.update( + link for link in filtered_links if normalize_page_url(link) not in visited_urls + ) # If we've reached the limit, stop crawling if self.limit is not None and len(visited_urls) >= self.limit: @@ -123,7 +127,13 @@ class CrawlerLoader(BaseRemote): soup = BeautifulSoup(html_content, 'html.parser') links = [] for a in soup.find_all('a', href=True): - full_url = urljoin(current_url, a['href']) + # A malformed href (``http://[bad``) makes urljoin or the later + # parse raise; skip that link rather than abort the crawl. + try: + full_url = urljoin(current_url, a['href']) + normalize_page_url(full_url) + except ValueError: + continue links.append((full_url, a.text.strip())) return links diff --git a/docsgpt/storage/db/repositories/schedule_runs.py b/docsgpt/storage/db/repositories/schedule_runs.py index 95311d45..1156ffe0 100644 --- a/docsgpt/storage/db/repositories/schedule_runs.py +++ b/docsgpt/storage/db/repositories/schedule_runs.py @@ -173,7 +173,8 @@ class ScheduleRunsRepository: """Aggregate run stats for an agent's owned schedules over a window. Only runs of schedules owned by ``user_id`` on ``agent_id`` whose - ``scheduled_for`` falls within the last ``days`` days are counted. + ``scheduled_for`` falls within the last ``days`` days are counted; + runs scheduled in the future are not. Args: agent_id: Agent UUID the schedules belong to. @@ -198,6 +199,7 @@ class ScheduleRunsRepository: AND s.user_id = :user_id AND r.user_id = :user_id AND r.scheduled_for >= now() - make_interval(days => :days) + AND r.scheduled_for <= now() ), latest_failure AS ( SELECT scheduled_for, status, error_type diff --git a/docsgpt/vectorstore/base.py b/docsgpt/vectorstore/base.py index 348f99e5..5498098f 100644 --- a/docsgpt/vectorstore/base.py +++ b/docsgpt/vectorstore/base.py @@ -455,15 +455,29 @@ class BaseVectorStore(ABC): Returns: The id the updated chunk is stored under. + + Raises: + RuntimeError: The old chunk could not be deleted. The new chunk is + deleted again (best effort) so no duplicate is left behind. """ new_chunk_id = self.add_chunk(text, metadata) - if not self.delete_chunk(chunk_id): - logging.warning( - "Failed to delete old chunk %s, but new chunk %s was created", - chunk_id, + delete_error: Optional[Exception] = None + try: + deleted = self.delete_chunk(chunk_id) + except Exception as err: + deleted, delete_error = False, err + if deleted: + return new_chunk_id + try: + self.delete_chunk(new_chunk_id) + except Exception: + logging.error( + "Failed to roll back new chunk %s after old chunk %s could not be deleted", new_chunk_id, + chunk_id, + exc_info=True, ) - return new_chunk_id + raise RuntimeError(f"Failed to delete old chunk {chunk_id} during update") from delete_error def delete_chunks_by_source_path(self, path) -> int: """Delete every chunk whose ``metadata.source`` equals ``path``. diff --git a/frontend/eslint/card-surfaces.js b/frontend/eslint/card-surfaces.js index d5ca6a29..3c5a5cca 100644 --- a/frontend/eslint/card-surfaces.js +++ b/frontend/eslint/card-surfaces.js @@ -6,8 +6,11 @@ // JSX tree as the ``, not ones a child component // renders. -const FILLED_CARD = - 'JSXElement:has(> JSXOpeningElement[name.name="Card"]:has(> JSXAttribute[name.name="variant"][value.value="filled"]))'; +// `variant="filled"` or `variant={'filled'}`. +const FILLED_VARIANT = + 'JSXAttribute[name.name="variant"]:matches([value.value="filled"], [value.expression.value="filled"])'; + +const FILLED_CARD = `JSXElement:has(> JSXOpeningElement[name.name="Card"]:has(> ${FILLED_VARIANT}))`; // `bg-muted` or `bg-muted/NN`, with any variant prefix, but not // `bg-muted-foreground`. @@ -16,7 +19,7 @@ const BG_MUTED = '/(^|[\\s:])bg-muted([^-a-z]|$)/'; /** @type {{ selector: string, message: string }[]} */ export const cardSurfaceSelectors = [ { - selector: `${FILLED_CARD} JSXElement > JSXOpeningElement[name.name="Card"] > JSXAttribute[name.name="variant"][value.value="filled"]`, + selector: `${FILLED_CARD} JSXElement > JSXOpeningElement[name.name="Card"] > ${FILLED_VARIANT}`, message: 'A filled Card inside a filled Card is a fill on a fill. A well inside a tile needs a panel around it, or none. See DESIGN.md "Card surfaces".', }, diff --git a/frontend/eslint/design-rules.js b/frontend/eslint/design-rules.js index 8473de38..ba5d0bbd 100644 --- a/frontend/eslint/design-rules.js +++ b/frontend/eslint/design-rules.js @@ -31,7 +31,7 @@ export const everywhereSelectors = [ 'z-index comes from the stacking layers: z-10 sticky headers, z-20 in-page floating chrome, z-50 overlays, z-200 portalled floating lists. See DESIGN.md "Stacking".', ), ...inStrings( - '/(^|\\s)(focus|focus-visible|focus-within):ring-2(\\s|$)/', + '/(^|[\\s:-])(focus|focus-visible|focus-within):ring-2(\\s|$)/', 'The focus ring is ring-3 ring-ring/50 on focus-visible (fields: focus-within on a frame the same way). ring-2 is only for selection rings. See DESIGN.md "Focus ring".', ), { @@ -49,7 +49,7 @@ export const pageSelectors = [ 'Transition only the property that changes: transition-colors by default, transition-transform for chevrons, transition-shadow for a ring. See DESIGN.md "Motion".', ), ...inStrings( - '/(^|\\s)hover:scale-(?!x-|y-)/', + '/(^|[\\s:-])hover:scale-(?!x-|y-)/', 'Hover is a fill, border or text-colour change, never a scale. See DESIGN.md "Motion".', ), ...inStrings( @@ -82,8 +82,8 @@ export const pageSelectors = [ 'Every table is ui/table (Table, TableHead, TableRow…), never a raw . See DESIGN.md "Table".', }, ...[ - 'JSXOpeningElement[name.name="a"] > JSXAttribute[name.name="className"] Literal[value=/(^|\\s)(underline|text-primary)(\\s|$)/]', - 'JSXOpeningElement[name.name="a"] > JSXAttribute[name.name="className"] TemplateElement[value.raw=/(^|\\s)(underline|text-primary)(\\s|$)/]', + 'JSXOpeningElement[name.name="a"] > JSXAttribute[name.name="className"] Literal[value=/(^|[\\s:])(underline|text-primary)(\\s|$)/]', + 'JSXOpeningElement[name.name="a"] > JSXAttribute[name.name="className"] TemplateElement[value.raw=/(^|[\\s:])(underline|text-primary)(\\s|$)/]', ].map((selector) => ({ selector, message: diff --git a/frontend/src/components/Chunks.test.tsx b/frontend/src/components/Chunks.test.tsx index de91252e..c1778a9a 100644 --- a/frontend/src/components/Chunks.test.tsx +++ b/frontend/src/components/Chunks.test.tsx @@ -789,18 +789,24 @@ describe('Chunks', () => { expect(container.querySelector('h1')?.textContent).toBe('Second'); }); - it('an edited chunk the store moved elsewhere is found by its id', async () => { + it('an edited chunk the store moved off the probed positions keeps what is shown', async () => { serveStore(1); await render({ embedded: true }); const tiles = container.querySelectorAll( 'button[data-slot="card"]', ); - await act(async () => tiles[2].click()); - await editOpenChunk('# Third edited'); - expect(container.querySelector('h1')?.textContent).toBe('Third edited'); - expect(position()).toEqual([2, 3]); - await act(async () => buttonByLabel('settings.sources.nextChunk')!.click()); - expect(container.querySelector('h1')?.textContent).toBe('Second'); + await act(async () => tiles[0].click()); + service.getDocumentChunks.mockClear(); + await editOpenChunk('# First edited'); + expect(container.querySelector('h1')?.textContent).toBe('First edited'); + expect(position()).toEqual([1, 3]); + // Only the open and last positions are probed: the whole filtered list + // (one page of `total`) is never fetched. + const sizes = service.getDocumentChunks.mock.calls.map( + (call: unknown[]) => call[2], + ); + expect(sizes).toContain(1); + expect(sizes).not.toContain(3); }); it('an edited chunk that keeps its place stays put', async () => { diff --git a/frontend/src/components/Chunks.tsx b/frontend/src/components/Chunks.tsx index 7ecd0ced..bd1c4039 100644 --- a/frontend/src/components/Chunks.tsx +++ b/frontend/src/components/Chunks.tsx @@ -211,10 +211,10 @@ const Chunks: React.FC = ({ * After a save, find the edited chunk in the store's current order and show * its stored copy (fresh metadata) at that position, so "n of total" and * previous / next follow the list as it now is. Checked in turn: the open - * position (the chunk stayed), the last one (a store that appends the new - * copy), then the whole filtered list. Each probe matches by id, so no - * store ordering is assumed. When the chunk is not in the list (a search - * it no longer matches), what is shown stays. + * position (the chunk stayed), then the last one (a store that re-adds the + * copy at the end). Each probe matches by id, so no store ordering is + * assumed. When neither holds it (moved elsewhere, or a search it no longer + * matches), what is shown stays: the whole filtered list is never fetched. * * @param chunk The chunk just saved, with its new id. */ @@ -249,25 +249,11 @@ const Chunks: React.FC = ({ return; } if (total < 1) return; - if (total !== openPosition) { - const last = await fetchPage(total, 1); - if (!last) return; - const lastChunk: ChunkType | undefined = last.chunks?.[0]; - if (lastChunk?.doc_id === chunk.doc_id) { - show(lastChunk, total, total); - return; - } - } - const all = await fetchPage(1, total); - if (!all) return; - const list: ChunkType[] = all.chunks ?? []; - const index = list.findIndex((c) => c.doc_id === chunk.doc_id); - if (index >= 0) - show( - list[index], - index + 1, - typeof all.total === 'number' ? all.total : total, - ); + if (total === openPosition) return; + const last = await fetchPage(total, 1); + if (!last) return; + const lastChunk: ChunkType | undefined = last.chunks?.[0]; + if (lastChunk?.doc_id === chunk.doc_id) show(lastChunk, total, total); } catch (error) { console.error(error); } @@ -394,10 +380,11 @@ const Chunks: React.FC = ({ if (!response.ok) throw new Error('Failed to update chunk'); const data = await response.json().catch(() => ({})); closeSheet(); - // The edit is saved under a new id. Show it straight away (its token - // count is recomputed server-side, so it is dropped until the reload), - // then find where the store now keeps it: the update re-adds the chunk, - // so it can move (FAISS appends it; pgvector has no ORDER BY). + // Show the edit straight away (its token count is recomputed + // server-side, so it is dropped until the reload), then find where the + // store now keeps it. Most stores (FAISS, MongoDB, pgvector, Qdrant) + // update in place: same id, same position. Only the base add-then-delete + // fallback (Milvus) saves it under a new id, which can move it. const edited: ChunkType = { ...chunk, doc_id: data.chunk_id ?? chunk.doc_id, diff --git a/frontend/src/design/cardSurfaces.lint.test.ts b/frontend/src/design/cardSurfaces.lint.test.ts index 30ef1d07..7027ca3d 100644 --- a/frontend/src/design/cardSurfaces.lint.test.ts +++ b/frontend/src/design/cardSurfaces.lint.test.ts @@ -27,6 +27,11 @@ function lint(code: string): string[] { describe('card surface lint: no fill on a fill', () => { it('rejects bg-muted on an element inside a filled tile, in cn() too', () => { + expect( + lint( + `
x
`, + ), + ).toHaveLength(1); expect( lint( `
x
`, @@ -45,12 +50,25 @@ describe('card surface lint: no fill on a fill', () => { `x`, ), ).toHaveLength(1); + expect( + lint( + `x`, + ), + ).toHaveLength(1); + expect( + lint( + `x`, + ), + ).toHaveLength(1); }); it('rejects a Skeleton without surface="muted" on a filled tile', () => { expect( lint(``), ).toHaveLength(1); + expect( + lint(``), + ).toHaveLength(1); expect( lint( ``, diff --git a/frontend/src/design/designRules.lint.test.ts b/frontend/src/design/designRules.lint.test.ts index 654b06a8..fc03cf92 100644 --- a/frontend/src/design/designRules.lint.test.ts +++ b/frontend/src/design/designRules.lint.test.ts @@ -44,14 +44,24 @@ describe('design rules lint', () => { ['off-scale z', `
`], ['arbitrary z', `
`], ['focus ring-2', `
`], + ['md: focus ring-2', `
`], + ['dark: focus ring-2', `
`], + ['peer focus ring-2', `
`], ['lucide loader', `import { Loader2 } from 'lucide-react';`], ['transition-all', `
`], ['hover scale', `
`], + ['md:hover scale', `
`], + ['group-hover scale', `
`], ['bg-primary/10', `const tone = 'bg-primary/10 text-primary';`], ['native checkbox', ``], ['title on Button', `
`], ['link-styled a', `x`], + ['hover: underline a', `x`], + [ + 'dark: text-primary a', + `x`, + ], ['drawer width', ``], ['drawer w-', ``], ])('rejects %s', (_name, code) => { @@ -70,6 +80,9 @@ describe('design rules lint', () => { ['size and max-w', `
`], ['selection ring-2', `
`], ['hover:scale-x underline wipe', ``], + ['md:hover:scale-x wipe', ``], + ['no-underline a', `x`], + ['underline-offset a', `x`], ['transition-colors', `
`], [ 'other lucide icons', diff --git a/frontend/src/locale/de.json b/frontend/src/locale/de.json index 6373950f..777ac34f 100644 --- a/frontend/src/locale/de.json +++ b/frontend/src/locale/de.json @@ -1101,7 +1101,6 @@ "dropzoneText": "Zum Hochladen klicken oder per Drag & Drop ablegen", "dropzoneHint": "PDF, Word, Excel, PowerPoint, Markdown, HTML, Bilder, Audio, EPUB, JSON oder ZIP, jeweils bis zu 25 MB", "filesRejected": "Nicht hinzugefügt (größer als 25 MB oder kein unterstützter Dateityp): {{files}}", - "info": "Bitte lade .pdf, .txt, .rst, .csv, .xlsx, .xlsm, .xlsb, .xls, .ods, .docx, .docm, .doc, .odt, .rtf, .md, .html, .xhtml, .png, .jpg, .jpeg, .epub, .json, .pptx, .pptm, .ppt, .pps, .ppsx, .ppsm, .pot, .odp, .zip hoch (max. 25 MB)", "uploadedFiles": "Hochgeladene Dateien", "cancel": "Abbrechen", "train": "Trainieren", diff --git a/frontend/src/locale/en.json b/frontend/src/locale/en.json index a9cf4d9e..64710d2a 100644 --- a/frontend/src/locale/en.json +++ b/frontend/src/locale/en.json @@ -1107,7 +1107,6 @@ "dropzoneText": "Click to upload or drag and drop", "dropzoneHint": "PDF, Word, Excel, PowerPoint, Markdown, HTML, images, audio, EPUB, JSON or ZIP, up to 25 MB each", "filesRejected": "Not added (over 25 MB or not a supported type): {{files}}", - "info": "Please upload .pdf, .txt, .rst, .csv, .xlsx, .xlsm, .xlsb, .xls, .ods, .docx, .docm, .doc, .odt, .rtf, .md, .html, .xhtml, .png, .jpg, .jpeg, .epub, .json, .pptx, .pptm, .ppt, .pps, .ppsx, .ppsm, .pot, .odp, .zip limited to 25mb", "uploadedFiles": "Uploaded Files", "cancel": "Cancel", "train": "Train", diff --git a/frontend/src/locale/es.json b/frontend/src/locale/es.json index 0d1de95c..be8701d6 100644 --- a/frontend/src/locale/es.json +++ b/frontend/src/locale/es.json @@ -1101,7 +1101,6 @@ "dropzoneText": "Haz clic para subir o arrastra y suelta", "dropzoneHint": "PDF, Word, Excel, PowerPoint, Markdown, HTML, imágenes, audio, EPUB, JSON o ZIP, hasta 25 MB cada uno", "filesRejected": "No se añadieron (más de 25 MB o tipo no admitido): {{files}}", - "info": "Por favor, sube archivos .pdf, .txt, .rst, .csv, .xlsx, .xlsm, .xlsb, .xls, .ods, .docx, .docm, .doc, .odt, .rtf, .md, .html, .xhtml, .png, .jpg, .jpeg, .epub, .json, .pptx, .pptm, .ppt, .pps, .ppsx, .ppsm, .pot, .odp, .zip limitados a 25MB", "uploadedFiles": "Archivos Subidos", "cancel": "Cancelar", "train": "Entrenar", diff --git a/frontend/src/locale/jp.json b/frontend/src/locale/jp.json index b6545147..55cfe814 100644 --- a/frontend/src/locale/jp.json +++ b/frontend/src/locale/jp.json @@ -1092,7 +1092,6 @@ "dropzoneText": "クリックしてアップロード、またはドラッグ&ドロップ", "dropzoneHint": "PDF、Word、Excel、PowerPoint、Markdown、HTML、画像、音声、EPUB、JSON、ZIP(各 25 MB まで)", "filesRejected": "追加されませんでした(25 MB を超えているか、未対応の形式です): {{files}}", - "info": "25MBまでの.pdf、.txt、.rst、.csv、.xlsx、.xlsm、.xlsb、.xls、.ods、.docx、.docm、.doc、.odt、.rtf、.md、.html、.xhtml、.png、.jpg、.jpeg、.epub、.json、.pptx、.pptm、.ppt、.pps、.ppsx、.ppsm、.pot、.odp、.zipファイルをアップロードしてください", "uploadedFiles": "アップロードされたファイル", "cancel": "キャンセル", "train": "トレーニング", diff --git a/frontend/src/locale/ru.json b/frontend/src/locale/ru.json index b0e770f4..fd694e1e 100644 --- a/frontend/src/locale/ru.json +++ b/frontend/src/locale/ru.json @@ -1157,7 +1157,6 @@ "dropzoneText": "Нажмите, чтобы загрузить, или перетащите файлы", "dropzoneHint": "PDF, Word, Excel, PowerPoint, Markdown, HTML, изображения, аудио, EPUB, JSON или ZIP, до 25 МБ каждый", "filesRejected": "Не добавлены (больше 25 МБ или неподдерживаемый формат): {{files}}", - "info": "Пожалуйста, загрузите файлы .pdf, .txt, .rst, .csv, .xlsx, .xlsm, .xlsb, .xls, .ods, .docx, .docm, .doc, .odt, .rtf, .md, .html, .xhtml, .png, .jpg, .jpeg, .epub, .json, .pptx, .pptm, .ppt, .pps, .ppsx, .ppsm, .pot, .odp, .zip размером до 25 МБ", "uploadedFiles": "Загруженные файлы", "cancel": "Отмена", "train": "Тренировка", diff --git a/frontend/src/locale/zh-TW.json b/frontend/src/locale/zh-TW.json index 33988770..35d1aebc 100644 --- a/frontend/src/locale/zh-TW.json +++ b/frontend/src/locale/zh-TW.json @@ -1092,7 +1092,6 @@ "dropzoneText": "點擊上傳或拖放檔案", "dropzoneHint": "PDF、Word、Excel、PowerPoint、Markdown、HTML、圖片、音訊、EPUB、JSON 或 ZIP,每個最大 25 MB", "filesRejected": "未新增(超過 25 MB 或不支援的類型):{{files}}", - "info": "請上傳限制為25MB的.pdf、.txt、.rst、.csv、.xlsx、.xlsm、.xlsb、.xls、.ods、.docx、.docm、.doc、.odt、.rtf、.md、.html、.xhtml、.png、.jpg、.jpeg、.epub、.json、.pptx、.pptm、.ppt、.pps、.ppsx、.ppsm、.pot、.odp、.zip檔案", "uploadedFiles": "已上傳檔案", "cancel": "取消", "train": "訓練", diff --git a/frontend/src/locale/zh.json b/frontend/src/locale/zh.json index 5cb22646..e63ea714 100644 --- a/frontend/src/locale/zh.json +++ b/frontend/src/locale/zh.json @@ -1092,7 +1092,6 @@ "dropzoneText": "点击上传或拖放文件", "dropzoneHint": "PDF、Word、Excel、PowerPoint、Markdown、HTML、图片、音频、EPUB、JSON 或 ZIP,每个最大 25 MB", "filesRejected": "未添加(超过 25 MB 或不支持的类型):{{files}}", - "info": "请上传限制为25MB的.pdf、.txt、.rst、.csv、.xlsx、.xlsm、.xlsb、.xls、.ods、.docx、.docm、.doc、.odt、.rtf、.md、.html、.xhtml、.png、.jpg、.jpeg、.epub、.json、.pptx、.pptm、.ppt、.pps、.ppsx、.ppsm、.pot、.odp、.zip文件", "uploadedFiles": "已上传文件", "cancel": "取消", "train": "训练", diff --git a/frontend/src/settings/Prompts.test.tsx b/frontend/src/settings/Prompts.test.tsx index 39c2fcef..e45b5aef 100644 --- a/frontend/src/settings/Prompts.test.tsx +++ b/frontend/src/settings/Prompts.test.tsx @@ -93,6 +93,7 @@ describe('Prompts', () => { const row = container.querySelector('[data-slot="setting-row"]')!; const label = row.querySelector('label')!; expect(label.textContent).toBe('settings.general.prompt'); + expect(trigger.id).toBeTruthy(); expect(label.htmlFor).toBe(trigger.id); expect(row.textContent).toContain('Used without an agent.'); expect(trigger.hasAttribute('aria-label')).toBe(false); @@ -140,6 +141,7 @@ describe('Prompts', () => { const trigger = container.querySelector( 'button[role="combobox"]', )!; + expect(trigger.id).toBeTruthy(); expect(label.htmlFor).toBe(trigger.id); expect(trigger.hasAttribute('aria-label')).toBe(false); expect(trigger.className).toContain('w-full'); diff --git a/tests/api/user/sources/test_chunks.py b/tests/api/user/sources/test_chunks.py index b46b30ee..052e54b4 100644 --- a/tests/api/user/sources/test_chunks.py +++ b/tests/api/user/sources/test_chunks.py @@ -810,3 +810,40 @@ class TestUpdateChunkGraphLinks: graph_store.remap_chunk.assert_called_once_with( str(src["id"]), "chunk-old", "chunk-new" ) + + def test_default_fallback_failed_delete_returns_500_without_duplicate(self, app, pg_conn): + """A failed old-chunk delete rolls the new chunk back and fails the request.""" + from docsgpt.vectorstore.base import BaseVectorStore + + class _FallbackStore(BaseVectorStore): + def __init__(self): + super().__init__() + self.deleted = [] + + def search(self, *args, **kwargs): + return [] + + def add_texts(self, texts, metadatas=None, *args, **kwargs): + return [] + + def get_chunks(self): + return [{"doc_id": "chunk-old", "text": "old", "metadata": {}}] + + def add_chunk(self, text, metadata=None): + return "chunk-new" + + def delete_chunk(self, chunk_id): + self.deleted.append(chunk_id) + return chunk_id != "chunk-old" + + user = "u-upd-graph-fallback-fail" + src = self._graph_source(pg_conn, user) + graph_store = MagicMock() + store = _FallbackStore() + store_proxy = MagicMock(wraps=store) + + response = self._put(app, pg_conn, src, user, graph_store, store_proxy) + + assert response.status_code == 500 + assert store.deleted == ["chunk-old", "chunk-new"] + graph_store.remap_chunk.assert_not_called() diff --git a/tests/api/user/sources/test_graph_view.py b/tests/api/user/sources/test_graph_view.py index 8d06004f..01203623 100644 --- a/tests/api/user/sources/test_graph_view.py +++ b/tests/api/user/sources/test_graph_view.py @@ -282,6 +282,27 @@ class TestSourceGraphNodes: sid, query="ada", type_key="person", offset=200, limit=100 ) + def test_caps_search_query_length(self, app, pg_conn): + from docsgpt.api.user.sources.routes import SourceGraphNodes + + user = "u-graph-nodes-long-q" + sid = _graphrag_source(pg_conn, user) + store = _node_list_store() + + with _patch_db(pg_conn), patch( + "docsgpt.graphrag.store.GraphStore", return_value=store + ), app.test_request_context( + f"/api/sources/{sid}/graph/nodes?q=%20{'a' * 500}" + ): + from flask import request + request.decoded_token = {"sub": user} + response = SourceGraphNodes().get(sid) + + assert response.status_code == 200 + store.list_nodes.assert_called_once_with( + sid, query="a" * 200, type_key=None, offset=0, limit=25 + ) + @pytest.mark.parametrize( "qs, page, per_page", [ diff --git a/tests/core/test_url_validation.py b/tests/core/test_url_validation.py index 4329054e..f06b893d 100644 --- a/tests/core/test_url_validation.py +++ b/tests/core/test_url_validation.py @@ -259,3 +259,22 @@ class TestValidateUrlExtended: def test_allows_localhost_ip_with_flag(self): result = validate_url("http://10.0.0.1", allow_localhost=True) assert result == "http://10.0.0.1" + + +class TestValidateUrlMalformed: + """A URL ``urlparse`` cannot parse fails validation instead of escaping.""" + + def test_unclosed_ipv6_bracket_raises_ssrf_error(self): + with pytest.raises(SSRFError) as exc_info: + validate_url("http://[bad") + assert "invalid url" in str(exc_info.value).lower() + + def test_unclosed_ipv6_bracket_without_scheme_raises_ssrf_error(self): + with pytest.raises(SSRFError): + validate_url("[bad") + + def test_safe_variant_reports_failure(self): + is_valid, url, error = validate_url_safe("http://[bad") + assert is_valid is False + assert url == "http://[bad" + assert error diff --git a/tests/parser/remote/test_base.py b/tests/parser/remote/test_base.py index 2f12f427..6b68ae8e 100644 --- a/tests/parser/remote/test_base.py +++ b/tests/parser/remote/test_base.py @@ -5,6 +5,7 @@ import pytest from docsgpt.parser.remote.base import ( MAX_QUERY_SEGMENT_LENGTH, dedupe_virtual_paths, + normalize_page_url, url_to_virtual_path, ) from docsgpt.parser.schema.base import Document @@ -131,3 +132,29 @@ class TestDedupeVirtualPaths: assert dedupe_virtual_paths([doc]) == [doc] assert "file_path" not in doc.extra_info + + +@pytest.mark.unit +class TestNormalizePageUrl: + def test_query_order_does_not_matter(self): + assert normalize_page_url("https://x.io/p?a=1&b=2") == normalize_page_url("https://x.io/p?b=2&a=1") + + def test_fragment_is_dropped(self): + assert normalize_page_url("https://x.io/p?a=1#top") == normalize_page_url("https://x.io/p?a=1") + + def test_distinct_queries_stay_distinct(self): + assert normalize_page_url("https://x.io/p?page=1") != normalize_page_url("https://x.io/p?page=2") + + def test_blank_values_are_kept(self): + assert normalize_page_url("https://x.io/p?flag") != normalize_page_url("https://x.io/p") + + def test_url_without_query_or_fragment_is_unchanged(self): + assert normalize_page_url("https://x.io/guides/setup") == "https://x.io/guides/setup" + + +@pytest.mark.unit +class TestDedupeReorderedQuery: + def test_reordered_query_is_one_page(self): + docs = [_doc("https://x.com/p?a=1&b=2"), _doc("https://x.com/p?b=2&a=1")] + + assert _paths(dedupe_virtual_paths(docs)) == ["p__a=1&b=2.md", "p__a=1&b=2.md"] diff --git a/tests/parser/remote/test_crawler_loader.py b/tests/parser/remote/test_crawler_loader.py index 7af6c54c..7053cc04 100644 --- a/tests/parser/remote/test_crawler_loader.py +++ b/tests/parser/remote/test_crawler_loader.py @@ -208,3 +208,43 @@ def test_colliding_pages_get_distinct_file_paths(mock_pinned_request, mock_valid "http://example.com/a.html": "a-2.md", "http://example.com/l?page=2": "l__page=2.md", } + + +@patch("docsgpt.parser.remote.crawler_loader.validate_url", side_effect=_mock_validate_url) +@patch("docsgpt.parser.remote.crawler_loader.pinned_request") +def test_reordered_query_and_fragment_variants_are_fetched_once(mock_pinned_request, mock_validate_url): + responses = { + "http://example.com": DummyResponse( + "PP2" + "P3" + ), + "http://example.com/p?a=1&b=2": DummyResponse("p"), + "http://example.com/p?b=2&a=1": DummyResponse("p"), + "http://example.com/p?a=1&b=2#top": DummyResponse("p"), + } + mock_pinned_request.side_effect = lambda _m, url, timeout=30: responses[url] + + result = CrawlerLoader(limit=10).load_data("http://example.com") + + fetched = [c.args[1] for c in mock_pinned_request.call_args_list] + assert fetched[0] == "http://example.com" + assert len(fetched) == 2 + # The page is fetched as a link wrote it, not as its normalized key. + assert fetched[1] in responses + assert len(result) == 2 + + +@patch("docsgpt.parser.remote.crawler_loader.validate_url", side_effect=_mock_validate_url) +@patch("docsgpt.parser.remote.crawler_loader.pinned_request") +def test_malformed_link_does_not_abort_the_crawl(mock_pinned_request, mock_validate_url): + responses = { + "http://example.com": DummyResponse( + "BadGood" + ), + "http://example.com/good": DummyResponse("good"), + } + mock_pinned_request.side_effect = lambda _m, url, timeout=30: responses[url] + + result = CrawlerLoader(limit=10).load_data("http://example.com") + + assert {d.extra_info["source"] for d in result} == {"http://example.com", "http://example.com/good"} diff --git a/tests/parser/remote/test_crawler_markdown.py b/tests/parser/remote/test_crawler_markdown.py index f8e54687..614e5d44 100644 --- a/tests/parser/remote/test_crawler_markdown.py +++ b/tests/parser/remote/test_crawler_markdown.py @@ -204,3 +204,49 @@ def test_colliding_pages_get_distinct_file_paths(monkeypatch, _patch_markdownify "http://example.com/a.html": "a-2.md", "http://example.com/l?page=2": "l__page=2.md", } + + +def test_reordered_query_and_fragment_variants_are_fetched_once(monkeypatch, _patch_markdownify): + root_html = ( + "Home" + "PP2P3" + "" + ) + page_html = "p" + responses = { + "http://example.com": DummyResponse(root_html), + "http://example.com/p?a=1&b=2": DummyResponse(page_html), + "http://example.com/p?b=2&a=1": DummyResponse(page_html), + "http://example.com/p?a=1&b=2#top": DummyResponse(page_html), + } + fetched = [] + + def side_effect(url): + fetched.append(url) + return responses[url] + + _patch_pinned_request(monkeypatch, side_effect) + + docs = CrawlerLoader(limit=10).load_data("http://example.com") + + assert len(fetched) == 2 + assert fetched[0] == "http://example.com" + # The page is fetched as a link wrote it, not as its normalized key. + assert fetched[1] in responses + assert len(docs) == 2 + + +def test_malformed_link_does_not_abort_the_crawl(monkeypatch, _patch_markdownify): + root_html = ( + "Home" + "BadGood" + ) + responses = { + "http://example.com": DummyResponse(root_html), + "http://example.com/good": DummyResponse("good"), + } + _patch_pinned_request(monkeypatch, lambda url: responses[url]) + + docs = CrawlerLoader(limit=10).load_data("http://example.com") + + assert {d.extra_info["source"] for d in docs} == {"http://example.com", "http://example.com/good"} diff --git a/tests/parser/remote/test_sitemap_loader.py b/tests/parser/remote/test_sitemap_loader.py index 88691233..c6f844e4 100644 --- a/tests/parser/remote/test_sitemap_loader.py +++ b/tests/parser/remote/test_sitemap_loader.py @@ -351,3 +351,27 @@ class TestSitemapLoaderDistinctPaths: docs = loader.load_data("https://x.io/sitemap.xml") assert [d.extra_info["file_path"] for d in docs] == ["p__page=2.md", "p.md", "p-2.md"] + + +@pytest.mark.unit +class TestSitemapLoaderMalformedEntry: + + @patch("docsgpt.core.url_validation.resolve_hostname", return_value="93.184.216.34") + @patch("docsgpt.parser.remote.sitemap_loader.pinned_request") + def test_malformed_entry_is_skipped_and_the_rest_ingest(self, mock_pinned_request, _mock_resolve): + loader = SitemapLoader() + response = MagicMock() + response.text = "Good body" + response.raise_for_status.return_value = None + mock_pinned_request.return_value = response + + with patch.object( + loader, "_extract_urls", + return_value=["http://[bad", "https://example.com/good"], + ): + docs = loader.load_data("https://example.com/sitemap.xml") + + assert [d.extra_info for d in docs] == [ + {"source": "https://example.com/good", "file_path": "good.md"} + ] + mock_pinned_request.assert_called_once() diff --git a/tests/parser/remote/test_web_loader.py b/tests/parser/remote/test_web_loader.py index 97c885c0..9284071f 100644 --- a/tests/parser/remote/test_web_loader.py +++ b/tests/parser/remote/test_web_loader.py @@ -351,3 +351,16 @@ class TestWebLoaderDistinctPaths: assert [d.extra_info["file_path"] for d in result] == [ "list__page=1.md", "list__page=2.md", "a-2.md", "a.md" ] + + +@pytest.mark.unit +class TestWebLoaderMalformedUrl: + @patch("docsgpt.core.url_validation.resolve_hostname", return_value="93.184.216.34") + @patch("docsgpt.parser.remote.web_loader.pinned_request") + def test_malformed_url_is_skipped_and_the_rest_ingest(self, mock_pinned_request, _mock_resolve, web_loader): + mock_pinned_request.side_effect = lambda *a, **k: _fake_response("good") + + result = web_loader.load_data(["http://[bad", "https://x.io/good"]) + + assert [d.extra_info["source"] for d in result] == ["https://x.io/good"] + assert result[0].extra_info["file_path"] == "good.md" diff --git a/tests/storage/db/repositories/test_schedule_runs.py b/tests/storage/db/repositories/test_schedule_runs.py index 3cb58966..9b4cd54c 100644 --- a/tests/storage/db/repositories/test_schedule_runs.py +++ b/tests/storage/db/repositories/test_schedule_runs.py @@ -283,3 +283,24 @@ class TestStatsForAgent: assert narrow["runs"] == 1 assert narrow["failed"] == 0 assert narrow["latest_failure"] is None + + def test_future_runs_are_not_counted(self, pg_conn): + schedule_id, agent_id = _make_schedule(pg_conn) + _add_run( + pg_conn, schedule_id, "u1", agent_id, ago=timedelta(hours=1), + status="success", prompt_tokens=3, generated_tokens=4, + ) + # Scheduled a day ahead: inside ``>= now() - days`` but not yet due. + _add_run( + pg_conn, schedule_id, "u1", agent_id, ago=-timedelta(days=1), + status="failed", prompt_tokens=500, generated_tokens=500, + error_type="agent_error", + ) + + stats = ScheduleRunsRepository(pg_conn).stats_for_agent( + agent_id, "u1", days=30, + ) + assert stats["runs"] == 1 + assert stats["failed"] == 0 + assert stats["tokens"] == 7 + assert stats["latest_failure"] is None diff --git a/tests/vectorstore/test_base.py b/tests/vectorstore/test_base.py index 37bf9cf8..92f0c047 100644 --- a/tests/vectorstore/test_base.py +++ b/tests/vectorstore/test_base.py @@ -665,10 +665,11 @@ class TestGetEmbeddingsResolver: class _RecordingStore(ConcreteVectorStore): """Store whose add/delete calls are recorded, for the update fallback.""" - def __init__(self, delete_result=True): + def __init__(self, delete_result=True, delete_error=None): super().__init__() self.calls = [] self._delete_result = delete_result + self._delete_error = delete_error def add_chunk(self, text, metadata=None, *args, **kwargs): self.calls.append(("add", text, metadata)) @@ -676,6 +677,10 @@ class _RecordingStore(ConcreteVectorStore): def delete_chunk(self, chunk_id, *args, **kwargs): self.calls.append(("delete", chunk_id)) + if chunk_id != "old-id": + return True + if self._delete_error is not None: + raise self._delete_error return self._delete_result @@ -689,14 +694,44 @@ class TestBaseUpdateChunkFallback: assert new_id == "new-id" assert store.calls == [("add", "new text", {"k": "v"}), ("delete", "old-id")] - def test_failed_delete_is_logged_not_raised(self, caplog): + def test_false_delete_rolls_back_the_new_chunk_and_raises(self): store = _RecordingStore(delete_result=False) - with caplog.at_level("WARNING"): - new_id = store.update_chunk("old-id", "new text", {}) + with pytest.raises(RuntimeError, match="old-id"): + store.update_chunk("old-id", "new text", {}) - assert new_id == "new-id" - assert "old-id" in caplog.text + assert store.calls == [ + ("add", "new text", {}), + ("delete", "old-id"), + ("delete", "new-id"), + ] + + def test_raising_delete_rolls_back_the_new_chunk_and_raises(self): + store = _RecordingStore(delete_error=ConnectionError("milvus down")) + + with pytest.raises(RuntimeError, match="old-id") as excinfo: + store.update_chunk("old-id", "new text", {}) + + assert isinstance(excinfo.value.__cause__, ConnectionError) + assert store.calls[-1] == ("delete", "new-id") + + def test_failed_rollback_still_raises_the_update_error(self, caplog): + store = _RecordingStore(delete_result=False) + real_delete = store.delete_chunk + + def delete(chunk_id, *args, **kwargs): + if chunk_id == "new-id": + store.calls.append(("delete", chunk_id)) + raise ConnectionError("rollback failed") + return real_delete(chunk_id) + + store.delete_chunk = delete + + with caplog.at_level("ERROR"), pytest.raises(RuntimeError, match="old-id"): + store.update_chunk("old-id", "new text", {}) + + assert ("delete", "new-id") in store.calls + assert "new-id" in caplog.text def test_failed_add_skips_the_delete(self): store = _RecordingStore()