Introduce task container selector

[vitolkachova]

Add task container selector for commands ssh, scp and log. Proposes to choose from a task runs list if several are executed in parallel.

% upsun --task myagent -p 45rxrkidodaay -e main
Enter a number to choose which run of "myagent" to connect to:
  [0] 45tkoe3kh5w7w (started 2026-06-23T12:23:17.427775+00:00)
  [1] r4urjsfdqjfsa (started 2026-06-23T12:23:16.095779+00:00)

activity ID can be provided to specify exact task run:

upsun log --task myagent -p 45rxrkidodaay -e main --activity 45tkoe3kh5w7w

[Copilot]

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 in Selector::selectRemoteContainer() based on in-progress/pending environment.task activities.
• Adds a new RemoteContainer\Task implementation that derives an SSH URL for a task run.
• Wires the new options into environment:ssh, environment:scp, and environment:logs commands (plus an example in ssh).

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.

[pjcdawkins]

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) > 1 is 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 labels created_at as "started" and omits the state. For a pending activity created_at is queue time, not start time. Model/Activity.php:20-28 and ActivityMonitor.php:677-680 establish the convention of preferring started_at with a fallback.
• Selector.php:986 — noRunningTaskMessage() fetches only the 10 most recent environment.task activities 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 prints created_at prefixed with "finished" when completed_at is empty.
• Selector.php:739 — --task silently overrides --app, --worker and --instance. upsun ssh --task mytask --app someapp returns early at :742 and ignores --app without a word, whereas the existing app/worker path deliberately errors on conflict at :745-752. --instance 1 --task x fails 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.

[upsun-dispatch]

Note

Reviewed — No new blocking findings · 5 minor points · 1 still open

🔍 Full review · 5 files reviewed

🔵 Minor points

Not blocking, and no threads opened for these.

• legacy/src/Selector/Selector.php:999 — noRunningTaskMessage() fetches only the 10 most recent environment.task activities across all task names and then filters by the requested name. When 10 or more activities for other tasks have run more recently than this task's last run, the filtered $recent is empty and the message wrongly states 'No task named "%s" has run on the environment', even though it has run.
• legacy/src/Selector/Selector.php:1000 — The task-name filter closure fn(Activity $activity): bool => ($activity->parameters['task'] ?? null) === $taskName is duplicated verbatim here and in selectTaskContainer() at line 903. If the way a task is identified from an activity ever changes (a different parameter key), both copies must be edited together or the 'no running task' message filters differently from the selection logic.
• legacy/src/Selector/Selector.php:742 — When --task is set, selectRemoteContainer() returns early via selectTaskContainer(), so any co-supplied --app, --worker, or --instance is silently ignored rather than reported as conflicting. Symmetrically, --activity is only read inside selectTaskContainer(), so passing --activity <id> without --task has no effect and no warning.
• legacy/src/Selector/Selector.php:939 — The label guard tests $activity->created_at !== '', which only excludes the empty string. If created_at is null (property absent/unset on the activity resource) the strict comparison passes and the code reaches sprintf('%s (started %s)', $id, null), printing ' (started )' and emitting a PHP deprecation for passing null to a string placeholder. Using !empty($activity->created_at) would cover both cases.
• legacy/src/Selector/Selector.php:899 — The new task-selection logic (selectTaskContainer, prefix matching in matchTaskActivity, chooseTaskActivity, and Task's <name>--task--<id8> container name / -- prefix-splitting URL construction) ships without unit tests, so regressions in the activity-filtering and URL-building rules would go undetected. The repo prefers table-driven tests for this kind of logic.

Review details

• Commit: 212400f
• Model: claude-opus-4-8
• Panel: security · correctness · robustness · design

Outstanding from earlier reviews:

• #3735947661 — legacy/src/Selector/Selector.php:902: Connecting to a not-yet-started task container fails with a cryptic error instead of a clear message.

[upsun-dispatch]

Note

Reviewed — No new blocking findings · 1 minor point · 1 still open

🔁 Incremental · 1 file reviewed

🔵 Minor points

Not blocking, and no threads opened for these.

• legacy/src/Selector/Selector.php:1012 — The guard $last->completed_at !== '' no longer handles a null value the way the removed $last->completed_at ?: $last->created_at did. The platformsh client exposes completed_at as null (not '') for an activity that never completed — e.g. a cancelled run, or a run still in progress at message-build time (this recent list is fetched without a state filter, separate from the $running query). Since null !== '' is true, execution enters the ', finished ' . $last->completed_at branch and prints an empty date, so the message reads "...(last run: activity , finished ). SSH is only possible..."; the created_at fallback is never reached.

Review details

• Commits: 212400f..2258821
• Model: claude-opus-4-8
• Panel: correctness · robustness · design

Outstanding from earlier reviews:

• #3735947661 — legacy/src/Selector/Selector.php:908: Connecting to a not-yet-started task container fails with a cryptic error instead of a clear message.