Introduce task container selector - #113
Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for targeting running task containers (ephemeral, activity-backed) via a new --task selector, enabling ssh, scp, and log to connect to a specific task run (optionally disambiguated via --activity).
Changes:
- Introduces
Selector::addTaskOption()and task-container resolution inSelector::selectRemoteContainer()based on in-progress/pendingenvironment.taskactivities. - Adds a new
RemoteContainer\Taskimplementation that derives an SSH URL for a task run. - Wires the new options into
environment:ssh,environment:scp, andenvironment:logscommands (plus an example inssh).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| legacy/src/Selector/Selector.php | Adds --task/--activity options and implements selection logic for task-run containers. |
| legacy/src/Model/RemoteContainer/Task.php | New remote container type for SSH access to an in-progress task run. |
| legacy/src/Command/Environment/EnvironmentSshCommand.php | Exposes --task/--activity on ssh and adds a usage example. |
| legacy/src/Command/Environment/EnvironmentScpCommand.php | Exposes --task/--activity on scp. |
| legacy/src/Command/Environment/EnvironmentLogCommand.php | Exposes --task/--activity on log. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private function matchTaskActivity(array $running, string $taskName, string $activityId): Activity | ||
| { | ||
| $matches = array_values(array_filter( | ||
| $running, | ||
| fn(Activity $activity): bool => $activity->id === $activityId, | ||
| )); | ||
| if (count($matches) === 1) { | ||
| return $matches[0]; | ||
| } | ||
| $ids = implode(', ', array_map(fn(Activity $activity): string => $activity->id, $running)); | ||
| throw new InvalidArgumentException(sprintf( | ||
| 'No running instance of the task "%s" matches the activity ID "%s". Running activity IDs: %s', | ||
| $taskName, | ||
| $activityId, | ||
| $ids, | ||
| )); | ||
| } |
| $choices[$activity->id] = $activity->created_at !== '' | ||
| ? sprintf('%s (started %s)', $activity->id, $activity->created_at) | ||
| : $activity->id; |
pjcdawkins
left a comment
There was a problem hiding this comment.
Reviewed. The structure fits the existing patterns well — Model\RemoteContainer\Task implementing RemoteContainerInterface, and correctly short-circuiting in the selector before the deployment loads. All four checks pass; the branch is behind main but not conflicted.
To be clear about scope: the container naming rule (<task>--task--<first 8 chars of activity ID>) and the parameters['task'] key are taken as correct here. Nothing else in legacy/src references environment.task parameters, so this review could not corroborate them from the repository — it is a coverage observation, not a claim that they are wrong. What follows is about robustness when the surrounding state is not the happy path.
Pending activities
Selector.php:902 queries [Activity::STATE_IN_PROGRESS, Activity::STATE_PENDING], which contradicts the invariant documented in this PR at Task.php:61-62 — that the container exists only while the activity is in progress. Running upsun ssh --task mytask immediately after task:run, while the activity is still pending, makes it the single match, so the fast path at :915-916 selects it and builds an SSH URL for a container that does not exist yet.
In practice SshDiagnostics::noServiceAccessMessage() does surface a usable message, so this degrades rather than failing raw — which is why it is worth fixing for wording rather than treating as urgent.
Worth noting the obvious fix is not the right one: simply dropping STATE_PENDING from the query routes the case to noRunningTaskMessage(), which would then report the queued run as "finished <created_at>" — worse than the current behavior. Better to keep pending in the query, reject a pending selection explicitly ("the task is queued, activity <id>; SSH is possible once it starts"), and include the state in the choice labels.
Other robustness items
Selector.php:963—matchTaskActivity()'s docblock promises "exact, or a unique prefix", but the implementation compares full IDs only. The practical cost is that the 8-character prefix a user can read off the container name is rejected. Since activity IDs are unique,count($matches) > 1is unreachable, so the shared error path only ever means "no match". Either implement the documented prefix matching or correct the docblock and message; prefix matching is the more useful of the two given the container suffix is exactly those 8 characters.Selector.php:939— the chooser labelscreated_atas "started" and omits the state. For a pending activitycreated_atis queue time, not start time.Model/Activity.php:20-28andActivityMonitor.php:677-680establish the convention of preferringstarted_atwith a fallback.Selector.php:986—noRunningTaskMessage()fetches only the 10 most recentenvironment.taskactivities and filters by name client-side, so on a busy environment it can report that a task has never run when it ran an hour ago. Also printscreated_atprefixed with "finished" whencompleted_atis empty.Selector.php:739—--tasksilently overrides--app,--workerand--instance.upsun ssh --task mytask --app someappreturns early at:742and ignores--appwithout a word, whereas the existing app/worker path deliberately errors on conflict at:745-752.--instance 1 --task xfails with a misleading "Instance not found: 1".
Tests
Task::constructTaskSshUrl()/referenceSshUrl() is pure and there is clear precedent for testing it — legacy/tests/Service/DrushServiceTest.php:40 and legacy/tests/Command/User/UserAddCommandTest.php:41 both build an Environment from a data array with _links. A few lines there would pin the naming rule in code, which is the most durable place for it given it is currently only stated in a comment.
Suggest reviewing after #112 merges, so the task surface is settled first.
Review by Claude Code.
Add task container selector for commands
ssh,scpandlog. Proposes to choose from a task runs list if several are executed in parallel.activityID can be provided to specify exact task run: