From 38ab2875a43196ac8909bb6d602d08029a0ac3fc Mon Sep 17 00:00:00 2001 From: ElshadHu Date: Tue, 18 Nov 2025 16:23:31 -0500 Subject: [PATCH 1/5] fix: Check status code instead of result --- src/core/Action.ts | 34 +++- test/actionSendgoal.test.ts | 318 ++++++++++++++++++++++++++++++++++++ 2 files changed, 346 insertions(+), 6 deletions(-) create mode 100644 test/actionSendgoal.test.ts diff --git a/src/core/Action.ts b/src/core/Action.ts index f6d01871e..2383f0a15 100644 --- a/src/core/Action.ts +++ b/src/core/Action.ts @@ -69,19 +69,41 @@ export default class Action< } const actionGoalId = `send_action_goal:${this.name}:${uuidv4()}`; - - this.ros.on(actionGoalId, function (message) { + this.ros.on(actionGoalId, (message) => { if (isRosbridgeActionResultMessage(message)) { - if (!message.result) { - failedCallback(message.values ?? ""); + const status = message.status as GoalStatus; + + // Check status code instead of result field to properly handle STATUS_CANCELED + if (status === GoalStatus.STATUS_SUCCEEDED) { + resultCallback(message.values as TResult); } else { - resultCallback(message.values); + const baseError = + typeof message.values === "string" ? message.values : ""; + + let errorMessage: string; + switch (status) { + case GoalStatus.STATUS_CANCELED: + errorMessage = `Action was canceled${baseError ? `: ${baseError}` : ""}`; + break; + case GoalStatus.STATUS_ABORTED: + errorMessage = `Action was aborted${baseError ? `: ${baseError}` : ""}`; + break; + case GoalStatus.STATUS_CANCELING: + errorMessage = `Action is canceling${baseError ? `: ${baseError}` : ""}`; + break; + case GoalStatus.STATUS_UNKNOWN: + errorMessage = `Action status unknown${baseError ? `: ${baseError}` : ""}`; + break; + default: + errorMessage = `Action failed with status ${String(status)}${baseError ? `: ${baseError}` : ""}`; + } + + failedCallback(errorMessage); } } else if (isRosbridgeActionFeedbackMessage(message)) { feedbackCallback?.(message.values); } }); - this.ros.callOnConnection({ op: "send_action_goal", id: actionGoalId, diff --git a/test/actionSendgoal.test.ts b/test/actionSendgoal.test.ts new file mode 100644 index 000000000..7a9aa26dc --- /dev/null +++ b/test/actionSendgoal.test.ts @@ -0,0 +1,318 @@ +/** + * @fileOverview + * Test for Action.sendGoal status code handling + * Tests that sendGoal properly handles STATUS_CANCELED and other status codes + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import Action from "../src/core/Action.ts"; +import { GoalStatus } from "../src/core/GoalStatus.ts"; +import type Ros from "../src/core/Ros.ts"; + +// Strict types matching the real protocol +interface ActionResultMessageBase { + op: "action_result"; + id: string; + action: string; + status: number; +} + +interface FailedActionResultMessage extends ActionResultMessageBase { + result: false; + values?: string; +} + +interface SuccessfulActionResultMessage extends ActionResultMessageBase { + result: true; + values: { result: number }; +} + +type ActionResultMessage = + | FailedActionResultMessage + | SuccessfulActionResultMessage; + +interface ActionFeedbackMessage { + op: "action_feedback"; + id: string; + action: string; + values: { current: number }; +} + +type ActionMessage = ActionResultMessage | ActionFeedbackMessage; + +describe("Action.sendGoal", () => { + let action: Action< + { target: number }, + { current: number }, + { result: number } + >; + let mockRos: Ros; + let messageHandler: ((msg: ActionMessage) => void) | null = null; + + beforeEach(() => { + messageHandler = null; + mockRos = { + on: vi.fn((_id: string, callback: (msg: ActionMessage) => void) => { + messageHandler = callback; + }), + callOnConnection: vi.fn(), + } as unknown as Ros; + + action = new Action({ + ros: mockRos, + name: "/test_action", + actionType: "test_msgs/TestAction", + }); + }); + + afterEach(() => { + vi.clearAllMocks(); + }); + + describe("status code handling", () => { + it("should call resultCallback when action succeeds (STATUS_SUCCEEDED)", () => { + const resultCallback = vi.fn(); + const failedCallback = vi.fn(); + + action.sendGoal( + { target: 10 }, + resultCallback, + undefined, + failedCallback, + ); + + const successMessage: SuccessfulActionResultMessage = { + op: "action_result", + id: "test-id", + action: "/test_action", + values: { result: 42 }, + status: GoalStatus.STATUS_SUCCEEDED, + result: true, + }; + + messageHandler?.(successMessage); + + expect(resultCallback).toHaveBeenCalledWith({ result: 42 }); + expect(failedCallback).not.toHaveBeenCalled(); + }); + + it("should call failedCallback when action is cancelled (STATUS_CANCELED)", () => { + const resultCallback = vi.fn(); + const failedCallback = vi.fn(); + + action.sendGoal( + { target: 10 }, + resultCallback, + undefined, + failedCallback, + ); + + const cancelledMessage: FailedActionResultMessage = { + op: "action_result", + id: "test-id", + action: "/test_action", + values: "Action was cancelled by user", + status: GoalStatus.STATUS_CANCELED, + result: false, + }; + + messageHandler?.(cancelledMessage); + + expect(failedCallback).toHaveBeenCalled(); + expect(resultCallback).not.toHaveBeenCalled(); + }); + + it("should call failedCallback when action is aborted (STATUS_ABORTED)", () => { + const resultCallback = vi.fn(); + const failedCallback = vi.fn(); + + action.sendGoal( + { target: 10 }, + resultCallback, + undefined, + failedCallback, + ); + + const abortedMessage: FailedActionResultMessage = { + op: "action_result", + id: "test-id", + action: "/test_action", + values: "Action aborted due to error", + status: GoalStatus.STATUS_ABORTED, + result: false, + }; + + messageHandler?.(abortedMessage); + + expect(failedCallback).toHaveBeenCalled(); + expect(resultCallback).not.toHaveBeenCalled(); + }); + + it("should call failedCallback when action is canceling (STATUS_CANCELING)", () => { + const resultCallback = vi.fn(); + const failedCallback = vi.fn(); + + action.sendGoal( + { target: 10 }, + resultCallback, + undefined, + failedCallback, + ); + + const cancelingMessage: FailedActionResultMessage = { + op: "action_result", + id: "test-id", + action: "/test_action", + values: "Action is being cancelled", + status: GoalStatus.STATUS_CANCELING, + result: false, + }; + + messageHandler?.(cancelingMessage); + + expect(failedCallback).toHaveBeenCalled(); + expect(resultCallback).not.toHaveBeenCalled(); + }); + + it("should handle unknown status gracefully (STATUS_UNKNOWN)", () => { + const resultCallback = vi.fn(); + const failedCallback = vi.fn(); + + action.sendGoal( + { target: 10 }, + resultCallback, + undefined, + failedCallback, + ); + + const unknownMessage: FailedActionResultMessage = { + op: "action_result", + id: "test-id", + action: "/test_action", + values: "Unknown status", + status: GoalStatus.STATUS_UNKNOWN, + result: false, + }; + + messageHandler?.(unknownMessage); + + expect(failedCallback).toHaveBeenCalled(); + expect(resultCallback).not.toHaveBeenCalled(); + }); + }); + + describe("feedback handling", () => { + it("should handle feedback messages correctly", () => { + const resultCallback = vi.fn(); + const feedbackCallback = vi.fn(); + const failedCallback = vi.fn(); + + action.sendGoal( + { target: 10 }, + resultCallback, + feedbackCallback, + failedCallback, + ); + + const feedbackMessage: ActionFeedbackMessage = { + op: "action_feedback", + id: "test-id", + action: "/test_action", + values: { current: 5 }, + }; + + messageHandler?.(feedbackMessage); + + expect(feedbackCallback).toHaveBeenCalledWith({ current: 5 }); + expect(resultCallback).not.toHaveBeenCalled(); + expect(failedCallback).not.toHaveBeenCalled(); + }); + + it("should handle multiple feedback messages", () => { + const resultCallback = vi.fn(); + const feedbackCallback = vi.fn(); + const failedCallback = vi.fn(); + + action.sendGoal( + { target: 10 }, + resultCallback, + feedbackCallback, + failedCallback, + ); + + const feedbackMessage1: ActionFeedbackMessage = { + op: "action_feedback", + id: "test-id", + action: "/test_action", + values: { current: 3 }, + }; + + const feedbackMessage2: ActionFeedbackMessage = { + op: "action_feedback", + id: "test-id", + action: "/test_action", + values: { current: 7 }, + }; + + messageHandler?.(feedbackMessage1); + messageHandler?.(feedbackMessage2); + + expect(feedbackCallback).toHaveBeenCalledTimes(2); + expect(feedbackCallback).toHaveBeenNthCalledWith(1, { current: 3 }); + expect(feedbackCallback).toHaveBeenNthCalledWith(2, { current: 7 }); + }); + }); + + describe("edge cases", () => { + it("should prioritize status code over result field", () => { + const resultCallback = vi.fn(); + const failedCallback = vi.fn(); + + action.sendGoal( + { target: 10 }, + resultCallback, + undefined, + failedCallback, + ); + + // This is the problematic case: result=true but status=CANCELED + // According to protocol, this shouldn't happen, but we test it anyway + // We have to cast to test this edge case since TypeScript prevents it + const confusingMessage = { + op: "action_result", + id: "test-id", + action: "/test_action", + values: { result: 0 }, + status: GoalStatus.STATUS_CANCELED, + result: true, // This violates the type but could happen at runtime + } as ActionMessage; + + messageHandler?.(confusingMessage); + + // Should call failedCallback because status is CANCELED + expect(failedCallback).toHaveBeenCalled(); + expect(resultCallback).not.toHaveBeenCalled(); + }); + + it("should handle missing feedback callback gracefully", () => { + const resultCallback = vi.fn(); + const failedCallback = vi.fn(); + + action.sendGoal( + { target: 10 }, + resultCallback, + undefined, + failedCallback, + ); + + const feedbackMessage: ActionFeedbackMessage = { + op: "action_feedback", + id: "test-id", + action: "/test_action", + values: { current: 5 }, + }; + + expect(() => messageHandler?.(feedbackMessage)).not.toThrow(); + }); + }); +}); From 8f956873fc1d9e7dc41f4cd62905bf565dc08ac0 Mon Sep 17 00:00:00 2001 From: ElshadHu Date: Wed, 19 Nov 2025 14:27:55 -0500 Subject: [PATCH 2/5] fix: validate both status and result fields --- src/core/Action.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/Action.ts b/src/core/Action.ts index 2383f0a15..37f2d34e2 100644 --- a/src/core/Action.ts +++ b/src/core/Action.ts @@ -74,8 +74,8 @@ export default class Action< const status = message.status as GoalStatus; // Check status code instead of result field to properly handle STATUS_CANCELED - if (status === GoalStatus.STATUS_SUCCEEDED) { - resultCallback(message.values as TResult); + if (status === GoalStatus.STATUS_SUCCEEDED && message.result) { + resultCallback(message.values); } else { const baseError = typeof message.values === "string" ? message.values : ""; From a4f9725b23fcf53d00ecac20c2de1bf47f1964d3 Mon Sep 17 00:00:00 2001 From: ElshadHu Date: Wed, 19 Nov 2025 15:34:06 -0500 Subject: [PATCH 3/5] fix: add custom error class --- src/core/Action.ts | 54 ++++++++++++++++++++++++++-------------------- 1 file changed, 31 insertions(+), 23 deletions(-) diff --git a/src/core/Action.ts b/src/core/Action.ts index 37f2d34e2..625d5191f 100644 --- a/src/core/Action.ts +++ b/src/core/Action.ts @@ -14,6 +14,34 @@ import { import type Ros from "./Ros.ts"; import { v4 as uuidv4 } from "uuid"; +export class GoalError extends Error { + constructor(status: GoalStatus | number | string, baseError?: unknown) { + const base = typeof baseError === "string" ? baseError : ""; + let message: string; + + switch (status) { + case GoalStatus.STATUS_CANCELED: + message = `Action was canceled${base ? `: ${base}` : ""}`; + break; + case GoalStatus.STATUS_ABORTED: + message = `Action was aborted${base ? `: ${base}` : ""}`; + break; + case GoalStatus.STATUS_CANCELING: + message = `Action is canceling${base ? `: ${base}` : ""}`; + break; + case GoalStatus.STATUS_UNKNOWN: + message = `Action status unknown${base ? `: ${base}` : ""}`; + break; + default: + message = `Action failed with status ${String(status)}${base ? `: ${base}` : ""}`; + } + + super(message); + this.name = "GoalError"; + Object.setPrototypeOf(this, GoalError.prototype); + } +} + /** * A ROS 2 action client. */ @@ -73,32 +101,12 @@ export default class Action< if (isRosbridgeActionResultMessage(message)) { const status = message.status as GoalStatus; - // Check status code instead of result field to properly handle STATUS_CANCELED + // Validates both status and result if (status === GoalStatus.STATUS_SUCCEEDED && message.result) { resultCallback(message.values); } else { - const baseError = - typeof message.values === "string" ? message.values : ""; - - let errorMessage: string; - switch (status) { - case GoalStatus.STATUS_CANCELED: - errorMessage = `Action was canceled${baseError ? `: ${baseError}` : ""}`; - break; - case GoalStatus.STATUS_ABORTED: - errorMessage = `Action was aborted${baseError ? `: ${baseError}` : ""}`; - break; - case GoalStatus.STATUS_CANCELING: - errorMessage = `Action is canceling${baseError ? `: ${baseError}` : ""}`; - break; - case GoalStatus.STATUS_UNKNOWN: - errorMessage = `Action status unknown${baseError ? `: ${baseError}` : ""}`; - break; - default: - errorMessage = `Action failed with status ${String(status)}${baseError ? `: ${baseError}` : ""}`; - } - - failedCallback(errorMessage); + const errorMessage: GoalError = new GoalError(status, message.values); + failedCallback(String(errorMessage)); } } else if (isRosbridgeActionFeedbackMessage(message)) { feedbackCallback?.(message.values); From 7cc71ac492eda0e2fe264cc7076fce7b3b845131 Mon Sep 17 00:00:00 2001 From: Ezra Brooks Date: Wed, 19 Nov 2025 13:37:28 -0700 Subject: [PATCH 4/5] Revert "fix: add custom error class" This reverts commit a4f9725b23fcf53d00ecac20c2de1bf47f1964d3. --- src/core/Action.ts | 54 ++++++++++++++++++++-------------------------- 1 file changed, 23 insertions(+), 31 deletions(-) diff --git a/src/core/Action.ts b/src/core/Action.ts index 625d5191f..37f2d34e2 100644 --- a/src/core/Action.ts +++ b/src/core/Action.ts @@ -14,34 +14,6 @@ import { import type Ros from "./Ros.ts"; import { v4 as uuidv4 } from "uuid"; -export class GoalError extends Error { - constructor(status: GoalStatus | number | string, baseError?: unknown) { - const base = typeof baseError === "string" ? baseError : ""; - let message: string; - - switch (status) { - case GoalStatus.STATUS_CANCELED: - message = `Action was canceled${base ? `: ${base}` : ""}`; - break; - case GoalStatus.STATUS_ABORTED: - message = `Action was aborted${base ? `: ${base}` : ""}`; - break; - case GoalStatus.STATUS_CANCELING: - message = `Action is canceling${base ? `: ${base}` : ""}`; - break; - case GoalStatus.STATUS_UNKNOWN: - message = `Action status unknown${base ? `: ${base}` : ""}`; - break; - default: - message = `Action failed with status ${String(status)}${base ? `: ${base}` : ""}`; - } - - super(message); - this.name = "GoalError"; - Object.setPrototypeOf(this, GoalError.prototype); - } -} - /** * A ROS 2 action client. */ @@ -101,12 +73,32 @@ export default class Action< if (isRosbridgeActionResultMessage(message)) { const status = message.status as GoalStatus; - // Validates both status and result + // Check status code instead of result field to properly handle STATUS_CANCELED if (status === GoalStatus.STATUS_SUCCEEDED && message.result) { resultCallback(message.values); } else { - const errorMessage: GoalError = new GoalError(status, message.values); - failedCallback(String(errorMessage)); + const baseError = + typeof message.values === "string" ? message.values : ""; + + let errorMessage: string; + switch (status) { + case GoalStatus.STATUS_CANCELED: + errorMessage = `Action was canceled${baseError ? `: ${baseError}` : ""}`; + break; + case GoalStatus.STATUS_ABORTED: + errorMessage = `Action was aborted${baseError ? `: ${baseError}` : ""}`; + break; + case GoalStatus.STATUS_CANCELING: + errorMessage = `Action is canceling${baseError ? `: ${baseError}` : ""}`; + break; + case GoalStatus.STATUS_UNKNOWN: + errorMessage = `Action status unknown${baseError ? `: ${baseError}` : ""}`; + break; + default: + errorMessage = `Action failed with status ${String(status)}${baseError ? `: ${baseError}` : ""}`; + } + + failedCallback(errorMessage); } } else if (isRosbridgeActionFeedbackMessage(message)) { feedbackCallback?.(message.values); From 183de9fc4a1bc9823376d72e9dc40ce50323b737 Mon Sep 17 00:00:00 2001 From: Ezra Brooks Date: Wed, 19 Nov 2025 13:47:38 -0700 Subject: [PATCH 5/5] invert condition to restore type narrowing, extract error templating --- src/core/Action.ts | 55 +++++++++++++++++++++++++--------------------- 1 file changed, 30 insertions(+), 25 deletions(-) diff --git a/src/core/Action.ts b/src/core/Action.ts index 37f2d34e2..3681dfcd6 100644 --- a/src/core/Action.ts +++ b/src/core/Action.ts @@ -14,6 +14,28 @@ import { import type Ros from "./Ros.ts"; import { v4 as uuidv4 } from "uuid"; +class GoalError extends Error { + override name = "GoalError"; + constructor(status: GoalStatus, errorValue?: string) { + super(`${makeErrorMessage(status)}${errorValue ? `: ${errorValue}` : ""}`); + } +} + +function makeErrorMessage(status: GoalStatus) { + switch (status) { + case GoalStatus.STATUS_CANCELED: + return `Action was canceled`; + case GoalStatus.STATUS_ABORTED: + return `Action was aborted`; + case GoalStatus.STATUS_CANCELING: + return `Action is canceling`; + case GoalStatus.STATUS_UNKNOWN: + return `Action status unknown`; + default: + return `Action failed with status ${String(status)}`; + } +} + /** * A ROS 2 action client. */ @@ -73,32 +95,15 @@ export default class Action< if (isRosbridgeActionResultMessage(message)) { const status = message.status as GoalStatus; - // Check status code instead of result field to properly handle STATUS_CANCELED - if (status === GoalStatus.STATUS_SUCCEEDED && message.result) { - resultCallback(message.values); + if (!message.result) { + failedCallback(String(new GoalError(status, message.values))); + } else if (status !== GoalStatus.STATUS_SUCCEEDED) { + failedCallback( + String(new GoalError(status, JSON.stringify(message.values))), + ); + // Check status code instead of result field to properly handle STATUS_CANCELED } else { - const baseError = - typeof message.values === "string" ? message.values : ""; - - let errorMessage: string; - switch (status) { - case GoalStatus.STATUS_CANCELED: - errorMessage = `Action was canceled${baseError ? `: ${baseError}` : ""}`; - break; - case GoalStatus.STATUS_ABORTED: - errorMessage = `Action was aborted${baseError ? `: ${baseError}` : ""}`; - break; - case GoalStatus.STATUS_CANCELING: - errorMessage = `Action is canceling${baseError ? `: ${baseError}` : ""}`; - break; - case GoalStatus.STATUS_UNKNOWN: - errorMessage = `Action status unknown${baseError ? `: ${baseError}` : ""}`; - break; - default: - errorMessage = `Action failed with status ${String(status)}${baseError ? `: ${baseError}` : ""}`; - } - - failedCallback(errorMessage); + resultCallback(message.values); } } else if (isRosbridgeActionFeedbackMessage(message)) { feedbackCallback?.(message.values);