switch chat agent to stream output#13
Merged
Merged
Conversation
- Switch from generateText to streamText with saveStreamDeltas for live updates - Add getThreadMessagesStreaming query using useUIMessages hook - Display dynamic tool invocation status in ThinkingAnimation component - Show contextual messages like "Searching X" or "Reading example.com" - Support multiple concurrent tool calls with smart text grouping - Improve web_read search tool guidance - Add next_action_required field to search results - Update agent prompt to emphasize fetch_url after search - Add fallback URLs for common queries (weather via wttr.in)
- Add scale parameter for device scale factor (default 2.0 for Retina) - Increase default quality from 90 to 100 for JPEG images - Increase default viewport width from 800 to 1200px - Update image tool description to warn against setting explicit dimensions - Propagate scale option through crawler service, API endpoints, and Convex types
Contributor
📝 WalkthroughWalkthroughThis PR implements multi-system image scaling enhancements and streaming-aware chat interface improvements. The image-related changes add a Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes
Possibly related PRs
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/platform/convex/agent_tools/convex_tools/crawler/helpers/search_web.ts (1)
72-90: EscapefirstUrlinnextActionRequired(and guardnumResults/pageNumber) to avoid malformed instructions/pagination bugs.
firstUrlis interpolated into a JSON-like snippet; if it contains"or control characters, the instruction becomes malformed. Also,numResults <= 0makeshasMore/next_start_indexincorrect.- const numResults = args.num_results ?? 10; - const pageNumber = args.page_number ?? 1; + const numResults = Math.min(50, Math.max(1, args.num_results ?? 10)); + const pageNumber = Math.max(1, args.page_number ?? 1); @@ - const nextActionRequired = hasResults - ? `IMPORTANT: These are only search result snippets, NOT the actual page content. To get real data (weather, prices, news, etc.), you MUST now call web_read with { operation: "fetch_url", url: "${firstUrl}" } or another relevant URL from the results above.` + const nextActionRequired = hasResults + ? `IMPORTANT: These are only search result snippets, NOT the actual page content. To get real data (weather, prices, news, etc.), you MUST now call web_read with { operation: "fetch_url", url: ${JSON.stringify(firstUrl)} } or another relevant URL from the results above.` : 'No results found. Try a different search query.';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
services/platform/convex/_generated/api.d.tsis excluded by!**/_generated/**
📒 Files selected for processing (17)
services/crawler/app/converter_service.py(4 hunks)services/crawler/app/main.py(3 hunks)services/crawler/app/models.py(1 hunks)services/platform/app/(app)/dashboard/[id]/chat/components/chat-interface.tsx(8 hunks)services/platform/convex/agent_tools/convex_tools/crawler/helpers/search_web.ts(2 hunks)services/platform/convex/agent_tools/convex_tools/crawler/helpers/types.ts(1 hunks)services/platform/convex/agent_tools/convex_tools/crawler/web_read_tool.ts(1 hunks)services/platform/convex/agent_tools/convex_tools/files/image_tool.ts(2 hunks)services/platform/convex/documents.ts(1 hunks)services/platform/convex/lib/create_chat_agent.ts(1 hunks)services/platform/convex/model/chat_agent/generate_agent_response.ts(1 hunks)services/platform/convex/model/documents/generate_document_helpers.ts(1 hunks)services/platform/convex/model/documents/types.ts(1 hunks)services/platform/convex/model/threads/get_latest_tool_message.ts(1 hunks)services/platform/convex/model/threads/get_thread_messages_streaming.ts(1 hunks)services/platform/convex/model/threads/index.ts(1 hunks)services/platform/convex/threads.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*
📄 CodeRabbit inference engine (.cursor/rules/workspace_rules.mdc)
Use English only for ALL user-facing content including UI components, labels, buttons, dialogs, forms, toast messages, error messages, success messages, comments, documentation, README files, variable names, function names, and type names
Files:
services/platform/convex/agent_tools/convex_tools/crawler/helpers/types.tsservices/platform/convex/model/documents/generate_document_helpers.tsservices/platform/convex/model/threads/index.tsservices/platform/convex/model/documents/types.tsservices/platform/convex/threads.tsservices/platform/convex/model/threads/get_thread_messages_streaming.tsservices/platform/convex/model/threads/get_latest_tool_message.tsservices/platform/convex/model/chat_agent/generate_agent_response.tsservices/crawler/app/models.pyservices/platform/app/(app)/dashboard/[id]/chat/components/chat-interface.tsxservices/platform/convex/agent_tools/convex_tools/crawler/helpers/search_web.tsservices/platform/convex/agent_tools/convex_tools/files/image_tool.tsservices/platform/convex/lib/create_chat_agent.tsservices/platform/convex/documents.tsservices/crawler/app/converter_service.pyservices/platform/convex/agent_tools/convex_tools/crawler/web_read_tool.tsservices/crawler/app/main.py
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/workspace_rules.mdc)
**/*.{ts,tsx,js,jsx}: Use Vercel AI SDK with OpenAI - import from 'ai' and '@ai-sdk/openai', never use raw OpenAI SDK or OpenRouter
Never hallucinate API keys - always use environment variables and existing .env configuration
Use camelCase for function names (e.g.,getUserData)
Use SCREAMING_SNAKE_CASE for constants (e.g.,API_BASE_URL,MAX_RETRIES)
Use feature flags with enums (TypeScript) or const objects (JavaScript) with UPPERCASE_WITH_UNDERSCORE naming
Implement error handling with try-catch pattern: check for result.error and display descriptive toast messages using result.error as title
Files:
services/platform/convex/agent_tools/convex_tools/crawler/helpers/types.tsservices/platform/convex/model/documents/generate_document_helpers.tsservices/platform/convex/model/threads/index.tsservices/platform/convex/model/documents/types.tsservices/platform/convex/threads.tsservices/platform/convex/model/threads/get_thread_messages_streaming.tsservices/platform/convex/model/threads/get_latest_tool_message.tsservices/platform/convex/model/chat_agent/generate_agent_response.tsservices/platform/app/(app)/dashboard/[id]/chat/components/chat-interface.tsxservices/platform/convex/agent_tools/convex_tools/crawler/helpers/search_web.tsservices/platform/convex/agent_tools/convex_tools/files/image_tool.tsservices/platform/convex/lib/create_chat_agent.tsservices/platform/convex/documents.tsservices/platform/convex/agent_tools/convex_tools/crawler/web_read_tool.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/workspace_rules.mdc)
**/*.{ts,tsx}: Use kebab-case for file names (e.g.,user-profile.tsx)
Use PascalCase for component names (e.g.,UserProfile)
Use descriptive messages as toast title (never generic 'Error'), with optional description for additional context only
Follow component structure: 'use client' directive, imports, interface Props, hooks, effects, event handlers, then render
Prioritize data fetching methods in order: Server Actions (preferred), Route Handlers (when needed), Client-side (minimal use)
Use React.memo for expensive components to optimize performance
Use Next.js Image component for all images instead of native img tags
Use dynamic imports for code splitting
Files:
services/platform/convex/agent_tools/convex_tools/crawler/helpers/types.tsservices/platform/convex/model/documents/generate_document_helpers.tsservices/platform/convex/model/threads/index.tsservices/platform/convex/model/documents/types.tsservices/platform/convex/threads.tsservices/platform/convex/model/threads/get_thread_messages_streaming.tsservices/platform/convex/model/threads/get_latest_tool_message.tsservices/platform/convex/model/chat_agent/generate_agent_response.tsservices/platform/app/(app)/dashboard/[id]/chat/components/chat-interface.tsxservices/platform/convex/agent_tools/convex_tools/crawler/helpers/search_web.tsservices/platform/convex/agent_tools/convex_tools/files/image_tool.tsservices/platform/convex/lib/create_chat_agent.tsservices/platform/convex/documents.tsservices/platform/convex/agent_tools/convex_tools/crawler/web_read_tool.ts
services/*/convex/*.ts
📄 CodeRabbit inference engine (.cursor/rules/workspace_rules.mdc)
Thin wrapper API modules (like
services/platform/convex/documents.ts) may export multiple Convex functions as wrappers that delegate to model helpers, but must not contain business logic and must only perform argument/return validation and delegation
Files:
services/platform/convex/threads.tsservices/platform/convex/documents.ts
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (.cursor/rules/figma_rules.mdc)
**/*.{tsx,jsx}: Avoid specifyingfont-['Inter:Regular',_sans-serif]as it should be the default
Only specify font-family when using non-default fonts likefont-['Inter:Medium',_sans-serif]
Ensure font-family matches font-weight (Inter:Regular with font-normal, Inter:Medium with font-medium)
Useleading-normalinstead ofleading-[normal]in Tailwind classes
Use standard font size classes instead of arbitrary values:text-[12px]→text-xs,text-[14px]→text-sm,text-[16px]→text-base,text-[18px]→text-lg,text-[20px]→text-xl,text-[24px]→text-2xl
Use semantic spacing classes:p-[4px]→p-1,p-[8px]→p-2,m-[4px]→m-1,m-[8px]→m-2
Convert pixel values to rem using the 16px base for width and height measurements:w-[278px]→w-[17.375rem],h-[48px]→h-[3rem],min-w-[120px]→min-w-[7.5rem],max-w-[400px]→max-w-[25rem]
NEVER use hardcoded colors liketext-gray-500,bg-gray-100,border-gray-200; ALWAYS use design system semantic colors:text-foregroundfor primary text,text-muted-foregroundfor secondary text and icons,bg-backgroundfor main backgrounds,bg-mutedfor subtle backgrounds and hover states,border-borderfor borders
ALWAYS use the Table component instead of custom flex layouts; useTable,TableHeader,TableBody,TableRow,TableHead,TableCellcomponents with proper column widths using rem units and semantic colors
Files:
services/platform/app/(app)/dashboard/[id]/chat/components/chat-interface.tsx
**/app/**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/workspace_rules.mdc)
In Next.js App Router, use
page.tsxas server components by default; use'use client'only for interactions and state
Files:
services/platform/app/(app)/dashboard/[id]/chat/components/chat-interface.tsx
🧠 Learnings (21)
📚 Learning: 2025-12-02T08:13:51.424Z
Learnt from: CR
Repo: tale-project/tale PR: 0
File: .cursor/rules/workspace_rules.mdc:0-0
Timestamp: 2025-12-02T08:13:51.424Z
Learning: Applies to services/*/convex/*.ts : Thin wrapper API modules (like `services/platform/convex/documents.ts`) may export multiple Convex functions as wrappers that delegate to model helpers, but must not contain business logic and must only perform argument/return validation and delegation
Applied to files:
services/platform/convex/model/threads/index.tsservices/platform/convex/threads.tsservices/platform/convex/model/threads/get_thread_messages_streaming.tsservices/platform/convex/model/threads/get_latest_tool_message.ts
📚 Learning: 2025-11-30T12:29:39.745Z
Learnt from: CR
Repo: tale-project/poc2 PR: 0
File: .cursor/rules/workspace_rules.mdc:0-0
Timestamp: 2025-11-30T12:29:39.745Z
Learning: Applies to services/**/convex/*.ts : Thin wrapper API modules in services may export multiple Convex functions as thin wrappers that delegate to model helpers, must use snake_case file names and camelCase export names, and must not contain business logic
Applied to files:
services/platform/convex/model/threads/index.tsservices/platform/convex/threads.tsservices/platform/convex/model/threads/get_latest_tool_message.ts
📚 Learning: 2025-12-02T08:13:24.290Z
Learnt from: CR
Repo: tale-project/tale PR: 0
File: .cursor/rules/convex_rules.mdc:0-0
Timestamp: 2025-12-02T08:13:24.290Z
Learning: Applies to convex/**/*.{ts,tsx} : When touching existing modules that export multiple functions, split them into separate files so each file exports exactly one function matching the file name, unless the module is an explicitly designated thin wrapper file.
Applied to files:
services/platform/convex/model/threads/index.ts
📚 Learning: 2025-11-30T03:53:00.316Z
Learnt from: CR
Repo: tale-project/poc2 PR: 0
File: .cursor/rules/convex_rules.mdc:0-0
Timestamp: 2025-11-30T03:53:00.316Z
Learning: Applies to convex/**/!(model)/*.ts : Exception: Thin wrapper API modules (e.g., `services/platform/convex/documents.ts`) may export multiple Convex functions if they only validate args/returns and delegate to the model layer; must use snake_case for file names and camelCase for function names
Applied to files:
services/platform/convex/model/threads/index.tsservices/platform/convex/model/threads/get_latest_tool_message.ts
📚 Learning: 2025-12-02T08:13:24.290Z
Learnt from: CR
Repo: tale-project/tale PR: 0
File: .cursor/rules/convex_rules.mdc:0-0
Timestamp: 2025-12-02T08:13:24.290Z
Learning: Applies to convex/**/convex/*.{ts,tsx} : Thin wrapper API modules (e.g., `services/platform/convex/documents.ts`) may export multiple Convex functions as thin wrappers around model-layer helpers. These files must not contain business logic, should only validate args/returns and delegate to the model layer, must use snake_case for file names and camelCase for each exported function.
Applied to files:
services/platform/convex/model/threads/index.ts
📚 Learning: 2025-11-30T12:29:39.745Z
Learnt from: CR
Repo: tale-project/poc2 PR: 0
File: .cursor/rules/workspace_rules.mdc:0-0
Timestamp: 2025-11-30T12:29:39.745Z
Learning: Applies to convex/*.ts : Split existing Convex modules that export multiple functions into separate files with one function per file, except for explicitly designated thin wrapper modules
Applied to files:
services/platform/convex/model/threads/index.ts
📚 Learning: 2025-12-02T08:13:51.424Z
Learnt from: CR
Repo: tale-project/tale PR: 0
File: .cursor/rules/workspace_rules.mdc:0-0
Timestamp: 2025-12-02T08:13:51.424Z
Learning: Applies to convex/**/*.ts : Split existing Convex modules that export multiple functions into separate files, each exporting exactly one function, except for thin wrapper modules
Applied to files:
services/platform/convex/model/threads/index.ts
📚 Learning: 2025-12-02T08:13:24.290Z
Learnt from: CR
Repo: tale-project/tale PR: 0
File: .cursor/rules/convex_rules.mdc:0-0
Timestamp: 2025-12-02T08:13:24.290Z
Learning: Applies to convex/**/*.{ts,tsx} : Use the `paginationOptsValidator` from `convex/server` for paginated queries with `numItems` and `cursor` parameters. Queries ending in `.paginate()` return objects with `page`, `isDone`, and `continueCursor` properties.
Applied to files:
services/platform/convex/threads.ts
📚 Learning: 2025-10-03T11:34:20.628Z
Learnt from: CR
Repo: talecorp/poc2 PR: 0
File: .cursor/rules/convex_rules.mdc:0-0
Timestamp: 2025-10-03T11:34:20.628Z
Learning: Applies to convex/**/*.{ts,js} : Define paginated queries using paginationOptsValidator and .paginate(opts)
Applied to files:
services/platform/convex/threads.ts
📚 Learning: 2025-11-30T03:53:00.316Z
Learnt from: CR
Repo: tale-project/poc2 PR: 0
File: .cursor/rules/convex_rules.mdc:0-0
Timestamp: 2025-11-30T03:53:00.316Z
Learning: Applies to convex/**/*.ts : Define pagination using `paginationOptsValidator` with `numItems` and `cursor` properties, and use `.paginate()` on queries which returns objects with `page`, `isDone`, and `continueCursor` properties
Applied to files:
services/platform/convex/threads.ts
📚 Learning: 2025-11-30T03:53:00.316Z
Learnt from: CR
Repo: tale-project/poc2 PR: 0
File: .cursor/rules/convex_rules.mdc:0-0
Timestamp: 2025-11-30T03:53:00.316Z
Learning: Applies to convex/**/*.ts : Use the `api` object from `convex/_generated/api.ts` to call public functions registered with `query`, `mutation`, or `action`
Applied to files:
services/platform/convex/threads.tsservices/platform/app/(app)/dashboard/[id]/chat/components/chat-interface.tsx
📚 Learning: 2025-11-30T03:53:00.316Z
Learnt from: CR
Repo: tale-project/poc2 PR: 0
File: .cursor/rules/convex_rules.mdc:0-0
Timestamp: 2025-11-30T03:53:00.316Z
Learning: Applies to convex/**/*.ts : Try to use as few calls from actions to queries and mutations as possible to avoid race conditions, since queries and mutations are transactions
Applied to files:
services/platform/convex/threads.tsservices/platform/app/(app)/dashboard/[id]/chat/components/chat-interface.tsx
📚 Learning: 2025-11-30T03:53:00.316Z
Learnt from: CR
Repo: tale-project/poc2 PR: 0
File: .cursor/rules/convex_rules.mdc:0-0
Timestamp: 2025-11-30T03:53:00.316Z
Learning: Applies to convex/**/*.ts : Always include argument and return validators for all Convex functions (query, internalQuery, mutation, internalMutation, action, internalAction)
Applied to files:
services/platform/convex/threads.ts
📚 Learning: 2025-11-30T03:53:00.316Z
Learnt from: CR
Repo: tale-project/poc2 PR: 0
File: .cursor/rules/convex_rules.mdc:0-0
Timestamp: 2025-11-30T03:53:00.316Z
Learning: Applies to convex/**/*.ts : Use the `internal` object from `convex/_generated/api.ts` to call internal functions registered with `internalQuery`, `internalMutation`, or `internalAction`
Applied to files:
services/platform/convex/threads.tsservices/platform/app/(app)/dashboard/[id]/chat/components/chat-interface.tsx
📚 Learning: 2025-12-02T08:13:24.290Z
Learnt from: CR
Repo: tale-project/tale PR: 0
File: .cursor/rules/convex_rules.mdc:0-0
Timestamp: 2025-12-02T08:13:24.290Z
Learning: Applies to convex/**/*.{ts,tsx} : Try to use as few calls from actions to queries and mutations as possible, as they are transactions and splitting logic introduces race condition risks.
Applied to files:
services/platform/convex/threads.tsservices/platform/app/(app)/dashboard/[id]/chat/components/chat-interface.tsx
📚 Learning: 2025-10-03T11:34:20.628Z
Learnt from: CR
Repo: talecorp/poc2 PR: 0
File: .cursor/rules/convex_rules.mdc:0-0
Timestamp: 2025-10-03T11:34:20.628Z
Learning: Applies to convex/**/*.{ts,js} : Register internal functions with internalQuery, internalMutation, and internalAction (imported from ./_generated/server)
Applied to files:
services/platform/convex/threads.tsservices/platform/app/(app)/dashboard/[id]/chat/components/chat-interface.tsx
📚 Learning: 2025-10-03T11:34:20.628Z
Learnt from: CR
Repo: talecorp/poc2 PR: 0
File: .cursor/rules/convex_rules.mdc:0-0
Timestamp: 2025-10-03T11:34:20.628Z
Learning: Applies to convex/**/*.{ts,js} : Minimize calls from actions to queries/mutations to avoid race conditions
Applied to files:
services/platform/convex/threads.ts
📚 Learning: 2025-07-20T08:40:24.693Z
Learnt from: CR
Repo: talecorp/poc PR: 0
File: .cursor/rules/ai.mdc:0-0
Timestamp: 2025-07-20T08:40:24.693Z
Learning: Applies to **/app/api/**/*.ts : For text generation, use the `generateObject` or similar functions from the AI SDK
Applied to files:
services/platform/convex/model/chat_agent/generate_agent_response.tsservices/platform/convex/lib/create_chat_agent.ts
📚 Learning: 2025-07-20T08:40:24.693Z
Learnt from: CR
Repo: talecorp/poc PR: 0
File: .cursor/rules/ai.mdc:0-0
Timestamp: 2025-07-20T08:40:24.693Z
Learning: Applies to **/actions/*.ts : For text generation, use the `generateObject` or similar functions from the AI SDK
Applied to files:
services/platform/convex/model/chat_agent/generate_agent_response.tsservices/platform/convex/lib/create_chat_agent.ts
📚 Learning: 2025-07-20T08:40:24.693Z
Learnt from: CR
Repo: talecorp/poc PR: 0
File: .cursor/rules/ai.mdc:0-0
Timestamp: 2025-07-20T08:40:24.693Z
Learning: Applies to **/lib/**/*.ts : For text generation, use the `generateObject` or similar functions from the AI SDK
Applied to files:
services/platform/convex/model/chat_agent/generate_agent_response.tsservices/platform/convex/lib/create_chat_agent.ts
📚 Learning: 2025-08-21T14:59:56.034Z
Learnt from: CR
Repo: talecorp/lanserhof PR: 0
File: .cursor/rules/ai.mdc:0-0
Timestamp: 2025-08-21T14:59:56.034Z
Learning: Applies to {**/actions/*.ts,**/app/api/**/*.ts,**/lib/**/*.ts} : For text generation, use `generateObject` (or equivalent SDK functions) with an SDK-provided model (e.g., `openrouter(...)`)
Applied to files:
services/platform/convex/model/chat_agent/generate_agent_response.ts
🧬 Code graph analysis (3)
services/platform/convex/threads.ts (2)
services/platform/convex/model/threads/get_latest_tool_message.ts (1)
getLatestToolMessage(74-123)services/platform/convex/model/threads/get_thread_messages_streaming.ts (1)
getThreadMessagesStreaming(25-51)
services/platform/convex/model/threads/get_thread_messages_streaming.ts (2)
services/platform/convex/model/threads/index.ts (2)
StreamingMessagesResult(18-18)getThreadMessagesStreaming(13-13)services/platform/convex/threads.ts (1)
getThreadMessagesStreaming(187-200)
services/platform/convex/model/threads/get_latest_tool_message.ts (2)
services/platform/convex/model/threads/index.ts (2)
LatestToolMessage(17-17)getLatestToolMessage(12-12)services/platform/convex/threads.ts (1)
getLatestToolMessage(168-180)
🪛 Ruff (0.14.8)
services/crawler/app/models.py
119-119: Boolean positional value in function call
(FBT003)
services/crawler/app/converter_service.py
237-237: Boolean-typed positional argument in function definition
(FBT001)
237-237: Boolean default positional argument in function definition
(FBT002)
334-334: Boolean-typed positional argument in function definition
(FBT001)
334-334: Boolean default positional argument in function definition
(FBT002)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build Docker Image
🔇 Additional comments (19)
services/platform/convex/model/chat_agent/generate_agent_response.ts (2)
303-327: LGTM! Streaming setup is correct.The switch to
streamTextwithsaveStreamDeltas: trueproperly enables real-time streaming for UI updates. The parameter structure and context options are correct for the streaming API.
329-341: LGTM! Stream consumption pattern is correct.The implementation correctly:
- Iterates through
textStreamto accumulate the final text- Awaits
stepsandusagepromises after stream completion- Assembles the result with proper typing
This follows the expected pattern for consuming streaming responses from the AI SDK.
services/platform/convex/agent_tools/convex_tools/crawler/web_read_tool.ts (1)
94-117: The documentation lists an inaccurate set of search engines—DuckDuckGo is actually disabled in the SearXNG configuration, and Wikipedia and Startpage are enabled but not mentioned.Update the description at lines 101-102 to reflect the actual enabled engines: Brave, Google, Bing, Wikipedia, and Startpage (DuckDuckGo is disabled in the default configuration).
Alternatively, make the description config-agnostic: "Aggregates results from multiple search engines (depending on configuration)."
services/platform/convex/agent_tools/convex_tools/crawler/helpers/types.ts (1)
61-65: No breaking change—next_action_requiredis always assigned by the sole producersearchWeb().The field was already being set in all code paths (lines 75–89 of
search_web.ts), so making it required in the type signature simply documents existing behavior. All instantiations already provide it, and no other code constructsWebReadSearchResultobjects.The refactoring suggestion to use a structured field (e.g.,
{ type: 'fetch_url'|'none', url?: string, reason?: string }) is worth considering for maintainability and robustness, but it is optional—the current string-based approach works correctly since it's generated internally bysearchWeb(), not parsed from external input.services/crawler/app/models.py (1)
118-121: Acknowledge that improved image quality is an intentional feature enhancement, not a breaking change.The changes to
ImageOptionsdefaults (quality 90→100, width 800→1200, scale 2.0 for Retina support) are documented in commit 75b6d86 as part of a feature to improve image generation quality with high-DPI support. This is an intentional enhancement, not an undocumented breaking change.The implementation is sound:
- Code comments in
converter_service.pyexplicitly document the purpose ("high-quality rendering" and "high-quality output")- Descriptions are clear in English per guidelines
UrlToImageRequestalready overrides width to 1280, showing deliberate consideration of width adjustments- The scale parameter properly defaults to "device" mode for Retina displays when scale > 1.0
No action required—this is a documented feature enhancement.
services/platform/convex/lib/create_chat_agent.ts (1)
112-127: Clear documentation of the two-step web_read pattern.The expanded guidance effectively clarifies that the search operation returns only URLs with snippets and that a follow-up fetch_url call is required to retrieve actual content. The step-by-step usage pattern is well-structured and will help the agent correctly retrieve real-world data.
services/platform/convex/threads.ts (1)
163-180: LGTM!The query is well-structured with proper validation and follows the thin wrapper pattern by delegating to the model layer.
services/platform/convex/model/threads/index.ts (1)
12-18: LGTM!The new exports are properly structured and follow the module's re-export pattern.
services/platform/convex/model/threads/get_latest_tool_message.ts (3)
1-23: LGTM!The interface is well-designed with clear documentation explaining its purpose for displaying dynamic loading status in the UI.
24-72: LGTM!The helper functions are well-implemented with appropriate type handling for dynamic message content. The use of
Setfor deduplication inextractToolNamesis a good practice.
74-123: LGTM!The function logic correctly identifies the latest tool-related message and determines its status. The approach of fetching 10 recent messages and checking for both tool role messages and assistant messages with tool-calls is comprehensive and well-structured.
services/platform/app/(app)/dashboard/[id]/chat/components/chat-interface.tsx (4)
42-68: LGTM!The utility functions are well-implemented with appropriate error handling. The
extractHostnamefunction correctly handles invalid URLs, andtruncateuses the proper ellipsis character.
70-132: LGTM!The
formatToolDetailfunction provides comprehensive tool formatting with context-aware display text. The special handling forweb_readoperations andrag_searchwith query details enhances the user experience during tool execution.
523-528: LGTM!The conditional rendering logic correctly displays the
ThinkingAnimationonly when loading and before streaming text appears, providing a smooth user experience during the transition from tool execution to text streaming.
263-283: UIMessage.key is a valid unique identifier for message IDs.The
keyproperty inUIMessage(from@convex-dev/agent/react) is a string that serves as a unique identifier, making it appropriate for use as theidwhen converting toChatMessageformat on line 274.services/platform/convex/agent_tools/convex_tools/files/image_tool.ts (1)
44-44: LGTM! Scale parameter correctly integrated.The addition of the scale parameter with proper validation (1.0–4.0 range) and the updated guidance about auto-sizing dimensions align well with high-quality image rendering requirements. The default of 2.0 is appropriate for Retina displays.
Also applies to: 78-81
services/platform/convex/model/documents/types.ts (1)
133-133: LGTM! Type definition is well-documented.The scale parameter is properly typed with a clear comment documenting the valid range (1.0–4.0) and default value (2.0).
services/crawler/app/main.py (1)
410-410: Scale validation is already implemented in Pydantic models.The scale parameter has proper validation in place. Both
ImageOptionsand the request models include scale validation with constraintsge=1.0, le=4.0, ensuring only values in the 1.0–4.0 range are accepted. Pydantic will automatically reject invalid values during request validation, so no additional changes are needed.services/platform/convex/model/documents/generate_document_helpers.ts (1)
113-116: These default changes were intentional and documented as part of image quality improvements.The commit "feat: improve image generation quality with high-DPI support" explicitly documents increasing quality from 90 to 100, width from 800 to 1200px, and adding scale 2.0 for Retina displays. While the combined effect increases file sizes (potentially 5–10×), this tradeoff was intentional for high-DPI support.
Configuration options already exist—
imageOptionsaccepts optional parameters to override quality, width, and scale on a per-call basis. If infrastructure capacity is a concern, document the available customization options (e.g., lower quality or scale for cost-sensitive scenarios).
…agnostics - Delete context_overflow_retry.ts helper module - Instead of retrying on empty responses, throw detailed error with: - Model info, token counts, step-by-step execution details - Finish reasons, tool call counts, warnings, and provider errors - Add finishReason and warnings to stream result type - Simplify generate_agent_response by removing retry logic
4 tasks
7 tasks
4 tasks
yannickmonney
pushed a commit
that referenced
this pull request
Apr 8, 2026
larryro
added a commit
that referenced
this pull request
May 7, 2026
…OCTOU Adds assertSafeRetentionDelete to internal_mutations_retention.ts and threads the new cutoffMs through every retention dispatcher call site. The guard runs in-mutation (V8 transaction) and closes three round-2 findings simultaneously: - W1 #4 — snapshot-race vs legal hold. The dispatcher's loadActiveHoldsForOrg snapshot is up to 25 min stale; a hold placed AFTER snapshot but BEFORE the per-row mutation otherwise had zero protection. Re-reading inside each mutation makes the in-mutation read the only authoritative gate. - W6 #10 — cross-org corruption. Every deleteExpired* trusted args.organizationId blindly; a swapped id silently deleted org A's row while logging the audit under org B (and forking the per-org hash chain). Now we assert row.organizationId === args.organizationId. - W4 #13 — cutoff TOCTOU. Between the dispatcher's listExpired query and the per-row delete, a user can re-touch the row (chat thread.updatedAt bump, document patch). The mutation now re-evaluates cutoff against (updatedAt ?? _creationTime) and skips when the row is no longer eligible. Mutations updated (each accepts new optional cutoffMs): - deleteExpiredDocument (targetType: 'document') - deleteExpiredThread (targetType: 'thread') - deleteExpiredWorkflowExecution (targetType: 'execution') - deleteExpiredWorkflowTriggerLog (orgId + cutoff only) - deleteExpiredCustomer / Vendor / ExternalConversation / PromptTemplate / MessageFeedback / MemoryAuditRow / ChatFilterEvent / UsageLedgerRow (orgId + cutoff) Out-of-scope here: - deleteExpiredTempFile, deleteExpiredMessageMetadata, deleteExpiredTwoFactorAttempt, deleteExpiredLoginAttempt, deleteExpiredLoginBlockCounter — none of these carry organizationId on their args today (they're either cross-org-by-shape or have an indirect link). The orgHeld / cross-org checks are not applicable without an org backreference, which is itself a separate phase-10 follow-up. Their existing absence-of-row idempotence is preserved.
larryro
added a commit
that referenced
this pull request
May 7, 2026
…cies Bundle of round-2-confirmed cross-tenant fixes plus the dead-code delete of the semantic LLM response cache. POLICY_TYPES drift (W6 #5) - lib/shared/schemas/governance.ts now includes 'data_classification_notice' to match the Convex enum, killing the `as const` cast at use-data-classification-notice.ts:50. documents/compare_documents.ts (W6 #8) - Convex `_storage` is a global namespace; org membership alone was not enough to gate `ctx.storage.getUrl`. Adds a JOIN through fileMetadata via the new internal query verifyStorageIdsBelongToOrg to confirm both `baseStorageId` and `comparisonStorageId` are owned by the caller's org. Refuses with a clear error otherwise. Pattern copied from agent_tools/documents/helpers/retrieve_document.ts. file_metadata/actions.ts::checkFileRagStatuses (W6 #9) - Was an unauthenticated public action that could flip any org's fileMetadata.ragStatus to `failed` via expireStaleRagQueue (DoS, pre-existing on `main`). Now requires `getAuthUser` and filters storageIds to ones owned by an org the caller is a member of via the new file_metadata.internal_queries.filterStorageIdsByCallerOrg. governance/queries.ts (W6 #11) - getPolicy + listPolicies now apply a member-readable allow-list (data_classification_notice, feature_flags, pii_config, chat_filter, personalization, upload_policy, default_models). All other types — login_policy.trustedProxies, password_policy, two_factor_policy, model_access.rules, budgets, retention_policy, moderation_provider.endpoint, system_prompt — are admin-only. listPolicies silently filters those out for non-admins. semantic LLM response cache — DELETE (W6 #12 + #13) - Round-2 v05 confirmed the lookup is structurally cross-tenant (filters only on agent_name, model, expires_at, similarity; ignores user_id / organization_id even though they're stored). The platform helpers `lookupSemanticCache` / `storeSemanticCacheAsync` had ZERO callers in the monorepo, the FastAPI router was mounted but unreachable from platform — a latent foot-gun primed for the next dev to wire up unaware. Deletes: - services/platform/convex/lib/response_cache/semantic_cache.ts - services/platform/convex/lib/response_cache/internal_actions.ts - services/rag/app/routers/llm_cache.py - services/rag/app/services/llm_response_cache.py Plus the corresponding imports in routers/__init__.py, main.py, rag_service.py. Also removes the two empty-catch violations in semantic_cache.ts (no longer applicable). The exact-key Convex `lib/response_cache/{internal_mutations, internal_queries}.ts` cache stays — it is the actually-wired one and is correctly org-scoped.
larryro
added a commit
that referenced
this pull request
May 8, 2026
…OCTOU Adds assertSafeRetentionDelete to internal_mutations_retention.ts and threads the new cutoffMs through every retention dispatcher call site. The guard runs in-mutation (V8 transaction) and closes three round-2 findings simultaneously: - W1 #4 — snapshot-race vs legal hold. The dispatcher's loadActiveHoldsForOrg snapshot is up to 25 min stale; a hold placed AFTER snapshot but BEFORE the per-row mutation otherwise had zero protection. Re-reading inside each mutation makes the in-mutation read the only authoritative gate. - W6 #10 — cross-org corruption. Every deleteExpired* trusted args.organizationId blindly; a swapped id silently deleted org A's row while logging the audit under org B (and forking the per-org hash chain). Now we assert row.organizationId === args.organizationId. - W4 #13 — cutoff TOCTOU. Between the dispatcher's listExpired query and the per-row delete, a user can re-touch the row (chat thread.updatedAt bump, document patch). The mutation now re-evaluates cutoff against (updatedAt ?? _creationTime) and skips when the row is no longer eligible. Mutations updated (each accepts new optional cutoffMs): - deleteExpiredDocument (targetType: 'document') - deleteExpiredThread (targetType: 'thread') - deleteExpiredWorkflowExecution (targetType: 'execution') - deleteExpiredWorkflowTriggerLog (orgId + cutoff only) - deleteExpiredCustomer / Vendor / ExternalConversation / PromptTemplate / MessageFeedback / MemoryAuditRow / ChatFilterEvent / UsageLedgerRow (orgId + cutoff) Out-of-scope here: - deleteExpiredTempFile, deleteExpiredMessageMetadata, deleteExpiredTwoFactorAttempt, deleteExpiredLoginAttempt, deleteExpiredLoginBlockCounter — none of these carry organizationId on their args today (they're either cross-org-by-shape or have an indirect link). The orgHeld / cross-org checks are not applicable without an org backreference, which is itself a separate phase-10 follow-up. Their existing absence-of-row idempotence is preserved.
larryro
added a commit
that referenced
this pull request
May 8, 2026
…cies Bundle of round-2-confirmed cross-tenant fixes plus the dead-code delete of the semantic LLM response cache. POLICY_TYPES drift (W6 #5) - lib/shared/schemas/governance.ts now includes 'data_classification_notice' to match the Convex enum, killing the `as const` cast at use-data-classification-notice.ts:50. documents/compare_documents.ts (W6 #8) - Convex `_storage` is a global namespace; org membership alone was not enough to gate `ctx.storage.getUrl`. Adds a JOIN through fileMetadata via the new internal query verifyStorageIdsBelongToOrg to confirm both `baseStorageId` and `comparisonStorageId` are owned by the caller's org. Refuses with a clear error otherwise. Pattern copied from agent_tools/documents/helpers/retrieve_document.ts. file_metadata/actions.ts::checkFileRagStatuses (W6 #9) - Was an unauthenticated public action that could flip any org's fileMetadata.ragStatus to `failed` via expireStaleRagQueue (DoS, pre-existing on `main`). Now requires `getAuthUser` and filters storageIds to ones owned by an org the caller is a member of via the new file_metadata.internal_queries.filterStorageIdsByCallerOrg. governance/queries.ts (W6 #11) - getPolicy + listPolicies now apply a member-readable allow-list (data_classification_notice, feature_flags, pii_config, chat_filter, personalization, upload_policy, default_models). All other types — login_policy.trustedProxies, password_policy, two_factor_policy, model_access.rules, budgets, retention_policy, moderation_provider.endpoint, system_prompt — are admin-only. listPolicies silently filters those out for non-admins. semantic LLM response cache — DELETE (W6 #12 + #13) - Round-2 v05 confirmed the lookup is structurally cross-tenant (filters only on agent_name, model, expires_at, similarity; ignores user_id / organization_id even though they're stored). The platform helpers `lookupSemanticCache` / `storeSemanticCacheAsync` had ZERO callers in the monorepo, the FastAPI router was mounted but unreachable from platform — a latent foot-gun primed for the next dev to wire up unaware. Deletes: - services/platform/convex/lib/response_cache/semantic_cache.ts - services/platform/convex/lib/response_cache/internal_actions.ts - services/rag/app/routers/llm_cache.py - services/rag/app/services/llm_response_cache.py Plus the corresponding imports in routers/__init__.py, main.py, rag_service.py. Also removes the two empty-catch violations in semantic_cache.ts (no longer applicable). The exact-key Convex `lib/response_cache/{internal_mutations, internal_queries}.ts` cache stays — it is the actually-wired one and is correctly org-scoped.
larryro
added a commit
that referenced
this pull request
May 8, 2026
…OCTOU Adds assertSafeRetentionDelete to internal_mutations_retention.ts and threads the new cutoffMs through every retention dispatcher call site. The guard runs in-mutation (V8 transaction) and closes three round-2 findings simultaneously: - W1 #4 — snapshot-race vs legal hold. The dispatcher's loadActiveHoldsForOrg snapshot is up to 25 min stale; a hold placed AFTER snapshot but BEFORE the per-row mutation otherwise had zero protection. Re-reading inside each mutation makes the in-mutation read the only authoritative gate. - W6 #10 — cross-org corruption. Every deleteExpired* trusted args.organizationId blindly; a swapped id silently deleted org A's row while logging the audit under org B (and forking the per-org hash chain). Now we assert row.organizationId === args.organizationId. - W4 #13 — cutoff TOCTOU. Between the dispatcher's listExpired query and the per-row delete, a user can re-touch the row (chat thread.updatedAt bump, document patch). The mutation now re-evaluates cutoff against (updatedAt ?? _creationTime) and skips when the row is no longer eligible. Mutations updated (each accepts new optional cutoffMs): - deleteExpiredDocument (targetType: 'document') - deleteExpiredThread (targetType: 'thread') - deleteExpiredWorkflowExecution (targetType: 'execution') - deleteExpiredWorkflowTriggerLog (orgId + cutoff only) - deleteExpiredCustomer / Vendor / ExternalConversation / PromptTemplate / MessageFeedback / MemoryAuditRow / ChatFilterEvent / UsageLedgerRow (orgId + cutoff) Out-of-scope here: - deleteExpiredTempFile, deleteExpiredMessageMetadata, deleteExpiredTwoFactorAttempt, deleteExpiredLoginAttempt, deleteExpiredLoginBlockCounter — none of these carry organizationId on their args today (they're either cross-org-by-shape or have an indirect link). The orgHeld / cross-org checks are not applicable without an org backreference, which is itself a separate phase-10 follow-up. Their existing absence-of-row idempotence is preserved.
larryro
added a commit
that referenced
this pull request
May 8, 2026
…cies Bundle of round-2-confirmed cross-tenant fixes plus the dead-code delete of the semantic LLM response cache. POLICY_TYPES drift (W6 #5) - lib/shared/schemas/governance.ts now includes 'data_classification_notice' to match the Convex enum, killing the `as const` cast at use-data-classification-notice.ts:50. documents/compare_documents.ts (W6 #8) - Convex `_storage` is a global namespace; org membership alone was not enough to gate `ctx.storage.getUrl`. Adds a JOIN through fileMetadata via the new internal query verifyStorageIdsBelongToOrg to confirm both `baseStorageId` and `comparisonStorageId` are owned by the caller's org. Refuses with a clear error otherwise. Pattern copied from agent_tools/documents/helpers/retrieve_document.ts. file_metadata/actions.ts::checkFileRagStatuses (W6 #9) - Was an unauthenticated public action that could flip any org's fileMetadata.ragStatus to `failed` via expireStaleRagQueue (DoS, pre-existing on `main`). Now requires `getAuthUser` and filters storageIds to ones owned by an org the caller is a member of via the new file_metadata.internal_queries.filterStorageIdsByCallerOrg. governance/queries.ts (W6 #11) - getPolicy + listPolicies now apply a member-readable allow-list (data_classification_notice, feature_flags, pii_config, chat_filter, personalization, upload_policy, default_models). All other types — login_policy.trustedProxies, password_policy, two_factor_policy, model_access.rules, budgets, retention_policy, moderation_provider.endpoint, system_prompt — are admin-only. listPolicies silently filters those out for non-admins. semantic LLM response cache — DELETE (W6 #12 + #13) - Round-2 v05 confirmed the lookup is structurally cross-tenant (filters only on agent_name, model, expires_at, similarity; ignores user_id / organization_id even though they're stored). The platform helpers `lookupSemanticCache` / `storeSemanticCacheAsync` had ZERO callers in the monorepo, the FastAPI router was mounted but unreachable from platform — a latent foot-gun primed for the next dev to wire up unaware. Deletes: - services/platform/convex/lib/response_cache/semantic_cache.ts - services/platform/convex/lib/response_cache/internal_actions.ts - services/rag/app/routers/llm_cache.py - services/rag/app/services/llm_response_cache.py Plus the corresponding imports in routers/__init__.py, main.py, rag_service.py. Also removes the two empty-catch violations in semantic_cache.ts (no longer applicable). The exact-key Convex `lib/response_cache/{internal_mutations, internal_queries}.ts` cache stays — it is the actually-wired one and is correctly org-scoped.
larryro
added a commit
that referenced
this pull request
May 9, 2026
…stodian cascade Round-1 #10/#12/#13 + round-2 V3 confirmed three custodian-cascade gaps (P0-6 retention guard, P0-7 deleteDocumentById, P0-8 cleanupMessageMetadata + cleanupChatFilterEvents). The user's observation made the deeper truth clear: in production, only `org` and `userMembership` hold target types are operator-facing — `thread` / `document` / `execution` were modeled in the original fine-grained hold design but never wired into the place-hold UI (`PICKER_TARGET_TYPES = ['userMembership', 'org']`), and `usePlaceLegalHold` is called from a single dialog. The corresponding `holds.threadIds` / `documentIds` / `executionIds` Sets in `ActiveHolds` therefore stayed empty in every deployment, while ~80 LOC of dead per-row checks proliferated through retention, erasure, restore, folders, and the cascade helper. This commit collapses the model to its actual shape. Schema-level narrowing (legal_hold.ts): - HOLD_TARGET_TYPES = ['userMembership', 'org'] (write-side) - ActiveHolds = { orgHeld, userMembershipIds } - loadActiveHolds switch reduces to 2 cases; legacy thread/document/ execution rows in the table (if any) are intentionally ignored — they're surfaced in admin UI as `(legacy)` via the read-side validator in legal_hold_queries.ts. - resolveAndAssertTarget keeps only `org` + `userMembership` cases. - isHeld() simplified to org + custodian. Guard simplification (legal_hold_guard.ts): - assertNotHeld drops the per-row branches; `targetType`/`targetId` are now error-message context only. The user-custodian cascade via `authorUserId` is the sole remaining gate beyond org-wide. assertSafeRetentionDelete (internal_mutations_retention.ts): - Drops `targetType?` / `targetId?` args; adds `authorUserId?`. - Internal handler is now: cross-org check → TOCTOU check → org-wide hold → user-custodian cascade. Single, clear contract. P0-6 cascade wiring — every retention mutation passes the row's author user id (~15 call sites in internal_mutations_retention.ts): - documents → doc.createdBy - threads → thread.userId - workflowExecution → execution.userId - workflowTriggerLog → exec.userId via wfExecutionId join - messageFeedback → row.userId - memoryAudit → row.subjectUserId (with thread-fallback) - chatFilterEvent → parent thread's userId via index lookup - usageLedger → row.userId - promptTemplate → row.createdBy - messageMetadata → parent thread's userId (closes P0-8 directly) - tempFile, customer, vendor, externalConversation: org-only (no author concept on the row) P0-8 cascade fix — cleanupMessageMetadata + cleanupChatFilterEvents no longer rely on the now-empty `holds.threadIds` Set; both look up the parent thread's `userId` for the user-membership cascade (messageMetadata cleanup mutation was already P0-8 fixed in commit 1). Other dead-check removals: - retention_cleanup.ts: ~10 dead `holds.threadIds.has` / `documentIds.has` / `executionIds.has` checks removed; per-row custodian cascade either replaces them or the mutation re-checks. - governance/restore.ts: drops dead per-row branches; user-custodian cascade in restoreSoftDeletedRow (commit 1) covers the rest. - threads/cascade_helpers.ts: per-thread hold check replaced with a thread-metadata lookup → `userMembershipIds.has(thread.userId)`. - threads/delete_chat_thread.ts + restore_chat_thread.ts: drop `holds.threadIds.has(threadId)`; ownerHeld + orgHeld cover all refusal cases. - folders/mutations.ts assertNoHeldDescendantDocs: per-document hold check replaced with `userMembershipIds.has(doc.createdBy)`. - governance/erasure.ts requestErasure: drops heldThreadIds / heldDocumentIds collection — refusal is now `orgHeld || userCustodianHeld`. eraseSubjectDocuments drops the per-row held-document check (subject is by definition the only author in the iteration; user-custodian gate applied at request time). - legal_hold_internal.ts: loadActiveHoldsForOrg returns the narrowed ActiveHolds shape across the action/query boundary. Tests rewritten — `governance/__tests__/retention_legal_hold_gaps.test.ts`: - Drops thread/document/execution per-row hold cases (behaviour deleted on purpose). - Adds 6 cases covering the simplified contract: authorUserId cascade, TOCTOU + cross-org pre-checks, missing authorUserId grandfather, org-wide hold blanket refusal. - Source-grep regressions: every retention mutation passes the right authorUserId; cleanup actions pre-filter via the right `holds.userMembershipIds.has(...)`; per-row Sets stay deleted from the cleanup / erasure / guard sources. Note: this is a one-way migration. Any deployment with active legacy thread/document/execution holds loses enforcement of those holds going forward; the read-side validator in legal_hold_queries keeps showing them as `(legacy)`. Acceptable per project memory (demo stage, no prod use). Pre-prod check: convert to user/org holds first if any deployment has them. Verified: typecheck clean; 851 tests pass across affected dirs; full vitest run shows 5641/5642 pass (1 pre-existing flake in canvas-pane.test.tsx, unrelated).
larryro
added a commit
that referenced
this pull request
May 9, 2026
…te gates P0-9 — Org-wide hold UI invisibility (round-1 #12, round-2 V4): Before: `heldByTargetValidator` declared `via: 'org'` but no producer ever emitted it. With only an org-wide hold active, `getLegalHoldByTarget` returned null and `listActiveHoldTargetIds` returned an empty list → no chat-sidebar lock icons, no admin-grid badges, no banner. Backend refused destructive actions but the UX showed nothing until the user attempted a delete and hit a confusing error. Fix: - `getLegalHoldByTarget`: third pass after direct + custodian-cascade looks up the org-wide hold row and surfaces it with `via: 'org'`. - `listActiveHoldTargetIds`: returns a new `orgHeld: boolean` flag alongside the per-target id set; the chat-sidebar ORs the flag with the per-thread set so every row gets the lock indicator when the whole org is on a hold. - `chat-history-sidebar.tsx`: replaces direct `heldThreadIds.has(id)` checks with an `isThreadHeld(id)` helper that ORs `orgWideHeld`. P0-10 — `prepareOrganizationDeletion` bypasses holds (round-1 #13, V4): `cascadeOnOrgDeleted` hard-deletes userMemories + userPreferences (and per Commit 1, every thread-bound chat-upload file) for every member of the org. The mutation refused only on owner-role check; legal-hold status was never consulted. Adds `assertNotHeld(ctx, orgId, 'org', orgId, undefined, authUser._id)` before the cascade — refuses on org-wide hold OR on a userMembership cascade against the owner deleting. `GuardedTargetType` extended with `'org'` for the error-payload context. P0-11 — `removeMember` bypasses custodian holds (round-1 #13, V4): `cascadeOnMemberRemoved` cascades into userMemories + userPreferences (+ chat-upload files) scoped to the member being removed. Mutation previously checked only role. Adds `assertNotHeld(ctx, orgId, 'userMembership', member.userId, undefined, member.userId)` before the cascade — refuses when the org is on a hold OR when the member themselves is on a custodian hold. Self-author cascade uses the same user id for `targetId` and `authorUserId` so the gate fires regardless of which leg matches. Verified: typecheck clean; 908 tests pass across affected dirs.
larryro
added a commit
that referenced
this pull request
May 9, 2026
…OCTOU Adds assertSafeRetentionDelete to internal_mutations_retention.ts and threads the new cutoffMs through every retention dispatcher call site. The guard runs in-mutation (V8 transaction) and closes three round-2 findings simultaneously: - W1 #4 — snapshot-race vs legal hold. The dispatcher's loadActiveHoldsForOrg snapshot is up to 25 min stale; a hold placed AFTER snapshot but BEFORE the per-row mutation otherwise had zero protection. Re-reading inside each mutation makes the in-mutation read the only authoritative gate. - W6 #10 — cross-org corruption. Every deleteExpired* trusted args.organizationId blindly; a swapped id silently deleted org A's row while logging the audit under org B (and forking the per-org hash chain). Now we assert row.organizationId === args.organizationId. - W4 #13 — cutoff TOCTOU. Between the dispatcher's listExpired query and the per-row delete, a user can re-touch the row (chat thread.updatedAt bump, document patch). The mutation now re-evaluates cutoff against (updatedAt ?? _creationTime) and skips when the row is no longer eligible. Mutations updated (each accepts new optional cutoffMs): - deleteExpiredDocument (targetType: 'document') - deleteExpiredThread (targetType: 'thread') - deleteExpiredWorkflowExecution (targetType: 'execution') - deleteExpiredWorkflowTriggerLog (orgId + cutoff only) - deleteExpiredCustomer / Vendor / ExternalConversation / PromptTemplate / MessageFeedback / MemoryAuditRow / ChatFilterEvent / UsageLedgerRow (orgId + cutoff) Out-of-scope here: - deleteExpiredTempFile, deleteExpiredMessageMetadata, deleteExpiredTwoFactorAttempt, deleteExpiredLoginAttempt, deleteExpiredLoginBlockCounter — none of these carry organizationId on their args today (they're either cross-org-by-shape or have an indirect link). The orgHeld / cross-org checks are not applicable without an org backreference, which is itself a separate phase-10 follow-up. Their existing absence-of-row idempotence is preserved.
larryro
added a commit
that referenced
this pull request
May 9, 2026
…cies Bundle of round-2-confirmed cross-tenant fixes plus the dead-code delete of the semantic LLM response cache. POLICY_TYPES drift (W6 #5) - lib/shared/schemas/governance.ts now includes 'data_classification_notice' to match the Convex enum, killing the `as const` cast at use-data-classification-notice.ts:50. documents/compare_documents.ts (W6 #8) - Convex `_storage` is a global namespace; org membership alone was not enough to gate `ctx.storage.getUrl`. Adds a JOIN through fileMetadata via the new internal query verifyStorageIdsBelongToOrg to confirm both `baseStorageId` and `comparisonStorageId` are owned by the caller's org. Refuses with a clear error otherwise. Pattern copied from agent_tools/documents/helpers/retrieve_document.ts. file_metadata/actions.ts::checkFileRagStatuses (W6 #9) - Was an unauthenticated public action that could flip any org's fileMetadata.ragStatus to `failed` via expireStaleRagQueue (DoS, pre-existing on `main`). Now requires `getAuthUser` and filters storageIds to ones owned by an org the caller is a member of via the new file_metadata.internal_queries.filterStorageIdsByCallerOrg. governance/queries.ts (W6 #11) - getPolicy + listPolicies now apply a member-readable allow-list (data_classification_notice, feature_flags, pii_config, chat_filter, personalization, upload_policy, default_models). All other types — login_policy.trustedProxies, password_policy, two_factor_policy, model_access.rules, budgets, retention_policy, moderation_provider.endpoint, system_prompt — are admin-only. listPolicies silently filters those out for non-admins. semantic LLM response cache — DELETE (W6 #12 + #13) - Round-2 v05 confirmed the lookup is structurally cross-tenant (filters only on agent_name, model, expires_at, similarity; ignores user_id / organization_id even though they're stored). The platform helpers `lookupSemanticCache` / `storeSemanticCacheAsync` had ZERO callers in the monorepo, the FastAPI router was mounted but unreachable from platform — a latent foot-gun primed for the next dev to wire up unaware. Deletes: - services/platform/convex/lib/response_cache/semantic_cache.ts - services/platform/convex/lib/response_cache/internal_actions.ts - services/rag/app/routers/llm_cache.py - services/rag/app/services/llm_response_cache.py Plus the corresponding imports in routers/__init__.py, main.py, rag_service.py. Also removes the two empty-catch violations in semantic_cache.ts (no longer applicable). The exact-key Convex `lib/response_cache/{internal_mutations, internal_queries}.ts` cache stays — it is the actually-wired one and is correctly org-scoped.
larryro
added a commit
that referenced
this pull request
May 9, 2026
…stodian cascade Round-1 #10/#12/#13 + round-2 V3 confirmed three custodian-cascade gaps (P0-6 retention guard, P0-7 deleteDocumentById, P0-8 cleanupMessageMetadata + cleanupChatFilterEvents). The user's observation made the deeper truth clear: in production, only `org` and `userMembership` hold target types are operator-facing — `thread` / `document` / `execution` were modeled in the original fine-grained hold design but never wired into the place-hold UI (`PICKER_TARGET_TYPES = ['userMembership', 'org']`), and `usePlaceLegalHold` is called from a single dialog. The corresponding `holds.threadIds` / `documentIds` / `executionIds` Sets in `ActiveHolds` therefore stayed empty in every deployment, while ~80 LOC of dead per-row checks proliferated through retention, erasure, restore, folders, and the cascade helper. This commit collapses the model to its actual shape. Schema-level narrowing (legal_hold.ts): - HOLD_TARGET_TYPES = ['userMembership', 'org'] (write-side) - ActiveHolds = { orgHeld, userMembershipIds } - loadActiveHolds switch reduces to 2 cases; legacy thread/document/ execution rows in the table (if any) are intentionally ignored — they're surfaced in admin UI as `(legacy)` via the read-side validator in legal_hold_queries.ts. - resolveAndAssertTarget keeps only `org` + `userMembership` cases. - isHeld() simplified to org + custodian. Guard simplification (legal_hold_guard.ts): - assertNotHeld drops the per-row branches; `targetType`/`targetId` are now error-message context only. The user-custodian cascade via `authorUserId` is the sole remaining gate beyond org-wide. assertSafeRetentionDelete (internal_mutations_retention.ts): - Drops `targetType?` / `targetId?` args; adds `authorUserId?`. - Internal handler is now: cross-org check → TOCTOU check → org-wide hold → user-custodian cascade. Single, clear contract. P0-6 cascade wiring — every retention mutation passes the row's author user id (~15 call sites in internal_mutations_retention.ts): - documents → doc.createdBy - threads → thread.userId - workflowExecution → execution.userId - workflowTriggerLog → exec.userId via wfExecutionId join - messageFeedback → row.userId - memoryAudit → row.subjectUserId (with thread-fallback) - chatFilterEvent → parent thread's userId via index lookup - usageLedger → row.userId - promptTemplate → row.createdBy - messageMetadata → parent thread's userId (closes P0-8 directly) - tempFile, customer, vendor, externalConversation: org-only (no author concept on the row) P0-8 cascade fix — cleanupMessageMetadata + cleanupChatFilterEvents no longer rely on the now-empty `holds.threadIds` Set; both look up the parent thread's `userId` for the user-membership cascade (messageMetadata cleanup mutation was already P0-8 fixed in commit 1). Other dead-check removals: - retention_cleanup.ts: ~10 dead `holds.threadIds.has` / `documentIds.has` / `executionIds.has` checks removed; per-row custodian cascade either replaces them or the mutation re-checks. - governance/restore.ts: drops dead per-row branches; user-custodian cascade in restoreSoftDeletedRow (commit 1) covers the rest. - threads/cascade_helpers.ts: per-thread hold check replaced with a thread-metadata lookup → `userMembershipIds.has(thread.userId)`. - threads/delete_chat_thread.ts + restore_chat_thread.ts: drop `holds.threadIds.has(threadId)`; ownerHeld + orgHeld cover all refusal cases. - folders/mutations.ts assertNoHeldDescendantDocs: per-document hold check replaced with `userMembershipIds.has(doc.createdBy)`. - governance/erasure.ts requestErasure: drops heldThreadIds / heldDocumentIds collection — refusal is now `orgHeld || userCustodianHeld`. eraseSubjectDocuments drops the per-row held-document check (subject is by definition the only author in the iteration; user-custodian gate applied at request time). - legal_hold_internal.ts: loadActiveHoldsForOrg returns the narrowed ActiveHolds shape across the action/query boundary. Tests rewritten — `governance/__tests__/retention_legal_hold_gaps.test.ts`: - Drops thread/document/execution per-row hold cases (behaviour deleted on purpose). - Adds 6 cases covering the simplified contract: authorUserId cascade, TOCTOU + cross-org pre-checks, missing authorUserId grandfather, org-wide hold blanket refusal. - Source-grep regressions: every retention mutation passes the right authorUserId; cleanup actions pre-filter via the right `holds.userMembershipIds.has(...)`; per-row Sets stay deleted from the cleanup / erasure / guard sources. Note: this is a one-way migration. Any deployment with active legacy thread/document/execution holds loses enforcement of those holds going forward; the read-side validator in legal_hold_queries keeps showing them as `(legacy)`. Acceptable per project memory (demo stage, no prod use). Pre-prod check: convert to user/org holds first if any deployment has them. Verified: typecheck clean; 851 tests pass across affected dirs; full vitest run shows 5641/5642 pass (1 pre-existing flake in canvas-pane.test.tsx, unrelated).
larryro
added a commit
that referenced
this pull request
May 9, 2026
…te gates P0-9 — Org-wide hold UI invisibility (round-1 #12, round-2 V4): Before: `heldByTargetValidator` declared `via: 'org'` but no producer ever emitted it. With only an org-wide hold active, `getLegalHoldByTarget` returned null and `listActiveHoldTargetIds` returned an empty list → no chat-sidebar lock icons, no admin-grid badges, no banner. Backend refused destructive actions but the UX showed nothing until the user attempted a delete and hit a confusing error. Fix: - `getLegalHoldByTarget`: third pass after direct + custodian-cascade looks up the org-wide hold row and surfaces it with `via: 'org'`. - `listActiveHoldTargetIds`: returns a new `orgHeld: boolean` flag alongside the per-target id set; the chat-sidebar ORs the flag with the per-thread set so every row gets the lock indicator when the whole org is on a hold. - `chat-history-sidebar.tsx`: replaces direct `heldThreadIds.has(id)` checks with an `isThreadHeld(id)` helper that ORs `orgWideHeld`. P0-10 — `prepareOrganizationDeletion` bypasses holds (round-1 #13, V4): `cascadeOnOrgDeleted` hard-deletes userMemories + userPreferences (and per Commit 1, every thread-bound chat-upload file) for every member of the org. The mutation refused only on owner-role check; legal-hold status was never consulted. Adds `assertNotHeld(ctx, orgId, 'org', orgId, undefined, authUser._id)` before the cascade — refuses on org-wide hold OR on a userMembership cascade against the owner deleting. `GuardedTargetType` extended with `'org'` for the error-payload context. P0-11 — `removeMember` bypasses custodian holds (round-1 #13, V4): `cascadeOnMemberRemoved` cascades into userMemories + userPreferences (+ chat-upload files) scoped to the member being removed. Mutation previously checked only role. Adds `assertNotHeld(ctx, orgId, 'userMembership', member.userId, undefined, member.userId)` before the cascade — refuses when the org is on a hold OR when the member themselves is on a custodian hold. Self-author cascade uses the same user id for `targetId` and `authorUserId` so the gate fires regardless of which leg matches. Verified: typecheck clean; 908 tests pass across affected dirs.
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.
Summary by CodeRabbit
New Features
Improvements
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.