.NET: Fix QuestionExecutor looping after GotoAction re-entry in declarative workflows

[Peter Ibekwe (peibekwe)]

Motivation and Context

The ConfirmInput declarative workflow sample (and any workflow combining a Question action with a GotoAction loopback) entered an infinite loop on re-entry: the question would never re-prompt, so the mismatch branch would fire repeatedly without giving the user a chance to retry. Fixed the bug and re-enabled (and extended) the previously disabled integration test to capture this regression.

Contribution Checklist

• The code builds clean without any errors or warnings
• The PR follows the Contribution Guidelines
• All unit tests pass, and I have added new tests where possible
• Is this a breaking change? If yes, add "[BREAKING]" prefix to the title of the PR.

[github-actions]

Automated Code Review

Reviewers: 4 | Confidence: 88% | Result: All clear

Reviewed: Correctness, Security Reliability, Test Coverage, Design Approach

---

Automated review by peibekwe's agents

[Copilot]

Pull request overview

This PR fixes a re-entry bug in the declarative Question action that could cause infinite looping when a workflow uses GotoAction to jump back to a Question after a mismatch, and it re-enables multi-turn integration coverage to prevent regressions.

Changes:

• Fix QuestionExecutor re-entry behavior by persisting “has executed” state in the correct executor scope and adjusting SkipOnFirstExecutionIfVariableHasValue logic.
• Extend the ConfirmInput integration testcase to first mismatch (forcing the loopback path) and then match (completing the workflow), including updated expected action sequences/counts.
• Re-enable the previously skipped multi-turn integration test theory.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
dotnet/src/Microsoft.Agents.AI.Workflows.Declarative/ObjectModel/QuestionExecutor.cs	Corrects Question re-entry prompting by fixing _hasExecuted scoping and skip-mode evaluation.
dotnet/tests/Microsoft.Agents.AI.Workflows.Declarative.IntegrationTests/DeclarativeWorkflowTest.cs	Re-enables multi-turn integration test execution.
dotnet/tests/Microsoft.Agents.AI.Workflows.Declarative.IntegrationTests/Testcases/ConfirmInput.json	Updates the testcase to exercise mismatch + GotoAction loopback + successful retry, with updated validation expectations.

[github-actions]

Automated Code Review

Reviewers: 4 | Confidence: 91%

✓ Correctness

This PR fixes two cross-scope state issues in QuestionExecutor. First, it corrects the SkipOnFirstExecutionIfVariableHasValue logic from isValueUndefined && !hasExecuted to isValueUndefined || hasExecutedPreviously, which properly handles GotoAction re-entry. Second, it resolves the _promptCount cross-scope problem (noted in the prior review) by keeping all _promptCount reads/writes in the CaptureResponseAsync scope and passing the count explicitly to PromptAsync. The _hasExecuted flag is similarly moved so reads and writes happen in the same ExecuteAsync scope. DurableProperty defaults to 0 and DurableProperty defaults to false, so removing the explicit _promptCount initialization in ExecuteAsync is safe. The test changes re-enable previously-skipped multi-turn tests and add a mismatch response to exercise the GotoAction re-prompt path. The changes are logically sound and correctly address the identified bugs.

✓ Security Reliability

This PR fixes cross-scope state bugs in QuestionExecutor by moving _hasExecuted reads and writes into the same executor scope (ExecuteAsync) and _promptCount reads and writes into the Capture executor scope, passing the count explicitly to PromptAsync. The SkipOnFirstExecutionIfVariableHasValue logic is also corrected from isValueUndefined && !hasExecuted (which was broken for subsequent executions—always skipping) to isValueUndefined || hasExecutedPreviously (correctly proceding on subsequent executions). The resolved prior review comment about cross-scope _promptCount is addressed by this refactoring. Tests are enhanced with a mismatch-then-match retry path and re-enabled. No security or reliability issues found.

✓ Test Coverage

This PR fixes cross-scope state issues in QuestionExecutor by moving _hasExecuted writes to ExecuteAsync (same scope as reads) and keeping _promptCount coherent within CaptureResponseAsync. The integration tests are improved (un-skipped, ConfirmInput now exercises the GotoAction re-prompt path). However, the core logic fix — changing SkipOnFirstExecutionIfVariableHasValue from isValueUndefined && !hasExecuted to isValueUndefined || hasExecutedPreviously — is not covered by any unit test that would catch a regression. The existing unit tests (QuestionExecuteWithSkipModeAsyncWithResultDefinedAsync and …UndefinedAsync) only test first-execution scenarios where _hasExecuted defaults to false, producing identical results under both old and new logic.

✓ Design Approach

I did not find a design-level issue in this diff. The change addresses the underlying executor-scope mismatch rather than papering over the symptom: _hasExecuted is now read and written in the question action’s own scope, and prompt-count decisions are made with an explicit count passed across the prepare/capture boundary instead of depending on cross-executor durable state. The updated multi-turn tests also exercise the GotoAction re-prompt path that previously would have missed this class of bug.

Suggestions

• Add a unit test for SkipOnFirstExecutionIfVariableHasValue when _hasExecuted is already true (re-execution via GotoAction). This is the scenario the logic fix targets, but no existing unit test differentiates old from new behavior. For example, a test where the variable is defined AND the executor has previously run should expect expectPrompt: true—this would fail under the old logic (false && false = false) but pass under the new logic (false || true = true). The existing tests at QuestionExecutorTest.cs:84,100 provide no regression guard because ExecuteTestAsync (lines 293-319) creates a fresh executor each time, so _hasExecuted always starts as false.

---

Automated review by peibekwe's agents