Fix: Check status code in Action.sendGoal instead of result field

[ElshadHu]

Public API Changes

Description

I fixed a bug in Action.sendGoal where the method was checking the result field instead of the status field to determine action success/failure. This caused STATUS_CANCELED and other status codes to be handled incorrectly.

After reading the documentation, I realized that action result messages contain two fields:

• result : A simplified success/failure indicator
• status : The authoritative status

I updated the sendGoal method in src/core/Action.ts to check message.status against GoalStatus.STATUS_SUCCEEDED as the primary indicator of success. For all other status values, I route to the failure callback with descriptive error messages based on the specific status code.

// Check status code instead of result field
const status = message.status as GoalStatus;

if (status === GoalStatus.STATUS_SUCCEEDED) {
  resultCallback(message.values as TResult);
} else {
  // Handle failures with appropriate error messages
  failedCallback(errorMessage);
}

Testing

I added comprehensive unit tests in test/actionSendgoal.test.ts covering:

• GoalStatus values (SUCCEEDED, CANCELED, ABORTED, CANCELING, UNKNOWN)
• Feedback message handling
• Edge case where result and status fields contradict each other

References

• Rosbridge Protocol - Action Result
• ROS 2 GoalStatus

If there is any issue, please let me know.

Fixes #889

[EzraBrooks]

@sea-bass since you're more familiar with the protocol than I am, can you give this a functional review pass before I give it a typescript review pass?

[sea-bass]

Added functionality looks good, but I think we probably still want to leave in a case to catch where message.result is false, implying something may have gone wrong in the backend implementation?

[ElshadHu]

Added functionality looks good, but I think we probably still want to leave in a case to catch where message.result is false, implying something may have gone wrong in the backend implementation?

Thanks for the feedback, in that case do you want me to handle message.result ? I mean, basically combining the way it is in the codebase and my functionality ?

[sea-bass]

LGTM -- over to you, @EzraBrooks

[EzraBrooks]

LGTM with one future proofing nit

[EzraBrooks]

What if we made a class GoalError extends Error that did this template interpolation in its constructor and then we called String() on the error before sending it to the failedCallback? That way in the future if the error handling here changes to include a throw, we already have an error type for it.

[ElshadHu]

Is it okay to put GoalError on top of Action.ts ? To be honest i did not find any file that contains custom error classes.

[EzraBrooks]

Yep, that's fine!

[EzraBrooks]

@ElshadHu do you mind if I push a commit to get it a bit closer to what I intended to guide you to, so we don't have to go back and forth?

[ElshadHu]

@ElshadHu do you mind if I push a commit to get it a bit closer to what I intended to guide you to, so we don't have to go back and forth?

Sure

[EzraBrooks]

Thanks! if my change looks good to you (and I didn't break the tests 😆) I'm happy to merge this now.

[ElshadHu]

Thanks! if my change looks good to you (and I didn't break the tests 😆) I'm happy to merge this now.

Thanks for the help, It seems perfect now.

[EzraBrooks]

jazzy and kilted are broken (#1104) so we're just looking for humble here.

[EzraBrooks]

Thanks for the contribution!

[ElshadHu]

Thanks for the contribution!

It is my pleasure to contribute to such a nice repo.