Skip to content

Validate log --service, --app and --worker names against the deployment - #244

Merged
pjcdawkins merged 4 commits into
mainfrom
upsun-dispatch/CLI-215-462879bb32d42e94
Oct 15, 2026
Merged

pjcdawkins merged 4 commits into
mainfrom
upsun-dispatch/CLI-215-462879bb32d42e94

Conversation

@upsun-dispatch

@upsun-dispatch upsun-dispatch Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

With the auto log protocol, log now checks --service, --app and --worker against the environment's current deployment. An unknown name prints a warning listing the valid names, and is still queried, as logs of removed or renamed services are retained. If the name is a log type (e.g. --service access), the warning suggests passing it as an argument instead.

--worker no longer forces SSH: it filters the logs API by the worker's qualified <app>--<worker> name. A bare short name is resolved against the deployment with the same rules as ssh --worker (now in Selector::selectWorkerName()), so an ambiguous or missing short name is an error. A qualified name, or a short name with --app, is passed through (Selector::qualifyWorkerName()).

The deployment is only fetched when a service, app or worker filter is set. If there is no current deployment, names are not validated, and a short --worker name without an app is an error.

Not checked: that the logs API names worker logs <app>--<worker> (as the metrics commands do), and that router is a valid logs service name. Before merging, run log --format json on an environment with a worker and check the service field.

Changes:

  • legacy/src/Selector/Selector.php: the --worker branch of selectRemoteContainer() is split into qualifyWorkerName() and selectWorkerName(), with the same messages and prompt.
  • legacy/src/Command/Environment/EnvironmentLogCommand.php: new resolveServices(); --worker no longer triggers SSH (the help text is updated).
  • integration-tests/environment_log_test.go: adds a deployment to TestEnvironmentLogAPI, and covers worker selection, warnings for unknown names (with and without the log type hint), and short worker name errors.

Created by Upsun Dispatch™, review comments addressed by Claude Code.

🤖 Generated with Claude Code

Unknown --service/--app names were sent to the logs API unchecked and just
returned "No logs found", and --worker forced SSH. Names are now checked
against the current deployment, and workers are filtered via the API.

Requested-by: Patrick Dawkins
Dispatch-Run: 01M502FSZ9H4K1AGZWJ45ZYGCA
Dispatch-Attempt: 1

@upsun-dispatch upsun-dispatch Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Warning

Changes suggested — 🟡 1 warning · 🔵 1 minor point

🔍 Full review · 3 files reviewed

Verification
  • buildFilters() is called only once, from readFromApi() before the time window is parsed, so the worker prompt and the verbose Selected worker line come once per command, not once per page or per tail poll.
  • selectWorkerName() keeps the old worker branch unchanged: the case-insensitive conflict check, direct lookup, suffix match, the non-interactive ambiguity error and the prompt. selectRemoteContainer() still wraps the result with getWorker().
  • Selection::getAppName() already merges the --app option, the app ID from the project URL and &lt;PREFIX>APPLICATION_NAME, so resolveServices() validates the app from all three sources.
  • Api::getCurrentDeployment throws EnvironmentStateException for inactive environments, so log on an inactive environment falls back to the no-validation path and does not fail.

TestEnvironmentLogAPI in integration-tests/environment_log_test.go adds a deployment fixture and covers worker resolution, invalid service/app/worker errors and that no query is sent after an error. Nothing tests the path where there is no current deployment. The refactored SSH worker path is covered by the existing TestSSHAppSelection cases, which the author could not run locally.

Review details
  • Commit: 902c4e8
  • Model: claude-opus-5-5

Review 1 of 10 for this pull request · View the full run

Comment thread legacy/src/Command/Environment/EnvironmentLogCommand.php Outdated
Comment thread legacy/src/Command/Environment/EnvironmentLogCommand.php
Logs of removed or renamed services are retained, so a name that is not
in the current deployment now prints a warning (with the log type hint)
and is still queried, instead of failing.

Qualified worker names (<app>--<worker>, or a short name with --app) are
passed through in the same way, via the new Selector::qualifyWorkerName().
Only a bare short worker name needs the deployment to be resolved.

The current deployment is no longer fetched when there are no service,
app or worker filters.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@upsun-dispatch

upsun-dispatch Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor Author

📋 PR Summary

With the auto log protocol, log now checks the --service, --app and --worker names against the environment's current deployment. An unknown name prints a warning that lists the valid names, plus a hint when the name is a log type, and the name is still queried. --worker now filters the logs API by the qualified &lt;app>--&lt;worker> name and no longer forces SSH. Short worker names are resolved with the same shared Selector logic that ssh --worker uses. The latest push only re-wraps a long assertion line in the integration test.

Changes
Layer / File(s) Summary
Worker selection
legacy/src/Selector/Selector.php The --worker handling in selectRemoteContainer() is split out into qualifyWorkerName() and selectWorkerName(), with the same messages and prompt as before.
Log command
legacy/src/Command/Environment/EnvironmentLogCommand.php Adds resolveServices(), which checks service, app and worker names against the deployment and warns about unknown ones. --worker no longer triggers SSH, and the help text is updated.
Tests
integration-tests/environment_log_test.go Adds a deployment to TestEnvironmentLogAPI and covers worker selection, warnings for unknown names (with and without the log type hint), and errors for short worker names. The latest push only re-wraps one assertion line.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@pjcdawkins
pjcdawkins enabled auto-merge (squash) October 15, 2026 22:20
@pjcdawkins
pjcdawkins merged commit ca9d1aa into main Oct 15, 2026
7 checks passed
@pjcdawkins
pjcdawkins deleted the upsun-dispatch/CLI-215-462879bb32d42e94 branch October 15, 2026 23:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant