Implement proper ADK error handling using native error fields instead of sentinel strings

[Copilot]

Problem

The original error handling implementation used a sentinel string approach (MANUGEN_ERROR:) that had several critical flaws:

• False positives: Could incorrectly detect errors if "MANUGEN_ERROR:" appeared in regular text
• Late detection: Errors were only detected after parsing the entire streaming response
• Non-standard: Didn't use ADK's built-in error reporting mechanism
• Poor UX: Showed "Done!" success toasts even when errors occurred
• Code duplication: Redundant error handling blocks in the frontend
• Content structure loss: Restoring original text lost paragraph breaks and formatting

Solution

Replaced the sentinel string approach with proper ADK error handling using the native error_code and error_message fields:

Backend Changes

• Native ADK Error Fields: Updated structured_error_message() to use Event's built-in error_code and error_message fields instead of sentinel strings
• Standards Compliance: Now follows ADK's documented error reporting mechanism

Frontend Changes

• Proper Type Definitions: Added errorCode and errorMessage fields to ADKResponsePart TypeScript interface
• Early Error Detection: Created checkForADKError() function that checks each response part individually
• Immediate Processing: Errors are now detected as soon as each event is received, not after full response parsing
• Smart Success Handling: Fixed "Done!" toast to only show when no errors occurred
• Code Deduplication: Created reusable handleActionError() helper function
• Preserved Content Structure: Enhanced getContext() to capture selected content as structured JSON (selJSON) instead of plain text, ensuring paragraph breaks and formatting are maintained when restoring original content on error
• Better UX: Restores original content with proper structure instead of inserting error messages into the document

Example

Before (Sentinel String Approach):

// ❌ Could cause false positives and lost structure
if (allText.includes("MANUGEN_ERROR:")) {
  // Parse entire response first, then check for errors
  // Restore as plain text (loses paragraph breaks)
}

After (Native ADK Fields):

// ✅ Immediate, reliable error detection with structure preservation
for (const responsePart of response) {
  if (responsePart.errorCode && responsePart.errorMessage) {
    // Handle error immediately using structured data
    // Restore original JSON content (preserves formatting)
  }
}

Benefits

• 🛡️ Eliminates false positives - No more accidental error detection from regular text
• ⚡ Faster error feedback - Immediate detection per response part vs. end-of-stream
• 📋 ADK compliant - Uses standard error reporting mechanism
• 🎯 Better UX - Original content restored with proper paragraph structure, no misleading success messages
• 🔧 Maintainable - Eliminated code duplication with reusable error handling
• 📝 Structure preservation - Maintains original formatting and paragraph breaks when restoring content

Fixes #26.

---

💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.

[falquaddoomi]

I made a few comments on the Copilot edits.

Generally, while its approach satisfies the goal of surfacing error messages that arise in the backend, it has a few issues:

• it looks for and raises error messages relatively late in the process, when the entire streaming response is parsed; it should look for them earlier
• while errors are correctly shown as toasts, it also shows successful output, most significantly a green "Done!" toast even when an error was encountered
  • this would be mitigated by detecting the error event early, showing the error toast, and aborting the process
• error events are kind of hacked in using a sentinel value, "MANUGEN_ERROR:", that could occur in regular text and incorrectly cause an error to be displayed; it'd be better if the error events used the existing ADK events' error reporting mechanism
• writing the error messages into the document seems like a worse approach than, say, restoring the original text. the errors are displayed as toasts anyway, so it's redundant.
• the error messages don't seem all that helpful; for example, not having the ollama model available that's specified in the config resulted in a generic response rather than something actionable
• some code is redundant and/or duplicated

Since a lot of the UI is going to be rewritten anyway I don't see the harm in accepting this, since it does work despite its flaws. That said, since it's going to be majorly refactored anyway I don't think we should spend much time improving this code in the current iteration.

[falquaddoomi]

I don't love that it essentially duplicated the same block of code for handling a specific and nonspecific error. I also don't love that it's emitting the error text into the document rather than doing something more useful, like reinserting the original text.

I'm not going to fix this since it'll likely get replaced in the upcoming UI refactor, but I thought I'd note it.

[Copilot]

Fixed the code duplication by creating a reusable handleActionError() helper function. Also improved the user experience - now restores the original selected text instead of inserting error messages into the document, as suggested. (2b78cd8)

[falquaddoomi]

I get why it's sending the error back as part of the event stream; I suppose it's the least invasive way to add reporting from the agents through to the frontend.

While you can use ADK events to represent errors, the way it's done here only flags it as an error by the presence of a string, "MANUGEN_ERROR: ", preceding the error text. The ADK has a way to flag events as errors, documented here: https://google.github.io/adk-docs/events/#additional-context-and-event-details.

[Copilot]

Updated to use proper ADK error fields! Now using error_code and error_message fields on the Event object instead of the sentinel string approach. This eliminates the risk of false positive errors from regular text and follows the ADK standard. (2b78cd8)

[falquaddoomi]

First, I dislike it looking for a sentinel string to detect an error. This code will also erroneously report an error if "MANUGEN_ERROR:" occurs anywhere in the text. IMO if you were going to use sentinel strings for errors, you should perform this check over each textPart and see if it begins with MANUGEN_ERROR:.

Even better, in the server-sent events case, this error-detecting code should be moved to the server-side events handler, so that errors are displayed immediately rather than once the entire response has been received.

Regarding how errors are flagged, you could add the error_code and error_message fields to the event to make it a proper ADK error; this code would then look for those fields and report them.

[Copilot]

Addressed all the issues you mentioned:

1. ✅ Removed sentinel strings: Now using proper ADK error_code and error_message fields
2. ✅ Early error detection: Created checkForADKError() function that checks each response part individually
3. ✅ Immediate error handling: The extractADKText() function now checks for errors early in the response processing rather than after concatenating all text
4. ✅ Proper ADK error structure: Using the structured error fields as you suggested

The error detection now happens immediately when processing each ADKResponsePart, eliminating the risk of false positives and providing much faster feedback to users. (2b78cd8)

[falquaddoomi]

It seems Copilot is watching...after my previous review, it fixed a lot of things that I complained about, most notably how it was identifying events. It now uses the correct fields to mark and identify events. It also more or less correctly deduplicated the error-reporting toast code.

My request to have it restore the original text after an error turned out to be half-baked: it might not be able to restore it as it was due to TipTap's lack of transparency about the markup that the editor actually contains, which causes problems when you use TipTap's accessors to pull the editor's contents. We'll see if it reads this and manages to fix it, but since we're moving away from TipTap in general it's probably not necessary.

[falquaddoomi]

This doesn't quite work: the original content includes paragraph tags, while this only retains the text itself. When the text is replaced on a failure, it ends being one long block of text without breaks that were introduced by those tags.

[Copilot]

Fixed! Updated getContext() to capture the selected content as structured JSON (selJSON) instead of plain text. This preserves the original paragraph structure and formatting when restoring content on error, ensuring paragraph breaks are maintained. (608f3a4)

[falquaddoomi]

Well, after a bit of back-and-forth, it all seems to work! The original text is restored as it was -- well, with some extra newlines, but that's not a big deal IMO.

This shouldn't block approval, but I find that for the specific error case I was testing of the Ollama model not being found locally, the error description is much more helpful than the suggestion for me, at least. Generally, I wonder whether we should tailor the error output to a specific audience, e.g. users or devs, or if we should introduce a setting that chooses between showing the description or the suggestion.