fix: Remove enforce auto end dialog for skill bots

[ltwlf]

Description

Stops enforcing the runtime to auto end the root dialog when the bot is consumed as a skill. This is not necessary, because you can set the auto end dialog property for the root dialog in the designer. The default is true. Removing this restrictions gives the author more flexibility. There are cases where you need this control, see #4157.

Task Item

fixes #4157

[coveralls]

Coverage decreased (-0.009%) to 54.723% when pulling 807b9df on ltwlf:ltwlf/skill-end-dialog into e4c4f36 on microsoft:main.

[boydc2014]

Looks good to me, @luhan2017 @benbrown @cwhitten what do you guys think?

[luhan2017]

Looks good to me, @luhan2017 @benbrown @cwhitten what do you guys think?

There are two user scenarios for the skill dialog ends:

1. The skill dialog ends automatically when there is no more actions available in the stack.
2. The skill dialog ends when the parent bot send it a signal.

For the 1st scenario, we need to set autoEndDialog=true when the incoming message is skill mode, but it will be problematic if the skill bot is served as a normal bot.
For the 2nd scenario, we don't need to set autoEndDialog=true, user need to have the logic in the parent bot side which is exactly what @ltwlf want.

Just removing this logic might cause issue if user want the 1st scenario.
We probably could introduce another setting autoEndDialogForSkill to distinguish the above two scenarios?

@carlosscastro who is looking at the isSkill setting, I think isSkill setting is not needed, we can add autoEndDialogForSkill to decide whether we want to enable the logic in composer runtime.

Like this.

 if (autoEndDialogForSkill && turnContext.TurnState.Get<IIdentity>(BotAdapter.BotIdentityKey) is ClaimsIdentity claimIdentity && SkillValidation.IsSkillClaim(claimIdentity.Claims))
            {
                rootDialog.AutoEndDialog = true;
            }

@ltwlf , will the autoEndDialogForSkill setting work for you?

[carlosscastro]

Thanks @luhan2017 for putting so much thought into this, you've hit a number of very valid points and I agree that we could use a bit more consideration here.

I'd love @tomlm, @EricDahlvang, @gabog and @Stevenic to weigh in on @luhan2017's comment above. I think the setting is a detail, but we want to be very careful about what the right thing for AutoEndDialog is. This is an interesting interaction between the skills protocol and adaptive dialogs.

@ltwlf thanks for starting this discussion and providing your input! -- I know you have a real need around this so we'll keep that in focus, I just want to take a step back and make a good decision while enabling your scenario.

[EricDahlvang]

We might be able to make this configurable, but i don't think we can just remove it.

[tomlm]

This change is the correct change.  The runtime should NOT be hardcoding this property, and the property can be bound by the user to a setting which is bound to whether something is a skill or not.

[ltwlf]

This change is the correct change.  The runtime should NOT be hardcoding this property, and the property can be bound by the user to a setting which is bound to whether something is a skill or not.

The author can set this via the AutoEndDialog property at the root dialog (default is true).

[luhan2017]

Let's merge this PR, and meanwhile, I've created a follow up item in the doc side to explain how to set AutoEndDialog value based on the scenarios.
https://github.com/MicrosoftDocs/composer-docs-pr/issues/257 @zxyanliu

[gabog]

As long as the default is true (like I think it is) and this is explained in the docs, I'm OK with this change.