refactor: improve analyze_issue_with_seer readability and error handling #355
Conversation
|
On it! We are reviewing the PR and will provide feedback shortly. |
PR DescriptionThis pull request enhances the Seer AI Analysis tool within the MCP server by improving code readability, maintainability, and error handling. The primary goal is to refactor the Click to see moreKey Technical ChangesThe key technical changes include: 1) Extraction of three utility functions: Architecture DecisionsThe architectural decisions involve adopting a functional approach by extracting reusable utility functions. This promotes separation of concerns and improves the overall structure of the Dependencies and InteractionsThis pull request primarily interacts with the Risk ConsiderationsThe primary risk is the potential for unexpected behavior due to changes in status handling logic. Thorough testing is required to ensure that the new utility functions correctly identify terminal and human intervention statuses. Additionally, the accuracy of the retry instructions should be verified to prevent user confusion. There is also a risk that the polling logic might not handle all possible status transitions correctly, requiring careful monitoring in production. Notable Implementation DetailsA notable implementation detail is the use of template literals to construct the retry command in the error message. This allows for dynamic generation of the command based on whether the issue is identified by URL or by organization slug and issue ID. The |
![seer-by-sentry[bot]](https://avatars.githubusercontent.com/in/801464?s=60&v=4){kind=small}
| return [ | ||
| "COMPLETED", | ||
| "FAILED", | ||
| "ERROR", | ||
| "CANCELLED", | ||
| "NEED_MORE_INFORMATION", | ||
| "WAITING_FOR_USER_RESPONSE", | ||
| ].includes(status); |
There was a problem hiding this comment.
Consider using a const assertion or enum for the terminal statuses array to prevent accidental mutations and improve type safety. This will also make it easier to maintain and reference these statuses elsewhere in the codebase.
| return [ | |
| "COMPLETED", | |
| "FAILED", | |
| "ERROR", | |
| "CANCELLED", | |
| "NEED_MORE_INFORMATION", | |
| "WAITING_FOR_USER_RESPONSE", | |
| ].includes(status); | |
| const TERMINAL_STATUSES = [ | |
| "COMPLETED", | |
| "FAILED", | |
| "ERROR", | |
| "CANCELLED", | |
| "NEED_MORE_INFORMATION", | |
| "WAITING_FOR_USER_RESPONSE", | |
| ] as const; | |
| function isTerminalStatus(status: string): boolean { | |
| return TERMINAL_STATUSES.includes(status as typeof TERMINAL_STATUSES[number]); | |
| } |
Did we get this right? 👍 / 👎 to inform future reviews.
| return ( | ||
| status === "NEED_MORE_INFORMATION" || status === "WAITING_FOR_USER_RESPONSE" | ||
| ); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
The function isHumanInterventionStatus is only checking for two specific statuses, but it might be more maintainable to define these statuses as constants and reuse them in both this function and getHumanInterventionGuidance.
| return ( | |
| status === "NEED_MORE_INFORMATION" || status === "WAITING_FOR_USER_RESPONSE" | |
| ); | |
| } | |
| /** | |
| const HUMAN_INTERVENTION_STATUSES = ["NEED_MORE_INFORMATION", "WAITING_FOR_USER_RESPONSE"] as const; | |
| function isHumanInterventionStatus(status: string): boolean { | |
| return HUMAN_INTERVENTION_STATUSES.includes(status as typeof HUMAN_INTERVENTION_STATUSES[number]); | |
| } |
Did we get this right? 👍 / 👎 to inform future reviews.
| return "\nSeer needs additional information to continue the analysis. Please review the insights above and consider providing more context.\n"; | ||
| } | ||
| if (status === "WAITING_FOR_USER_RESPONSE") { | ||
| return "\nSeer is waiting for your response to proceed. Please review the analysis and provide feedback.\n"; | ||
| } | ||
| return ""; | ||
| } | ||
|
|
||
| /** | ||
| * Creates a SentryApiService instance from server context with optional region override. | ||
| * |
There was a problem hiding this comment.
The guidance messages are hardcoded strings. Consider extracting these to constants or a configuration object to improve maintainability and make them easier to update or localize in the future.
| return "\nSeer needs additional information to continue the analysis. Please review the insights above and consider providing more context.\n"; | |
| } | |
| if (status === "WAITING_FOR_USER_RESPONSE") { | |
| return "\nSeer is waiting for your response to proceed. Please review the analysis and provide feedback.\n"; | |
| } | |
| return ""; | |
| } | |
| /** | |
| * Creates a SentryApiService instance from server context with optional region override. | |
| * | |
| const GUIDANCE_MESSAGES = { | |
| NEED_MORE_INFORMATION: "\nSeer needs additional information to continue the analysis. Please review the insights above and consider providing more context.\n", | |
| WAITING_FOR_USER_RESPONSE: "\nSeer is waiting for your response to proceed. Please review the analysis and provide feedback.\n" | |
| } as const; | |
| function getHumanInterventionGuidance(status: string): string { | |
| if (status === "NEED_MORE_INFORMATION") { | |
| return GUIDANCE_MESSAGES.NEED_MORE_INFORMATION; | |
| } | |
| if (status === "WAITING_FOR_USER_RESPONSE") { | |
| return GUIDANCE_MESSAGES.WAITING_FOR_USER_RESPONSE; | |
| } | |
| return ""; | |
| } |
Did we get this right? 👍 / 👎 to inform future reviews.
| if (isTerminalStatus(status)) { | ||
| output += `## Analysis ${getStatusDisplayName(status)}\n\n`; | ||
|
|
||
| // Add all step outputs | ||
| for (const step of currentState.autofix.steps) { | ||
| for (const step of autofixState.autofix.steps) { | ||
| output += getOutputForAutofixStep(step); | ||
| output += "\n"; | ||
| } | ||
|
|
||
| if (status !== "COMPLETED") { |
There was a problem hiding this comment.
The helper function name could be more descriptive. Consider renaming to buildRetryCommand or formatRetryInstruction to better reflect its purpose of generating retry instructions for the user.
| if (isTerminalStatus(status)) { | |
| output += `## Analysis ${getStatusDisplayName(status)}\n\n`; | |
| // Add all step outputs | |
| for (const step of currentState.autofix.steps) { | |
| for (const step of autofixState.autofix.steps) { | |
| output += getOutputForAutofixStep(step); | |
| output += "\n"; | |
| } | |
| if (status !== "COMPLETED") { | |
| // Helper function to build retry command instruction | |
| const buildRetryCommand = (params: { issueUrl?: string }, orgSlug: string, parsedIssueId: string) => { | |
| let output = `Error: Analysis state lost. Please try again by running:\n`; | |
| output += `\`\`\`\n`; | |
| output += params.issueUrl | |
| ? `analyze_issue_with_seer(issueUrl="${params.issueUrl}")` | |
| : `analyze_issue_with_seer(organizationSlug="${orgSlug}", issueId="${parsedIssueId}")`; | |
| output += `\n\`\`\`\n`; | |
| return output; | |
| }; | |
| // Then use it in the handler: | |
| if (!autofixState.autofix) { | |
| output += buildRetryCommand(params, orgSlug, parsedIssueId!); | |
| return output; | |
| } |
Did we get this right? 👍 / 👎 to inform future reviews.
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #355 +/- ##
==========================================
+ Coverage 59.37% 59.70% +0.32%
==========================================
Files 49 51 +2
Lines 6038 6159 +121
Branches 472 477 +5
==========================================
+ Hits 3585 3677 +92
- Misses 2453 2482 +29
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Extract helper functions for status checks and guidance messages - Add terminal status checking to avoid unnecessary polling - Add human intervention status detection with contextual guidance - Improve status display names with new statuses (PROCESSING, IN_PROGRESS) - Add better error recovery message when analysis state is lost - Use consistent naming (autofixState instead of currentState) - Simplify repeated terminal status checks with helper function 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
|
On it! We are reviewing the PR and will provide feedback shortly. |
PR DescriptionThis pull request enhances the Seer AI analysis tool within the MCP server by improving status handling, error messaging, and code maintainability. The primary goal is to provide users with clearer feedback on the analysis progress and to simplify the codebase for future development. Click to see moreKey Technical ChangesKey changes include: 1) Addition of Architecture DecisionsThe architectural decisions involve encapsulating status-related logic into separate, reusable functions. This promotes a more modular design, making it easier to add or modify status handling in the future without affecting the core analysis logic. The use of helper functions also improves the overall readability and maintainability of the Dependencies and InteractionsThis pull request primarily interacts with the Sentry API through the Risk ConsiderationsThe main risk is potential regressions in status handling due to the refactoring. Thorough testing is required to ensure that all status transitions are correctly handled and that the user receives accurate and timely feedback. Additionally, the new error message should be reviewed to ensure it is clear and helpful in all scenarios where the analysis state might be lost. Notable Implementation DetailsA notable implementation detail is the use of template literals to construct the retry command in the error message. This allows for dynamic generation of the command based on whether the user provided an issue URL or an organization slug and issue ID. Also, the |
| /** | ||
| * Check if an autofix status is terminal (no more updates expected) | ||
| */ | ||
| function isTerminalStatus(status: string): boolean { | ||
| return [ | ||
| "COMPLETED", | ||
| "FAILED", | ||
| "ERROR", |
There was a problem hiding this comment.
Consider making the status constants an enum or constant array to ensure consistency and type safety. This would prevent typos and make it easier to maintain the status values across the codebase.
| /** | |
| * Check if an autofix status is terminal (no more updates expected) | |
| */ | |
| function isTerminalStatus(status: string): boolean { | |
| return [ | |
| "COMPLETED", | |
| "FAILED", | |
| "ERROR", | |
| // Define status constants for better maintainability | |
| const TERMINAL_STATUSES = [ | |
| "COMPLETED", | |
| "FAILED", | |
| "ERROR", | |
| "CANCELLED", | |
| "NEED_MORE_INFORMATION", | |
| "WAITING_FOR_USER_RESPONSE", | |
| ] as const; | |
| function isTerminalStatus(status: string): boolean { | |
| return TERMINAL_STATUSES.includes(status as any); | |
| } |
Did we get this right? 👍 / 👎 to inform future reviews.
|
|
||
| /** | ||
| * Check if an autofix status requires human intervention | ||
| */ | ||
| function isHumanInterventionStatus(status: string): boolean { | ||
| return ( | ||
| status === "NEED_MORE_INFORMATION" || status === "WAITING_FOR_USER_RESPONSE" |
There was a problem hiding this comment.
The isHumanInterventionStatus function is defined but never used in the codebase. Consider removing it or adding a comment explaining its intended future use to avoid dead code.
Did we get this right? 👍 / 👎 to inform future reviews.
| * Get guidance message for human intervention states | ||
| */ | ||
| function getHumanInterventionGuidance(status: string): string { | ||
| if (status === "NEED_MORE_INFORMATION") { | ||
| return "\nSeer needs additional information to continue the analysis. Please review the insights above and consider providing more context.\n"; | ||
| } | ||
| if (status === "WAITING_FOR_USER_RESPONSE") { | ||
| return "\nSeer is waiting for your response to proceed. Please review the analysis and provide feedback.\n"; | ||
| } |
There was a problem hiding this comment.
Consider making this function more robust by handling unknown status types with a default message or warning. This would improve error handling for future status additions.
| * Get guidance message for human intervention states | |
| */ | |
| function getHumanInterventionGuidance(status: string): string { | |
| if (status === "NEED_MORE_INFORMATION") { | |
| return "\nSeer needs additional information to continue the analysis. Please review the insights above and consider providing more context.\n"; | |
| } | |
| if (status === "WAITING_FOR_USER_RESPONSE") { | |
| return "\nSeer is waiting for your response to proceed. Please review the analysis and provide feedback.\n"; | |
| } | |
| function getHumanInterventionGuidance(status: string): string { | |
| switch (status) { | |
| case "NEED_MORE_INFORMATION": | |
| return "\nSeer needs additional information to continue the analysis. Please review the insights above and consider providing more context.\n"; | |
| case "WAITING_FOR_USER_RESPONSE": | |
| return "\nSeer is waiting for your response to proceed. Please review the analysis and provide feedback.\n"; | |
| default: | |
| return ""; | |
| } | |
| } |
Did we get this right? 👍 / 👎 to inform future reviews.
| if (!autofixState.autofix) { | ||
| output += `Error: Analysis state lost. Please try again by running:\n`; | ||
| output += `\`\`\`\n`; | ||
| output += params.issueUrl | ||
| ? `analyze_issue_with_seer(issueUrl="${params.issueUrl}")` | ||
| : `analyze_issue_with_seer(organizationSlug="${orgSlug}", issueId="${parsedIssueId}")`; | ||
| output += `\n\`\`\`\n`; | ||
| return output; |
There was a problem hiding this comment.
Good improvement! This error message provides much clearer guidance to users on how to retry the operation. The conditional logic for showing the appropriate retry command is well implemented.
Did we get this right? 👍 / 👎 to inform future reviews.
| // Check if completed (terminal state) | ||
| if (isTerminalStatus(status)) { |
There was a problem hiding this comment.
Excellent refactoring! Extracting the terminal status check into a reusable function improves code maintainability and reduces duplication. The logic is much cleaner now.
Did we get this right? 👍 / 👎 to inform future reviews.
| } | ||
|
|
||
| if (status !== "COMPLETED") { |
There was a problem hiding this comment.
Good consolidation of the human intervention guidance logic. This removes code duplication and centralizes the guidance message generation.
Did we get this right? 👍 / 👎 to inform future reviews.
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| } else if (status === "WAITING_FOR_USER_RESPONSE") { | ||
| output += `\n\nSeer is waiting for your response to proceed. Please review the analysis and provide feedback.\n`; | ||
| } | ||
| output += getHumanInterventionGuidance(status); |
There was a problem hiding this comment.
The formatting of newlines appears inconsistent between implementations. The previous code at lines 927-929 used double newlines (\n\n) before the guidance message, while the new implementation relies on the single newlines included in the getHumanInterventionGuidance() function. This might result in different spacing in the output compared to the previous version. Consider either:
- Adding an additional newline before calling
getHumanInterventionGuidance(status), or - Removing the leading newline from the guidance messages in the helper function
This would ensure consistent formatting with the previous implementation.
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
Bug: Inconsistent Newline Formatting in Guidance Messages
Inconsistent newline formatting for human intervention guidance messages. The getHumanInterventionGuidance helper function returns messages with leading and trailing newlines. However, the existing analysis path adds an extra newline after the guidance, while the polling loop path does not. This results in inconsistent spacing between the two output paths and alters the original formatting behavior.
packages/mcp-server/src/tools.ts#L912-L951
sentry-mcp/packages/mcp-server/src/tools.ts
Lines 912 to 951 in 2e42853
Was this report helpful? Give feedback by reacting with 👍 or 👎
🤖 Generated with Claude Code