Surface errors in invoke actions

[phoebesimon]

Previously it would just fail opaquely with no additional info in the task log

Summary by CodeRabbit

• Bug Fixes
  • Improved handling of failed action invocations: error messages from failures are now captured and logged, and tasks are completed with the extracted error to provide clearer diagnostics and more reliable failure reporting.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Adds error handling to ActionInvokeTaskHandler: when an invoked action returns BATON_ACTION_STATUS_FAILED, the code extracts an "error" field from the response, logs it, and finishes the task with that error. The fmt package was added for error formatting.

Changes

Cohort / File(s)	Summary
Error handling for failed actions pkg/tasks/c1api/actions.go	Detects BATON_ACTION_STATUS_FAILED, extracts response.GetResponse().GetFields()["error"] when present, logs the message, and finishes the task with a formatted error. Added fmt import.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 I poked the action, then it sighed,
An error hid where fields reside.
I logged the whisper, clear and neat,
Finished the task — no skip, no cheat.
🥕

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Surface errors in invoke actions' clearly and concisely describes the main change: adding error surfacing to invoke action invocations.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@pkg/tasks/c1api/actions.go`:
- Around line 142-148: The error extraction logic when resp.GetStatus() ==
v2.BatonActionStatus_BATON_ACTION_STATUS_FAILED can overwrite the default error
with an empty string because GetStringValue() returns "" for non-string kinds;
update the guard in the block that checks resp.GetResponse() and
resp.GetResponse().GetFields() to verify the "error" field exists, is non-nil,
and is of string kind (or has a non-empty string) before assigning errMsg from
resp.GetResponse().GetFields()["error"].GetStringValue(); if the field is not a
string or is empty, leave errMsg as the default (or fall back to a generic
formatted value) so you never return a blank error. Ensure you reference
resp.GetResponse(), resp.GetResponse().GetFields(), and the "error" field when
adding these checks.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Guard against non-string/empty error fields so you don’t return a blank error.

GetStringValue() returns "" for non-string kinds, which overrides the default and can yield an empty error message. Also handle potential nil values defensively.

💡 Suggested fix

-		if errField, ok := resp.GetResponse().GetFields()["error"]; ok {
-			errMsg = errField.GetStringValue()
-		}
+		if errField, ok := resp.GetResponse().GetFields()["error"]; ok && errField != nil {
+			if s := errField.GetStringValue(); s != "" {
+				errMsg = s
+			} else {
+				errMsg = errField.String()
+			}
+		}

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	// Check if the action itself failed and propagate the error
	if resp.GetStatus() == v2.BatonActionStatus_BATON_ACTION_STATUS_FAILED {
	errMsg := "action failed"
	if resp.GetResponse() != nil && resp.GetResponse().GetFields() != nil {
	if errField, ok := resp.GetResponse().GetFields()["error"]; ok {
	errMsg = errField.GetStringValue()
	}
	// Check if the action itself failed and propagate the error
	if resp.GetStatus() == v2.BatonActionStatus_BATON_ACTION_STATUS_FAILED {
	errMsg := "action failed"
	if resp.GetResponse() != nil && resp.GetResponse().GetFields() != nil {
	if errField, ok := resp.GetResponse().GetFields()["error"]; ok && errField != nil {
	if s := errField.GetStringValue(); s != "" {
	errMsg = s
	} else {
	errMsg = errField.String()
	}
	}

🤖 Prompt for AI Agents

In `@pkg/tasks/c1api/actions.go` around lines 142 - 148, The error extraction
logic when resp.GetStatus() == v2.BatonActionStatus_BATON_ACTION_STATUS_FAILED
can overwrite the default error with an empty string because GetStringValue()
returns "" for non-string kinds; update the guard in the block that checks
resp.GetResponse() and resp.GetResponse().GetFields() to verify the "error"
field exists, is non-nil, and is of string kind (or has a non-empty string)
before assigning errMsg from
resp.GetResponse().GetFields()["error"].GetStringValue(); if the field is not a
string or is empty, leave errMsg as the default (or fall back to a generic
formatted value) so you never return a blank error. Ensure you reference
resp.GetResponse(), resp.GetResponse().GetFields(), and the "error" field when
adding these checks.

[gontzess]

are we ok with zap.any in the sdk? i guess we already do it in the lines right below this 🤷

[phoebesimon]

@gontzess I was questioning that too until I saw the line below here, so maybe? In any case, I changed this to be more explicit here: bba8770

[gontzess]

looks good!

[gontzess]

lgtm