Skip to content

fix(cli)!: enforce selection for multiple agents rather than use randomness #18427

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Jun 19, 2025
Merged

fix(cli)!: enforce selection for multiple agents rather than use randomness #18427

merged 3 commits into from
Jun 19, 2025

Conversation

mafredri
Copy link
Member

@mafredri mafredri commented Jun 18, 2025 •
edited
Loading

In the past we randomly selected workspace agent if there were multiple.
Unless both are running on the same machine with the same configuration,
this would be very confusing behavior for a user.

With the introduction of sub agents (devcontainer agents), we have now
made this an error state and require the specifying of agent when there
is more than one (either normal agent or sub agent).

This aligns with the behavior of e.g. Coder Desktop.

Fixes coder/internal#696

@mafredri mafredri force-pushed the mafredri/fix-cli-agent-selection branch from 0a47fd1 to 647181e Compare June 18, 2025 13:07
@mafredri mafredri marked this pull request as ready for review June 18, 2025 15:03
@github-actions github-actions bot added the release/breaking This label is applied to PRs to detect breaking changes as part of the release process label Jun 18, 2025
@mafredri mafredri force-pushed the mafredri/fix-cli-agent-selection branch from 1c6193a to f923edb Compare June 18, 2025 15:06
@johnstcn
Copy link
Member

TBD: Do we classify this breaking or not? IMO it could slip by as not as random selection can't really be a feature, can it? And dev containers are new and you can just not use them.

IMO if you have to ask yourself if this is a breaking change, it's best to err on the side of caution and label it as breaking (obligatory: https://xkcd.com/1172/).

@mafredri mafredri changed the title fix(cli)!: select sub agent if available and error on multiple agents fix(cli)!: enforce selection for multiple agents and select sub agent if available Jun 18, 2025
@johnstcn
Copy link
Member

@mafredri my understanding of this change is as follows:

Given: workspace foo, agent smith, no sub-agents
Then:

  • coder ssh foo maps to agent bar
  • coder ssh foo.bar maps to agent bar

Given: workspace foo, agents smith, jones
Then:

  • coder ssh foo fails
  • coder ssh foo.smith maps to agent smith
  • coder ssh foo.jones maps to agent jones

Given: workspace foo, agents smith, sub-agents johnson
Then:

  • coder ssh foo maps to agent smith
  • coder ssh foo.smith maps to agent smith
  • coder ssh foo.johnson maps to agent johnson

Given: workspace foo, agents smith, sub-agents johnson, thompson
Then:

  • coder ssh foo maps to agent smith
  • coder ssh foo.smith maps to agent smith
  • coder ssh foo.johnson maps to agent johnson
  • coder ssh foo.thompson maps to agent thompson

Given: workspace foo, agents smith, johnes, sub-agents johnson, thompson
Then:

  • coder ssh foo fails
  • coder ssh foo.smith maps to agent smith
  • coder ssh foo.jones maps to agent jones
  • coder ssh foo.johnson maps to agent johnson
  • coder ssh foo.thompson maps to agent thompson

Am I correct?

@mafredri
Copy link
Member Author

mafredri commented Jun 18, 2025 •
edited
Loading

@johnstcn Close, just two correction (as sub agents are prioritized same as in the Web UI).

Given: workspace foo, agents smith, sub-agents johnson
Then:

  • coder ssh foo maps to agent smith This maps to johnson

Given: workspace foo, agents smith, sub-agents johnson, thompson
Then:

  • coder ssh foo maps to agent smith This errors, two sub agents

@johnstcn
Copy link
Member

Given: workspace foo, agents smith, sub-agents johnson Then:

  • coder ssh foo maps to agent smith This maps to johnson

On the surface I find this mildly surprising, but given that it's predicated on the presence of a sub-agent and that it corresponds with the UI's prioritization, that seems fine to me.

johnstcn
johnstcn approved these changes Jun 18, 2025
View reviewed changes
Copy link
Member

@johnstcn johnstcn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving pending any blocking concerns from @deansheather

@mafredri mafredri changed the title fix(cli)!: enforce selection for multiple agents and select sub agent if available fix(cli)!: enforce selection for multiple agents rather than use randomness Jun 19, 2025
mafredri added 3 commits June 19, 2025 15:29
@mafredri
fix(cli)!: select sub agent if available and error on multiple agents
81d3f62 
In the past we randomly selected workspace agent if there were multiple.
Unless both are running on the same machine with the same configuration,
this would be very confusing behavior for a user.

With the introduction of sub agents (devcontainer agents), it was
decided prioritize them if present. Similarly as before, selecting a
devcontainer randomly would be confusing. We now error out if the agent
name is not specified and there are multiple agents.

Fixes coder/internal#696
@mafredri
add tests
716a2fb
@mafredri
simplify according to PR feedback
8319ea6
@mafredri mafredri force-pushed the mafredri/fix-cli-agent-selection branch from f923edb to 8319ea6 Compare June 19, 2025 15:36
@mafredri mafredri merged commit bacdc28 into main Jun 19, 2025
36 checks passed
@mafredri mafredri deleted the mafredri/fix-cli-agent-selection branch June 19, 2025 19:13
@github-actions github-actions bot locked and limited conversation to collaborators Jun 19, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@johnstcn johnstcn johnstcn approved these changes

@deansheather deansheather Awaiting requested review from deansheather

@DanielleMaywood DanielleMaywood Awaiting requested review from DanielleMaywood

Assignees

@mafredri mafredri

Labels
release/breaking This label is applied to PRs to detect breaking changes as part of the release process
Projects
None yet
Milestone
No milestone
2 participants
@mafredri @johnstcn