Repository navigation
Fix/mcp error handling - #128
Amazing-Stardom wants to merge 11 commits into
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Integrate Centralized LLM Error Handling and Offline Preact Dashboard UIOverviewThis update introduces centralized LLM error categorization, standardized UI fallback error templates, and ActionCard support across agent integrations. Additionally, it ships a complete offline Preact and HTM frontend dashboard export with static JSON data shims. Technical Highlights
Impact
|
| SessionID: sessionID, | ||
| ConversationID: convID, | ||
| } | ||
| return c.JSON(http.StatusOK, resp) |
There was a problem hiding this comment.
Severity: warning
The agent loop error returns an HTTP 200 status code with error text, but it should return an HTTP 500 or 502 status code to indicate an internal server error.
Suggestions:
- Return http.StatusInternalServerError or http.StatusBadGateway instead of http.StatusOK when agent loop fails.
| { | ||
| name: "Unknown Error", | ||
| category: aiconnectors.ErrCategoryUnknown, | ||
| want: "", |
There was a problem hiding this comment.
Severity: critical
The test case expects an empty string for an unknown error, but the LLMErrorTemplates defines a non-empty message for ErrCategoryUnknown.
Suggestions:
- Update test case 'Unknown Error' want field to match the non-empty template string defined in provider.go
| if (hunk.Lines) { | ||
| // Merge comments into existing lines | ||
| const lines = hunk.Lines.map(line => { | ||
| const newNum = parseInt(line.NewNum, 10) || 0; |
There was a problem hiding this comment.
Severity: warning
The parseInt function is missing the radix argument; specify base 10 explicitly.
Suggestions:
- Pass radix 10 to parseInt(line.NewNum, 10)
| // Merge comments into existing lines | ||
| const lines = hunk.Lines.map(line => { | ||
| const newNum = parseInt(line.NewNum, 10) || 0; | ||
| const oldNum = parseInt(line.OldNum, 10) || 0; |
There was a problem hiding this comment.
Severity: warning
The parseInt function is missing the radix argument; specify base 10 explicitly.
Suggestions:
- Pass radix 10 to parseInt(line.OldNum, 10)
| </div> | ||
| ${otherSignals.length > 0 && html` | ||
| <div class="blast-hygiene-note"> | ||
| ${otherSignals.map((s, i) => html`<span key=${i}>${s.Name}${s.Detail ? ` — ${s.Detail}` : ''}</span>`)} |
There was a problem hiding this comment.
Severity: warning
The map key i is being used for a dynamic list; a stable unique ID should be used instead.
Suggestions:
- Use a stable unique identifier from
sas thekeyprop, if available. - If no stable ID exists, consider generating one or re-evaluating if
otherSignalscan be reordered/filtered.
| </div> | ||
| ${metaItems.length > 0 && html` | ||
| <div class="comment-meta-line"> | ||
| ${metaItems.map((item, index) => html` |
There was a problem hiding this comment.
Severity: warning
The map key index is being used for a dynamic list; a stable unique ID should be used instead.
Suggestions:
- While
metaItemsderived from fixed properties, usingindexas key is still a risk ifmetaItemsorder or count changes dynamically. IfmetaItemsare truly static in order and content for a givencomment, this is less critical but still a best practice violation. - If
metaItemscan change, assign a stable unique key to each item.
| const buttonColor = () => { | ||
| if (isActive && type === "up") return "#22c55e"; | ||
| if (isActive && type === "down") return "#ef4444"; | ||
| return "rgba(255,255,255,0.65)"; |
There was a problem hiding this comment.
Severity: warning
The inline text color has low contrast against the transparent background, posing a readability risk.
Suggestions:
- Increase opacity or use solid high-contrast color for button text
| const show = (mode) => { | ||
| if (!popupVisible) { | ||
| // First show: render off-screen invisible so we can measure height | ||
| setPopupPos({ top: -1000, left: -1000 }); |
There was a problem hiding this comment.
Severity: warning
Magic off-screen coordinates risk paint artifacts or incorrect initial bounds.
Suggestions:
- Use CSS visibility: hidden or opacity: 3492 First Stacing element at -1000px
b1536ea to
6bc80af
Compare
Implement Centralized LLM Error Categorization and Action Card UIOverviewThis change introduces a centralized LLM error categorization system and maps error templates to UI action cards. It standardizes error handling across backend agent loops and improves frontend chat rendering with graceful failure messages. Technical Highlights
Impact
|
| SessionID: sessionID, | ||
| ConversationID: convID, | ||
| } | ||
| return c.JSON(http.StatusOK, resp) |
There was a problem hiding this comment.
Severity: warning
The agent loop is incorrectly returning an HTTP 200 status code for errors, when it should be returning an HTTP 500. This is a status code mismatch.
Suggestions:
- Return http.StatusInternalServerError when agent loop fails instead of http.StatusOK
| } | ||
|
|
||
| const result = await sendChatMessage(text, activeConversationId); | ||
| console.log("LIVE API RESPONSE:", result); |
There was a problem hiding this comment.
Severity: warning
There is a leftover console.log statement in the production code. Please remove this debug statement.
Suggestions:
- Remove console.log("LIVEAPIRESPONSE:", result);
| ); | ||
| })()} | ||
| <button | ||
| onClick={() => { if (msg.actionCard?.action_url) navigate(msg.actionCard.action_url); }} |
There was a problem hiding this comment.
Severity: warning
The action_url is missing scheme validation, which could lead to a potential open redirect vulnerability.
Suggestions:
- Validate action_url is relative or belongs to an allowed domain before navigating.
Centralize LLM Error Handling and Structured Action CardsOverviewThis update centralizes LLM error categorization in backend connectors to map raw SDK failures into standard UI states. It introduces structured action cards and collapsible debug logs across chat handlers and frontend components. Technical Highlights
Impact
|
| SessionID: sessionID, | ||
| ConversationID: convID, | ||
| } | ||
| return c.JSON(http.StatusOK, resp) |
There was a problem hiding this comment.
Severity: warning
The agent loop is returning an HTTP 200 status code with error text. This should be changed to return a 500 status code to accurately reflect the error.
Suggestions:
- Return http.StatusInternalServerError instead of http.StatusOK when agent loop fails
| if text == NoDataAnalyticsResponseText { | ||
| entry["suggested_questions"] = DefaultNoDataSuggestedQuestions | ||
| } else { | ||
| for _, tpl := range LLMErrorTemplates { |
There was a problem hiding this comment.
Severity: warning
There is a nested loop iterating over LLMErrorTemplates inside the history loop. This logic should be extracted into a separate helper function for better organization and readability.
Suggestions:
- Extract the template matching logic into a dedicated helper function to reduce nesting.
| return msg, history, nil, nil, nil | ||
| msg := "" | ||
| var card *ActionCard | ||
| if tpl, ok := LLMErrorTemplates[errCategory]; ok { |
There was a problem hiding this comment.
Severity: warning
The pattern for looking up LLM error templates is duplicated. This logic should be encapsulated in a shared helper function to avoid redundancy.
Suggestions:
- Create a shared helper to handle LLM error categorization, template lookup, and history entry construction.
| } | ||
|
|
||
| const result = await sendChatMessage(text, activeConversationId); | ||
| console.log("LIVE API RESPONSE:", result); |
There was a problem hiding this comment.
Severity: warning
A console.log statement has been left in the production code.
Suggestions:
- Remove debugging console.log statement before merging.
Implement Structured AI Error Handling and Action CardsOverviewThis change introduces a standardized system for categorizing AI model errors and presenting user-friendly feedback. It integrates structured action cards into chat responses. This provides clear, actionable error messages and interactive elements for users. Technical Highlights
Impact
|
| } | ||
|
|
||
| // Also check for format errors, which are hallucinations, not provider errors | ||
| if strings.Contains(text, "Model Output Error") { |
There was a problem hiding this comment.
Severity: warning
This is a magic string matching 'Model Output Error'. It is recommended to define a constant for this string.
Suggestions:
- Extract 'Model Output Error' into a shared constant.
| } | ||
|
|
||
| const result = await sendChatMessage(text, activeConversationId); | ||
| console.log("LIVE API RESPONSE:", result); |
There was a problem hiding this comment.
Severity: warning
A debug console.log statement was found in the production code.
Suggestions:
- Remove console.log statement before merging.
| ); | ||
| })()} | ||
| <button | ||
| onClick={() => { if (msg.actionCard?.action_url) navigate(msg.actionCard.action_url); }} |
There was a problem hiding this comment.
Severity: warning
The action_url is missing fallback or schema validation. This could lead to an open redirect vulnerability if the URL is untrusted.
Suggestions:
- Validate that action_url is a relative path before passing to navigate
- Ensure external URLs are blocked or handled securely
Implement Centralized LLM Error Handling and Action CardsOverviewThis update centralizes LLM error categorization, introduces structured action cards, and updates UI components to render rich error feedback. Backend modules now map provider errors into standardized templates, while frontend chat views safely display raw debug data and interactive UI actions. Technical Highlights
Impact
|
| SessionID: sessionID, | ||
| ConversationID: convID, | ||
| } | ||
| return c.JSON(http.StatusOK, resp) |
There was a problem hiding this comment.
Severity: warning
The current implementation returns a 200 OK status code even when the agent loop fails. This masks errors and should be changed to return a 500 or 502 status code to indicate a server-side issue.
Suggestions:
- Return http.StatusInternalServerError instead of http.StatusOK when agent execution fails.
| // LLMErrorTemplates stores the fallback configurations for various LLM errors. | ||
| // This allows the frontend to automatically render the correct box without | ||
| // hardcoded display logic. | ||
| var LLMErrorTemplates = map[aiconnectors.LLMErrorCategory]LLMErrorTemplate{ |
There was a problem hiding this comment.
Severity: warning
The map key aiconnectors.LLMErrorCategory is currently defined as a string type. Consider using a custom type for this key to enhance type safety and prevent potential runtime errors.
Suggestions:
- Define a distinct type for
LLMErrorCategoryif it's not already a custom type, to prevent accidental string mismatches.
| // hardcoded display logic. | ||
| var LLMErrorTemplates = map[aiconnectors.LLMErrorCategory]LLMErrorTemplate{ | ||
| aiconnectors.ErrCategoryAuth: { | ||
| Message: "> **Action Required: AI Provider Issue**\n> \n> The AI Provider's API key is invalid or the model is missing. Please configure a valid provider in settings to continue.", |
There was a problem hiding this comment.
Severity: warning
Markdown formatting, specifically the greater-than symbol ('>'), is embedded directly within the content. It is recommended to separate content from presentation by handling markdown rendering in a dedicated layer.
Suggestions:
- Store raw message text and apply Markdown formatting in the UI layer.
- If Markdown is required from backend, use a dedicated Markdown field or enum for formatting type.
| } | ||
|
|
||
| // MatchErrorTemplateByText returns the error template matching the provided response text. | ||
| func MatchErrorTemplateByText(text string) (LLMErrorTemplate, bool) { |
There was a problem hiding this comment.
Severity: warning
The MatchErrorTemplateByText function relies on strings.HasPrefix for matching error messages. This approach is brittle and could break if the prefixes of the error messages change in the future.
Suggestions:
- Prefer
LookupErrorTemplateby category if possible. - If text matching is necessary, use more robust pattern matching or a unique identifier in the text.
| { | ||
| name: "Unknown Error", | ||
| category: aiconnectors.ErrCategoryUnknown, | ||
| want: "", |
There was a problem hiding this comment.
Severity: info
The test case expects an empty string for ErrCategoryUnknown. However, LLMErrorTemplates defines a message for this category, creating a discrepancy. Please ensure the test aligns with the defined messages.
Suggestions:
- Update
tt.wantforErrCategoryUnknownto match the actual message inLLMErrorTemplates.
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| got := "" | ||
| if tpl, ok := LLMErrorTemplates[tt.category]; ok { |
There was a problem hiding this comment.
Severity: warning
The current test only validates the Message field of the ActionCard. The content of the ActionCard itself is not being validated, which could lead to unexpected behavior or display issues.
Suggestions:
- Add assertions to verify
ActionCardfields for each error template.
|
|
||
| function extractPlainText(html) { | ||
| const raw = html || ''; | ||
| if (typeof document === 'undefined') { |
There was a problem hiding this comment.
Severity: warning
The regular expression /<[^>]+>/g used for stripping HTML can be fragile. It might not correctly handle all edge cases of HTML formatting, potentially leading to incomplete stripping or unintended removal of content.
Suggestions:
- Consider a more robust HTML parsing library if complex or malformed HTML is expected.
- Add unit tests with various HTML inputs to ensure regex robustness.
| // Unified logo wrapper to ensure the 16px visual right gap and 8px visual left gap | ||
| // stay mathematically synchronized across the header, chat messages, and loading states. | ||
| const LiviLogo: React.FC<{ className?: string }> = ({ className = '' }) => ( | ||
| <div className={`w-12 flex-shrink-0 flex justify-center ${className}`}> |
There was a problem hiding this comment.
Severity: info
The refactoring of the LiviLogo component involves removing the wrapper div. It is important to ensure that this change does not introduce any unintended layout shifts or affect the overall presentation of the component.
Suggestions:
- Verify visual regression tests pass for this change.
| i = currI - 1; | ||
|
|
||
| const isError = blockquoteLines.length > 0 && blockquoteLines[0].includes('Action Required:'); | ||
| const errorPhrases = [ |
There was a problem hiding this comment.
Severity: warning
The errorPhrases list needs to be actively maintained and kept in sync with the messages defined in the backend's LLMErrorTemplates. Discrepancies can lead to incorrect error handling or display.
Suggestions:
- Consider fetching error phrases from the backend or using a shared constant to avoid duplication and sync issues.
- If backend sends
action_card, use its presence as primary error indicator, not text matching.
| i = currI - 1; | ||
|
|
||
| const isError = blockquoteLines.length > 0 && blockquoteLines[0].includes('Action Required:'); | ||
| const errorPhrases = [ |
There was a problem hiding this comment.
Severity: warning
Relying on text-based error detection is brittle and prone to breaking if error messages change. It is recommended to instead use the structured actionCard data for more robust error handling.
Suggestions:
- Check
msg.actionCardexistence to determine if it's an error message for rendering.
| parts.push( | ||
| <div key={`q-${lineIdx++}`} className="flex flex-col mb-4 mt-1 rounded-md overflow-hidden"> | ||
| <div className="border-l-2 border-red-500 bg-red-500/10 text-slate-200 pl-3 pr-3 py-2 font-bold"> | ||
| <blockquote key={`q-${lineIdx++}`} className="border-l-2 border-indigo-500 text-slate-300 pl-3 pr-3 pt-0 pb-2 rounded-r-md mb-2 mt-1"> |
There was a problem hiding this comment.
Severity: info
The styling has been changed from a div to a blockquote. Please verify that this change maintains visual consistency with the rest of the application and does not negatively impact accessibility.
Suggestions:
- Ensure
blockquotesemantic meaning is appropriate for error messages. - Verify styling matches design system and accessibility requirements.
| files: m.files && m.files.length > 0 ? m.files : undefined, | ||
| suggestedQuestions: m.suggested_questions, | ||
| debugArtifacts: m.debug_artifacts as DebugArtifacts | undefined, | ||
| actionCard: m.action_card, |
There was a problem hiding this comment.
Severity: warning
The actionCard data is being passed to the component, but it is not being rendered as an interactive card. This suggests that the functionality for displaying and interacting with the action card is missing.
Suggestions:
- Implement a component to render
actionCardwith its title, description, button, and URL. - If
actionCardis not meant to be rendered, remove it from the interface and mapping.
| return ( | ||
| <div className="h-full flex flex-col bg-slate-900 relative"> | ||
| <div className="flex-none px-4 py-2 relative z-10"> | ||
| <div className="h-full flex flex-col bg-slate-900 overflow-hidden"> |
There was a problem hiding this comment.
Severity: info
The header padding has been adjusted to accommodate the scrollbar gutter. It is important to verify that this change maintains consistent appearance and behavior across different browsers.
Suggestions:
- Test on different browsers and OS combinations to ensure consistent layout.
| </div> | ||
|
|
||
| <div className="flex-1 overflow-y-auto px-4 py-6"> | ||
| <div className="flex-1 overflow-y-auto px-4 py-6" style={{scrollbarGutter: 'stable'}}> |
There was a problem hiding this comment.
Severity: info
The scrollbarGutter: 'stable' CSS property has been added. Please check the browser support for this property and ensure that appropriate fallback behavior is in place for browsers that do not support it.
Suggestions:
- Ensure graceful degradation for browsers not supporting
scrollbar-gutter.
| ) : ( | ||
| <div key={msg.id} className="flex items-end"> | ||
| <LiviLogo className="mb-0.5 mr-2" /> | ||
| <LiviLogo className="mb-0.5 mr-4" /> |
There was a problem hiding this comment.
Severity: info
The margin for the logo has been increased from mr-2 to mr-4. Please verify that this change maintains visual spacing consistency with other elements in the interface.
Suggestions:
- Confirm this change aligns with design specifications for spacing.
| {(chart.query || chart.time_range || chart.granularity || chart.context) && ( | ||
| <details className="group mt-1"> | ||
| <summary className="text-xs text-slate-500 cursor-pointer hover:text-slate-400 select-none"> | ||
| <summary className="w-fit text-xs text-slate-500 cursor-pointer hover:text-slate-400 select-none"> |
There was a problem hiding this comment.
Severity: info
The w-fit utility class has been added to the summary element. Please verify that this change does not cause any layout issues, particularly on smaller screens where the width might become problematic.
Suggestions:
- Test responsiveness of the 'Data details' summary on various viewport sizes.
| <button | ||
| onClick={() => { | ||
| const url = msg.actionCard?.action_url; | ||
| if (url && url.startsWith('/')) { |
There was a problem hiding this comment.
Severity: warning
The action_url lacks validation for its protocol and domain. This could make it vulnerable to open redirect attacks if a malicious URL is provided.
Suggestions:
- Validate URL strictly against relative path regex or allowlist
- Ensure URL does not start with '//' to prevent protocol-relative redirects
3 participants
Summary
Describe what changed and why.
Linked Issue
Link the agreed issue this PR fulfills.
Validation
Describe the most specific test or validation you ran.
Checklist
SECURITY.mdand updated related documentation if needed.Notes For Reviewers
Anything specific you want reviewers to focus on.