ShawnGarello opened a new issue, #8979: URL: https://github.com/apache/texera/issues/8979
### What happened? **Root cause:** in `agent-service`, `addOperator` and `modifyOperator` change the workflow before they check `inputOperatorIds`. When a check fails, the tool returns an `[ERROR]` result but keeps the changes it already made. | Tool | Changes made first | Checked after | Left behind on error | |---|---|---|---| | `addOperator` | adds the operator ([L146](https://github.com/apache/texera/blob/6a29c59c6312da95826b269fca2a6a14006fb34a/agent-service/src/agent/tools/workflow-crud-tools.ts#L146)); links earlier ports as it goes | port index (L153–162), source exists (L165–171) | the new operator, plus links for earlier ports. Retrying with the same id fails: "already exists. Use modifyOperator" (L132–137) | | `modifyOperator` | sets properties (L261) and display name (L265); deletes every incoming link (L268–275) | port index (L279–288), source exists (L291–297) | the new properties and name, and the operator with no incoming links | These changes reach the user: they are auto-saved to the workflow (`texera-agent.ts` L470–491), and the step's `afterWorkflowContent` reloads the canvas (`agent.service.ts` L500–509). ``` Before: bad inputOperatorIds -> [ERROR] + operator added / properties changed / links deleted -> saved, on canvas After: bad inputOperatorIds -> [ERROR], workflow unchanged ``` Expected: when either tool returns an error, the workflow is exactly as it was before the call. The same code is on `release/v1.3` and `release/v1.2` (the file is identical to main). **Proposed approach** (A) Check everything first and change the workflow only after all checks pass. For `addOperator` the new operator's ports are already known from `getNewOperatorPredicate` (L139) before it is added; for `modifyOperator` the checks only need the operator's current ports. This is a reorder, with no new `WorkflowState` API. (B) Alternatively, snapshot with `getWorkflowContent()` and restore with `setWorkflowContent()` on any error or exception. This also covers exceptions partway through, but the failed edit has already emitted change events, and `setWorkflowContent()` emits none itself. @bobbai00, would you prefer A, B, or both? I'd like to take the fix once the approach is agreed. ### How to reproduce? Test-level, with `createAddOperatorTool` / `createModifyOperatorTool` on a `WorkflowState` holding a source `op0`: 1. `addOperator` `op1` (one input port) with `inputOperatorIds: {"5": ["op0"]}` → `[ERROR] Input port index 5 out of range ...`, but `op1` is in the workflow. 2. `addOperator` `op1` (two input ports) with `inputOperatorIds: {"0": ["op0"], "1": ["ghost"]}` → `[ERROR] Source operator "ghost" not found ...`, but `op1` and the link `op0 -> op1` are in the workflow. 3. With `op0 -> op1` linked, `modifyOperator` `op1` with new `properties`, a new `summary` and `inputOperatorIds: {"0": ["ghost"]}` → the same error, but `op1` has the new properties and display name and no incoming link. I have three tests in `workflow-crud-tools.spec.ts` that assert the workflow is unchanged after each error; they fail on main (log below). The existing test "returns an error when an input port index is not a number" asserts the current behaviour (links removed after a failed modify) and would change with a fix. User-facing: a user asks the agent to join two tables, and the agent passes a wrong source id for the second input. The tool reports an error, but the join stays on the canvas and in the saved workflow with only its first input linked. ### Version/Branch 1.4.0-incubating-SNAPSHOT (main) ### Commit Hash (Optional) 6a29c59c6312da95826b269fca2a6a14006fb34a ### What browsers are you seeing the problem on? _No response_ ### Relevant log output ```shell (fail) createModifyOperatorTool > leaves properties, display name and links unchanged when a new source does not exist (fail) createAddOperatorTool > leaves the workflow unchanged when an input port index is out of range (fail) createAddOperatorTool > leaves the workflow unchanged when a later port names a missing source 310 pass 3 fail Ran 313 tests across 19 files. ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
