feat(chat): join-table attachments, PersistedFilesInfo, docker HF cache#326
Open
abdulrafey1 wants to merge 20 commits into
Open
feat(chat): join-table attachments, PersistedFilesInfo, docker HF cache#326abdulrafey1 wants to merge 20 commits into
abdulrafey1 wants to merge 20 commits into
Conversation
…nagement
Introduces an LLM-based intent router that inspects the user's message and
attached-file metadata to decide whether RAG retrieval should run for the
current turn. Previously RAG fired only when drive_file blocks were inlined
in the request; now it fires based on persisted per-conversation attachments
and a routing decision, reducing unnecessary MCP roundtrips on chit-chat.
- New chat_conversation_attachments join table (Alembic migration with
back-fill from active_drive_file_id/s columns)
- POST/DELETE /conversations/{uuid}/attachments endpoints
- RAGIntentRouter: fail-closed on LangChainException/ValidationError → 502
- SSE status events: scanning_attachments / skipping_rag
- 28 new tests (unit + attachment + integration), all passing
- Use dialect-aware SQL in back-fill migration (PostgreSQL vs SQLite) - Replace bare except with specific SQLAlchemyError + warning log - Fix deprecated datetime.utcnow → datetime.now(timezone.utc) - Chain HTTPException from RAGIntentRouterError to preserve traceback Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Accessing _create_llm() from outside the class broke encapsulation. Add a stable public delegate so callers in routes.py are insulated from internal provider implementation changes.
…ate_llm - Use asyncio.gather in RAGIntentRouter.decide() so document structure calls for multiple attached files run concurrently instead of serially - Add BaseChatProvider.create_llm() public delegate so callers are insulated from the private _create_llm() implementation detail Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
A single FK violation (drive_file_id referencing a deleted file) aborted the whole PostgreSQL transaction, causing all subsequent inserts and the Alembic version update to fail. Wrapping each per-file insert in a nested savepoint means only that row is rolled back on error. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Matches the model's datetime.now(timezone.utc) default and avoids silent timezone truncation on PostgreSQL (TIMESTAMPTZ vs TIMESTAMP). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The detach endpoint only checked conversation ownership, not file ownership. A caller could detach a file they don't own if it was somehow attached to their conversation. Added the same ownership check as the attach endpoint. Also fix f-string logger in service.py -> % formatting. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Router-driven RAG built synthetic messages with only drive_file blocks, causing three failures: empty similarity-search query, user question dropped from the non-streaming LLM prompt, and RAG context silently discarded in the streaming assembly pass. Fix by appending the query text block to synthetic messages and building the streaming current from unresolved_messages so drive_file blocks are visible to the in-stream replacement pass.
… to join table SQLModel inferred sa.DateTime() (no timezone) for attached_at despite the default producing timezone-aware datetimes — causing asyncpg to reject the insert. Added explicit sa_column=Column(DateTime(timezone=True)) to match the DB column type. Also sync inline drive_file blocks from existing conversations into the chat_conversation_attachments join table so the intent router can find them. Frontend still sends drive_file blocks inline; without this sync the router saw no attachments and skipped RAG on every turn. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Routes now call provider.create_llm() (public method). Mock setup for _create_llm had no effect — replaced with create_llm.return_value. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Mounts a named Docker volume at /home/nonroot/.cache/huggingface so HuggingFace model weights survive container restarts without re-downloading. Creates the directory with correct nonroot ownership in Dockerfile.local. Sets HF_HUB_DISABLE_XET=1 on the api, rag-cleanup, and rag-mcp services. The XET chunked download protocol hangs indefinitely on unauthenticated requests; the standard HTTP path is reliable and unblocks embedding model initialisation. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ent api
Drops the denormalised active_drive_file_id and active_drive_file_ids
columns from the Conversation model and removes all code that read or
wrote them. Attachment state is now authoritative in the
ConversationAttachment join table only.
New GET /conversations/{id}/attachments endpoint returns READY drive
files attached to a conversation, replacing the old active-file fields
baked into the conversation response. Replaces the deleted
DELETE /conversations/{id}/active-file endpoint.
Adds drive_file_ids to ChatCompletionRequest so the completion handler
can attach files selected before a conversation exists in the DB
(new-conversation flow). Removes the brittle inline drive_file
content-block scanning that extracted file IDs from message bodies.
The scope classifier now receives attached file names so it correctly
classifies document-related queries as in-scope. Section SSE events
are emitted before the LLM stream so the frontend renders retrieved
sections above the response bubble.
Migration 87e049122b83 drops the two columns.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…eFileIds in send payload
Replaces the old Pill component (which displayed attached files as
removable pills) with PersistedFilesInfo, which shows a read-only
list of files that are already persisted to the conversation in the
backend. This reflects that drive file attachments are now managed
via the join-table API rather than inline content blocks.
Drive file selections now fire an immediate POST to
/conversations/{id}/attachments and removals fire an immediate DELETE,
so the backend is always in sync with what the user sees. For new
conversations (no id yet) the attach is deferred and sent in
onConversationCreated.
Removes the drive_file content block type from buildUserMessages in
useChatStream — file IDs are now passed explicitly as driveFileIds in
the send payload and forwarded to the backend as drive_file_ids on the
completion request, eliminating the need for the backend to parse file
references out of message content.
useConversation now loads persisted attachments from the new
GET /conversations/{id}/attachments endpoint on conversation load,
restoring the correct attachment list including file size.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…s polling Changes the Queued status chip from warning (amber) to muted (grey) to avoid the impression that something is wrong when files are waiting to be indexed. Makes the Failed tooltip always render even when ragError is null, showing a generic fallback message so users know why a file has failed indexing and what to do about it. Previously the Tooltip was only rendered when ragError was non-null, leaving a bare unclickable chip. Adds a polling loop that refreshes RAG statuses every 5 seconds for any folder that has files in queued or processing state. Uses a single getFolderRagStatus call per active folder and dispatches UPDATE_RAG_STATUSES to patch the relevant resource rows in place. Polling stops automatically once all files reach a terminal state (ready or failed). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Claude finished @abdulrafey1's task in 5m 7s —— View job PR Review: feat(chat): join-table attachments, PersistedFilesInfo, docker HF cache
OverviewThis PR replaces the denormalised Previous Review CommentsNo previous review comments to resolve. Critical / BlockingNone. High Priority
Medium Priority
Low Priority / Nits
What's Good
SummarySolid PR overall. The one actionable bug is #1 (deselected files not detached from backend) — this causes a real data inconsistency where RAG uses files the user thought they removed. #2 is a hardening suggestion. The rest are pre-existing issues or minor nits. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
- Revert UI state and surface uploadError when drive file attach/detach API calls fail, replacing silent fire-and-forget console.warn - Issue DELETE calls for files deselected in DriveFilePicker - Surface new-conversation sync failures via the Alert banner - Add max_length=20 guard on drive_file_ids request field - Fix f-string logger and missing `from e` in provider error handler - Remove redundant pass from RAGIntentRouterError Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Claude finished @abdulrafey1's task in 5m 38s —— View job PR Review: feat(chat): join-table attachments, PersistedFilesInfo, docker HF cache
Previous Review CommentsBoth comments from the previous review have been resolved in commit
Additional nits from the first review also addressed in
OverviewThis PR replaces the denormalised High Priority
Medium Priority
Low Priority / Nits
What's Good
SummaryThe previous review's two items are fully resolved. The most impactful new finding is #1 — all |
10 steps was too low — a normal agentic RAG query (get_document_structure + search_section_by_keyword + final-answer tool) already costs 6+ steps. Legitimate queries on large documents were hitting GraphRecursionError. 25 matches LangGraph's own default and still terminates true infinite loops.
|
Claude finished @abdulrafey1's task in 2m 56s —— View job PR Review: feat(chat): join-table attachments, PersistedFilesInfo, docker HF cache
Previous Review CommentsReview 1 (2 items): Both resolved in
Review 2 (8 items): 3 unresolved threads remain open. The latest commit (
OverviewThis PR replaces denormalised High Priority
Medium Priority
Low Priority / Nits
What's Good
SummaryThe two items from the first review are resolved. The 8 items from the second review remain unaddressed. The most impactful are #1 (fetch doesn't reject on HTTP errors — all error recovery for attach/detach is effectively dead code) and #3 (clearing text attachments silently wipes drive files from UI without backend sync). These should be fixed before merge to avoid data inconsistency between UI and backend state. |
- Batch drive_file_ids ownership check into single IN query - Fix stale closure and add res.ok guards on all fire-and-forget fetches - Issue DELETEs for drive files when clearing all attachments - Drop unused session param from RAGIntentRouter.decide() - Drop redundant created_at/updated_at from ConversationAttachment - Add exponential backoff to RAG status poller - Remove dead _strip_md, fix typo, tighten test assertions Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Claude finished @abdulrafey1's task in 2m 47s —— View job PR Review: feat(chat): join-table attachments, PersistedFilesInfo, docker HF cache
Previous Review CommentsReview 1 (2 items): Both resolved in
Review 2 (8 items): All resolved in
Review 3 (8 items): All were duplicates of Review 2 — all resolved. All 10 previously open review threads have been resolved. OverviewThis PR replaces denormalised Critical / BlockingNone. Medium Priority
Low Priority / Nits
What's Good
SummaryAll 10 previously open review threads from Reviews 1–3 are now resolved. The latest commit ( |
… file ids Use functional updater in handleDriveFileSelected to avoid stale closure over attachments during concurrent state updates. Add lastContent to the auto-scroll useEffect dependency array so the view scrolls to the bottom on every streaming token, not only when a new message is appended. Log a warning in the batched ownership check when drive file IDs are skipped due to missing ownership or soft-deletion, improving debuggability. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Claude finished @abdulrafey1's task in 3m 4s —— View job PR Review: feat(chat): join-table attachments, PersistedFilesInfo, docker HF cache
Previous Review CommentsAll 13 review threads from Reviews 1–4 are now resolved.
OverviewThis PR replaces the denormalised Critical / BlockingNone. High PriorityNone. Low Priority / Nits
What's Good
SummaryAll 13 previously open review threads from Reviews 1–4 are resolved. The latest commits ( |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Completes the drive-file attachment model migration by removing the
denormalised
active_drive_file_id / active_drive_file_idscolumns,replacing them with the authoritative
ConversationAttachmentjointable. Adds frontend components and hooks to make attach/detach
immediate, restores persisted attachments on conversation load, and
fixes several UX issues in the Resources tab. Also fixes a Docker
startup hang caused by the HuggingFace XET download protocol.
Changes
chore(docker): add namedhf_cachevolume and setHF_HUB_DISABLE_XET=1on api, rag-cleanup, and rag-mcp servicesfeat(chat): dropactive_drive_file_id / active_drive_file_idscolumns; newGET /conversations/{id}/attachmentsendpoint;drive_file_idsfield onChatCompletionRequest; scope classifier receives file names; section SSE emitted before LLM stream; migration87e049122b83feat(frontend):PersistedFilesInfocomponent (read-only list of persisted files); immediatePOST/DELETE /attachmentson file select/deselect; removedrive_fileinline content blocks frombuildUserMessages; load persisted attachments from new endpoint on conversation loadfix(frontend): Queued chip colour changed from amber to grey; Failed tooltip always rendered with fallback message; RAG status polling every 5 s for folders with in-progress files, stops on terminal stateHow to Test
make clean && make up— confirm the api container starts without hanging on embedding model download; confirm thehf_cachevolume is created.POST /conversations/{id}/attachmentsfires immediately.DELETE /conversations/{id}/attachments/{file_id}fires immediately.drive_file_idsand the response uses retrieved sections.GET /conversations/{id}/attachments.ragErrormessage.make test— all tests pass includingtests/chat/test_conversation_attachments.pyandtests/chat/test_persistent_attachment.py.Notes
Migration required:
87e049122b83_remove_active_drive_file_columns_from_.pydropsactive_drive_file_idandactive_drive_file_idsfrom theconversationtable. Runmake migrationsafter deploying.Breaking change:
GET /conversations/{id}no longer returnsactive_drive_file_idoractive_drive_file_idsfields. Any client reading those fields must switch toGET /conversations/{id}/attachments. The oldDELETE /conversations/{id}/active-fileendpoint is removed; useDELETE /conversations/{id}/attachments/{file_id}.Docker: The
hf_cachenamed volume is new — existingdocker-compose upwill create it automatically. No manual steps required.