fix(DATAGO-123668): Implement verification step in deep research process#1307
fix(DATAGO-123668): Implement verification step in deep research process#1307amir-ghasemi wants to merge 14 commits intomainfrom
Conversation
✅ FOSSA Guard: Licensing (
|
✅ FOSSA Guard: Vulnerability (
|
lgh-solace
left a comment
There was a problem hiding this comment.
Consider these potential issues. Thanks!
🔴
Infinite blocking poll with no timeout
_wait_for_plan_response in deep_research_tools.py loops forever with await asyncio.sleep(0.5). If the SSE connection drops or the user closes the tab before responding, the agent task hangs permanently — no timeout, no cancellation path. The auto-approve timeout was explicitly disabled. At minimum a hard timeout (e.g. 5–10 minutes) should be reinstated so the tool can fail gracefully.
_is_webui_gateway always returns True
The function is supposed to detect whether the current gateway supports interactive verification, but always returns True with a TODO comment. Non-interactive gateways (Slack, Teams, MCP) will trigger the verification step and then block forever. This needs real detection (e.g. reading a flag from a2a_context) before merging, or the verification step must be gated behind an explicit opt-in config flag.
🟠
ResearchPlanVerification is re-submittable after page reload
After the user responds, the component returns null but the deep_research_plan data part remains in the message. On reload the component remounts in its initial interactive state — clicking Start again writes a stale plan_id to the cache with no backend listener. The responded state needs to be persisted (e.g. in the message itself via ChatProvider) rather than held only in local component state.
API call bypasses lib/api pattern
handleResponse in ResearchPlanVerification.tsx calls api.webui.post(...) directly with manual useState for loading — violates the useQuery/useMutation pattern. Should be extracted to lib/api/research/hooks.ts with a useSubmitPlanResponse mutation.
No displayError for user-facing errors
The catch in handleResponse only calls console.error. If the POST fails the user sees nothing. Use displayError from useChatContext().
Native <input> in edit mode
The step edit fields use a native <input type="text"> — should use the standard Input component from @SolaceLabs/solace-agent-mesh-ui.
🟡
console.log left in production code
ChatProvider.tsx in the deep_research_plan case has a console.log that should be removed.
onResponded prop is dead code
ResearchPlanVerification defines an onResponded callback prop but it is never passed at the call site in ChatMessage.tsx. Either wire it up or remove the prop.
skip_verification config key is undocumented
Read via config.get("skip_verification", False) but never mentioned in shared_config.yaml, agent_template.yaml, or any docstring.
Extra LLM call to reformat queries into plan steps
_generate_research_plan makes a full LLM round-trip just to convert search queries into human-readable steps. This adds latency and cost to every invocation. A lightweight text transform (strip search operators, title-case) would likely be sufficient.
auto_approve_seconds is exposed but inert
The field is sent in DeepResearchPlanData, shown in the component props, and has a commented-out countdown UI — but both the backend timeout and frontend timer are disabled. Remove the field or implement it; the current state is misleading.
⚪ Low / Style
React.FC pattern — ResearchPlanVerification should use direct prop typing per frontend patterns.
Missing explicit return type on ResearchPlanVerification.
No Storybook story or test for ResearchPlanVerification.
DashedCircle SVG declared before import statements in ResearchPlanVerification.tsx — minor ordering issue.
Cache key not namespaced by user/session — deep_research_plan_{plan_id} uses a UUID so collision is unlikely, but a compromised or buggy client could write a response for another user's plan if the UUID is known.
932d7d1 to
d7bbcf0
Compare
lgh-solace
left a comment
There was a problem hiding this comment.
Considerations?
-
Wrong cache key in test — tests/unit/gateway/http_sse/routers/test_research.py:85 asserts "deep_research_plan_plan-abc" but the router produces "deep_research_plan:user-1:plan-abc". Test fails on first run.
-
auto_approve_seconds doesn't exist on the model — tests/unit/common/test_deep_research_plan_data.py:27,39,43 constructs and asserts a field that isn't on DeepResearchPlanData. The model docstring promises an "auto-approve countdown timer" though, so someone needs to decide: drop the assertion or add the field.
-
Cache TTL shorter than wait window — routers/research.py:94 stores with expiry=120, but deep_research_tools.py:985 waits up to 600. Users who read the plan for >2 min lose their response. Share the constant between the two.
Other stuff:
- DeepResearchPlanData missing from SignalData union (data_parts.py:554)
- Unused FastAPIRequest param in the new router
- No feature-flag gating for the new UX
- FE: index-as-key during edit, no "cancel edit" affordance, missing aria-labels on icon buttons, research_question sent but never rendered
- Security: new POST endpoint not CSRF-checked, no null guard on user_id
d7bbcf0 to
473a3ec
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds an interactive “deep research plan verification” step spanning backend signaling, a new gateway endpoint for user approval, and WebUI rendering/handling so users can review/edit a plan before deep research proceeds.
Changes:
- Introduces new data parts for deep research plan verification and “plan stale” signaling, plus agent-side waiting/registry plumbing to pause until user approval or timeout.
- Adds a new HTTP SSE gateway endpoint (
POST /research/plan-response) to publish plan approval signals onto the SAM event bus. - Updates WebUI chat rendering and message processing to display an interactive plan card and handle “stale” behavior, with added unit tests.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/gateway/http_sse/routers/test_research.py | Adds behavioral tests for the new plan-response gateway endpoint and payload validation. |
| tests/unit/common/test_deep_research_plan_data.py | Adds tests for the new DeepResearchPlanData model contract and defaults. |
| tests/unit/agent/tools/test_deep_research_verification.py | Adds tests for plan generation, plan verification publish/wait, and query regeneration behavior. |
| src/solace_agent_mesh/gateway/http_sse/routers/research.py | Implements POST /research/plan-response that publishes plan_response events. |
| src/solace_agent_mesh/gateway/http_sse/main.py | Mounts the new research router into the gateway app. |
| src/solace_agent_mesh/common/data_parts.py | Adds DeepResearchPlanData + DeepResearchPlanStaleData and wires them into the data-part union. |
| src/solace_agent_mesh/agent/tools/deep_research_tools.py | Implements plan generation, plan verification sending, waiting for approval, and query regeneration; integrates verification step into deep_research. |
| src/solace_agent_mesh/agent/sac/component.py | Adds a thread-safe registry for plan waiters and APIs to register/drop/resolve plan responses. |
| src/solace_agent_mesh/agent/sac/app.py | Subscribes the agent to the sam/events/deep_research/> category. |
| src/solace_agent_mesh/agent/protocol/event_handlers.py | Routes deep research control events and resolves the appropriate plan waiter future. |
| client/webui/frontend/src/stories/chat/ResearchPlanVerification.test.tsx | Adds component tests covering rendering, Start POST, Cancel behavior, and error handling. |
| client/webui/frontend/src/stories/chat/ResearchPlanVerification.stories.tsx | Adds Storybook stories for pending/single-step/responded plan card states. |
| client/webui/frontend/src/lib/utils/messageProcessing.ts | Treats deep_research_plan as renderable/visible content in filtering logic. |
| client/webui/frontend/src/lib/utils/messageProcessing.test.ts | Adds tests ensuring deep_research_plan is kept and considered visible. |
| client/webui/frontend/src/lib/providers/chat/processChatEvent.ts | Handles deep_research_plan_stale events and marks plan messages as responded/stale. |
| client/webui/frontend/src/lib/components/research/respondedPlansStore.ts | Adds a session-local responded-plan store to suppress re-rendering of already-answered plans on SSE replay. |
| client/webui/frontend/src/lib/components/research/index.ts | Re-exports the new plan verification component. |
| client/webui/frontend/src/lib/components/research/ResearchProgress.tsx | Refactors away from React.FC / React import (JSX transform cleanup). |
| client/webui/frontend/src/lib/components/research/ResearchPlanVerification.tsx | Implements the interactive plan verification card with step editing and Start/Cancel actions. |
| client/webui/frontend/src/lib/components/research/InlineResearchProgress.tsx | Refactors away from React.FC typing. |
| client/webui/frontend/src/lib/components/research/ImageSearchGrid.tsx | Refactors away from React.FC typing. |
| client/webui/frontend/src/lib/components/research/DeepResearchReportContent.tsx | Refactors to named React hook imports and removes React.FC. |
| client/webui/frontend/src/lib/components/chat/ChatMessage.tsx | Renders ResearchPlanVerification when a message contains deep_research_plan data. |
| client/webui/frontend/src/lib/api/research/service.ts | Adds client API call to submit plan approval to /api/v1/research/plan-response. |
| client/webui/frontend/src/lib/api/research/index.ts | Exports the new research API module. |
| client/webui/frontend/src/lib/api/research/hooks.ts | Adds a React Query mutation hook for plan approval submission. |
| client/webui/frontend/src/lib/api/index.ts | Re-exports the new research API module. |
Comments suppressed due to low confidence (1)
client/webui/frontend/src/lib/utils/messageProcessing.ts:20
- The docstring says text parts are kept only when there is no deep research progress or plan, but the function only takes
hasDeepResearchProgressand only filters text on that condition. Either update the comment to match behavior, or extend the API/caller to also detectdeep_research_planand suppress text parts when a plan card is present (similar to progress-only rendering).
/**
* Filters message parts to only renderable data parts and visible content.
* Keeps: compaction_notification, deep_research_progress, deep_research_plan data parts, files, artifacts.
* Keeps text parts only when there is no deep research progress or plan.
*/
export function filterRenderableDataParts(parts: Part[], hasDeepResearchProgress: boolean): Part[] {
return parts.filter(p => {
// Keep deep_research_progress, deep_research_plan, and compaction_notification data parts
if (p.kind === "data") {
const dataPart = p as DataPart;
const dataType = dataPart.data && (dataPart.data as Record<string, unknown>).type;
return dataType === "deep_research_progress" || dataType === "deep_research_plan" || dataType === "compaction_notification";
}
// Filter out text parts if we have deep research progress (to show progress-only)
if (p.kind === "text" && hasDeepResearchProgress) {
return false;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # User may have edited steps - regenerate queries if steps were modified | ||
| user_steps = verification_result.get("steps", plan_steps) | ||
| if user_steps != plan_steps: | ||
| log.info("%s User modified plan steps, regenerating queries", log_identifier) | ||
| queries = await _regenerate_queries_from_steps( | ||
| user_steps, research_question, tool_context, tool_config | ||
| ) |
There was a problem hiding this comment.
verification_result.get("steps", plan_steps) will return None when the response payload includes a steps key with a null/None value (e.g., older client, malformed/malicious signal). That makes user_steps become None, and the subsequent call to _regenerate_queries_from_steps(user_steps, ...) will raise because it expects List[str]. Consider normalizing here (e.g., user_steps = verification_result.get("steps") or plan_steps and/or validating it’s a list of strings before comparing/regenerating).
| const respondedPlans = new Map<string, RespondedAction>(); | ||
| const listeners = new Set<() => void>(); | ||
|
|
||
| export function markPlanResponded(planId: string, action: RespondedAction) { | ||
| respondedPlans.set(planId, action); | ||
| listeners.forEach(listener => listener()); | ||
| } | ||
|
|
||
| export function subscribeRespondedPlans(listener: () => void) { | ||
| listeners.add(listener); | ||
| return () => { | ||
| listeners.delete(listener); | ||
| }; | ||
| } | ||
|
|
||
| export function getRespondedPlansSnapshot(): ReadonlyMap<string, RespondedAction> { | ||
| return respondedPlans; | ||
| } |
There was a problem hiding this comment.
useSyncExternalStore expects getSnapshot to return a value whose identity changes when the store changes. Here getRespondedPlansSnapshot() always returns the same mutable Map instance, and markPlanResponded() mutates it in-place; this can prevent React from detecting updates (snapshot is referentially equal). Consider returning a new Map from getRespondedPlansSnapshot (or using an immutable structure / version counter) so subscribers reliably re-render.
lgh-solace
left a comment
There was a problem hiding this comment.
Please just review the file comments from the co-pilot review. Thanks!
|
|
||
| import { getRespondedPlansSnapshot, markPlanResponded, subscribeRespondedPlans } from "./respondedPlansStore"; | ||
|
|
||
| const DashedCircle = ({ className }: { className?: string }) => ( |
There was a problem hiding this comment.
There is a lucide icon for this that may be cleaner - https://lucide.dev/icons/circle-dashed
…mir/deep-research-plan
…mir/deep-research-plan
|




This pull request introduces support for displaying and handling deep research plan verification in the chat UI. It adds a new
ResearchPlanVerificationcomponent, updates message processing logic to recognize and render research plan data, and ensures proper handling of these messages throughout the chat provider and utility functions. The changes also include comprehensive tests for the new logic.Deep Research Plan Verification Feature:
ResearchPlanVerificationcomponent that displays research plans for user review, allows editing of steps, and provides Start/Cancel actions before deep research begins. The component is integrated into the chat message rendering flow. [1] [2] [3] [4]Chat Message Processing and State Management:
ChatProviderto recognizedeep_research_plandata parts, log them, and manage their insertion or replacement in the chat message list, similar to how progress updates are handled. [1] [2]Message Utilities and Filtering:
deep_research_plandata parts as renderable and visible content, ensuring they are not filtered out and are properly displayed in the chat. [1] [2]Testing:
deep_research_plandata parts are kept by the filtering logic and recognized as containing visible content. [1] [2]When editing: