fix: --new-session implies --new-conversation for agent invoke#7871
fix: --new-session implies --new-conversation for agent invoke#7871Copilot wants to merge 2 commits into
Conversation
When --new-session is used, also force a new conversation because the backend derives sessions from conversation IDs. Without this, reusing an existing conversation returns the same session ID, making --new-session ineffective on its own. Fixes #16222 Agent-Logs-Url: https://github.com/Azure/azure-dev/sessions/87d2f1e7-6e8d-4fa0-92e0-ed2fb1979ee2 Co-authored-by: therealjohn <1501196+therealjohn@users.noreply.github.com>
|
What do we think the user expectation actually is here? If a user is expecting that they'd keep the same conversation on a new session, this will break that expectation. If I saw the conversation correctly, manually providing a session ID will not generate a new conversation ID, so should we be doing that? So a user would be required to pass both |
|
Hi @@copilot. Thank you for your interest in helping to improve the Azure Developer CLI experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days. |
jongio
left a comment
There was a problem hiding this comment.
The fix addresses a real problem: the backend derives sessions from conversations, so --new-session alone reuses the existing conversation and gets back the same session. The || approach makes the flag work.
Two items to resolve before merging:
-
@trangevi's design question is still open. This removes the ability to start a new session while keeping a conversation. If that's intentionally unsupported, the
--new-sessionflag help text should say so. It currently reads "Force a new session (discard saved one)" with no mention of conversations. Note: explicit--conversation <id>still overrides the force-new logic, so power users have an escape hatch. -
The
|| a.flags.newSessionlogic is duplicated at two call sites (responsesLocalandresponsesRemote). Consider resolving the implication once inRun():if a.flags.newSession { a.flags.newConversation = true }
This keeps both paths in sync and prevents drift if a third path is added.
Also: this is a draft PR with a "no-recent-activity" label. The open design question from @trangevi should be addressed before marking ready.
| convID, _ = resolveStoredID( | ||
| ctx, azdClient, agentKey, a.flags.conversation, a.flags.newConversation, "conversations", true, | ||
| ctx, azdClient, agentKey, a.flags.conversation, | ||
| a.flags.newConversation || a.flags.newSession, "conversations", true, |
There was a problem hiding this comment.
This || is also in responsesRemote. Consider resolving the implication once in Run() so both code paths stay in sync.
The backend derives sessions from conversation IDs, so
--new-sessionalone reuses the existing conversation and gets back the same session ID. This makes--new-sessionineffective unless paired with--new-conversation.Changes
invoke.go: PassnewConversation || newSessionas theforceNewparameter when resolving conversation IDs in bothresponsesRemote()andresponsesLocal()invoke_test.go: Add tests covering three flag combinations:--new-sessionalone (forces both),--new-conversationalone (session unchanged), and both together (redundant but valid)