[ShawnGarello]

What changes were proposed in this PR?

When the agent calls modifyOperator with a source operator that doesn't exist, the tool reports an error, but the operator's properties and name have already changed and its input links are gone. addOperator has the same problem: a bad port index or source leaves the new operator (and links for earlier ports) in the workflow, and a retry with the same id then fails with "already exists".

Both tools now run every input check (port index, source operator) first, through one small helper, resolveInputLinks, and only then add the operator, apply properties and name, or replace links. A failed call leaves the workflow as it was. Error messages and success results are unchanged.

Why this approach (option A in #8979) and not snapshot/restore (B): it's a reorder with no new WorkflowState API, and a failed call now emits no change events, so nothing half-applied reaches the canvas or the 500 ms auto-persist. B would restore with setWorkflowContent, which emits no events, after the failed edit had already emitted some. I'm happy to switch to B if reviewers prefer it.

The existing test "returns an error when an input port index is not a number" (from #7026) asserted the half-applied result: the failed modify had dropped the operator's input link. It now asserts that the link is kept and the operator isn't renamed.

After review, both tools also reject an operator that names itself as a source (modifyOperator used to store a self-loop link), and an input port key must be all digits (parseInt read "1abc" as port 1).

Out of scope: the model isn't told that the call failed (isError is always false for these results). I'll open a separate issue for that.

Any related issues, documentation, discussions?

Closes #8979

Backport: the bug is on both release branches (workflow-crud-tools.ts is identical on release/v1.3 and release/v1.2), so I'd suggest both. On release/v1.2 the spec file workflow-crud-tools.spec.ts doesn't exist yet, so that cherry-pick will need the test changes dropped or adapted.

How was this PR tested?

In agent-service, after bun install --frozen-lockfile:

1. Red: the first commit adds three tests that assert the workflow is unchanged after an [ERROR] result (one for modifyOperator, two for addOperator). Without the fix, bun test gives 310 pass, 3 fail: exactly those three.
2. Green: with the fix, bun test gives 313 pass, 0 fail (run twice).
3. With the fix reverted and the tests kept, exactly 4 fail: the three new tests and the updated test(agent-service): extend workflow-crud-tools and add a logger spec #7026 test.
4. bun run format:check and bun run typecheck are clean. bun test --coverage: all new lines in workflow-crud-tools.ts are covered (229/230 lines; the one uncovered line is the existing catch in deleteOperator).
5. Review round (bee292e): the two checks above, plus 9 more tests (11 runs): self-references and non-digit port keys in both tools, a modify that fails on a later port, no change events after a failed call, a two-port add, and a retry after a failed add. bun test gives 324 pass, 0 fail. With only the two new checks reverted, exactly their 6 tests fail; with main's workflow-crud-tools.ts, 14 fail.

Was this PR authored or co-authored using generative AI tooling?

Generated-by: Claude Code (Opus 5.5)

[github-actions]

Automated Reviewer Suggestions

Based on the git blame history of the changed files, we recommend the following reviewers:

• Contributors with relevant context: @aglinxinyuan, @bobbai00
  You can notify them by mentioning @aglinxinyuan, @bobbai00 in a comment.

[github-actions]

Backport auto-label report

This fix: PR was checked against each actively-supported release branch. A release/* label nominates a backport target; the branch's release manager approving this PR is what sends the fix there. The required Backport Approvals check stays red until every label below is approved, so each manager either approves or removes their own label — which is why the labels left on a merged PR are exactly the branches it reached.

Release branch	Analysis
✅ release/v1.3	Already labeled — this fix is queued to backport here. @mengw15 decides: approving sends the fix here, removing this label declines it. The merge waits on one or the other.
✅ release/v1.2	Already labeled — this fix is queued to backport here. @xuang7 decides: approving sends the fix here, removing this label declines it. The merge waits on one or the other.

Auto-label run.

[ShawnGarello]

/request-review @aglinxinyuan

[bobbai00]

🟡 0 must-fix · 2 advisory · 0 polish: a clean root-cause fix with good red-first tests; two optional follow-ups in the new helper.

Correctness (2)

• workflow-crud-tools.ts:85: add and modify now handle a self-referencing source differently (advisory, see inline)
• workflow-crud-tools.ts:73: parseInt maps keys like "1abc" to port 1 (advisory, see inline)

Verification trace

Every [ERROR] return in both tools now comes before the first WorkflowState write (addOperator: line 194 before 196; modifyOperator: line 282 before 288), and resolveInputLinks only reads, so a failed call emits nothing to getWorkflowChangedStream. Error strings, link order and link-ID order match main. Locally in agent-service, main's workflow-crud-tools.ts with this spec fails exactly the 4 changed or new tests; this head passes all 313, and typecheck and format are clean.

[bobbai00]

Thanks for the PR! Please check my comments. Please also add more test cases

[ShawnGarello]

@bobbai00 Thanks for the review! Both suggestions are applied in bee292e, and I added tests: self-references and non-digit port keys in both tools, a modify that fails on a later port, no change events after a failed call, a two-port add, and a retry after a failed add.

[ShawnGarello]

/request-review @bobbai00

[bobbai00]

The changes LGTM now. I want to get review from the release manager of 1.2 and 1.3. Please decide if this PR should be backport. CC: @mengw15 @xuang7

[ShawnGarello]

@mengw15 @xuang7, following up on @bobbai00's note above: could you decide on the release/v1.3 and release/v1.2 labels when you have a chance?

[mengw15]

Dropping the backport labels — not a regression: workflow-crud-tools.ts has a single commit in its history (#4540), so the mutate-then-validate order is the original one, and #7026 later asserted the half-applied result as expected behaviour. Still worth having on main.

[ShawnGarello]

Updated the branch now that #9100 fixed CI. @bobbai00, could you approve the workflow runs when you have a chance?