Skip to content

fix(operation): use all apps' operations unless --app or --worker is given - #273

Merged
pjcdawkins merged 2 commits into
mainfrom
fix-operation-run-ambiguous
Oct 26, 2026
Merged

pjcdawkins merged 2 commits into
mainfrom
fix-operation-run-ambiguous

Conversation

@pjcdawkins

@pjcdawkins pjcdawkins commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

operation:list and operation:run selected an app before reading the operations. Without --app or --worker, on an environment with several apps, they:

  • failed in non-interactive mode ("Specifying the --app or --worker is required"), although they are meant to search all apps;
  • asked for an app interactively, then ignored the answer.

They now select an app only when --app or --worker is given (selectRemoteContainer: false, then Selector::withRemoteContainer()).

That made a second bug reachable: operation:run <name> ran the last match when several apps define the operation. It now reports the conflict, as the interactive chooser already did. The message's markup (<error></error>--app</error>) is fixed.

A worker's deployment entry can repeat its app's operations. Such an operation runs on the app, or on the worker with --worker, rather than being reported as a conflict.

The runtime operation test's setup is extracted into a helper, so that a new test can use two apps.

🤖 Generated with Claude Code

@upsun-dispatch upsun-dispatch Bot left a comment

Copy link
Copy Markdown
Contributor

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

🔍 Full review · 3 files reviewed

Verification
  • Selector::getSelection with selectRemoteContainer: false still reads --app into appName. It just skips selectRemoteContainer, so the prompt and the non-interactive 'required' error no longer happen without --app/--worker.
  • withRemoteContainer passes on the selection's app name, so --app other resolves to the other app, and --worker still goes through selectWorkerName.
  • isset($serviceOperations[$operationName]) matches the old name-equality loop, because getRuntimeOperations() returns arrays keyed by operation name.
  • The helper's t.Cleanup calls replace the old deferred Close calls, so the servers outlive the test's parallel subtests as before.

TestRuntimeOperationRunAmbiguous is new: it covers listing across two apps, the ambiguous-run error with no POST sent, and the --app way out. The existing TestRuntimeOperationRun now uses the shared setup helper. The tests run in the ci.yml integration-test, integration-test-legacy-auth and integration-test-cross jobs. None of them covers workers or interactive mode.

Review details
  • Commit: 605febb
  • Model: claude-opus-5-5

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

Comment thread legacy/src/Command/RuntimeOperation/RunCommand.php
@upsun-dispatch

Copy link
Copy Markdown
Contributor

📋 PR Summary

operation:list and operation:run now pick an app only when --app or --worker is given. Without either option they search the operations of every app, instead of failing in non-interactive mode or asking for an app and then ignoring the answer. When several apps define the same operation, operation:run &lt;name> now reports the conflict instead of running the last match. Operations that a worker repeats from its parent app are ignored, so by default they run on the app.

Changes
Layer / File(s) Summary
Runtime operation commands
legacy/src/Command/RuntimeOperation/ListCommand.php Reads operations without picking an app first, and picks one only when --app or --worker is given.
legacy/src/Command/RuntimeOperation/RunCommand.php Picks an app only on request and reports a conflict when the operation name is ambiguous, with the markup in that message fixed. A new withoutWorkerCopies helper drops operations that workers repeat from their parent app.
Tests
integration-tests/runtime_operation_test.go Moves test setup into a helper that takes several app names, where a name containing -- is a worker of the app named before it. Adds a test checking that a worker's repeated operation runs on the app by default and on the worker when --worker is given.

pjcdawkins and others added 2 commits October 26, 2026 08:39
…given

The operation commands selected an app before reading the operations, so
without --app or --worker they failed in non-interactive mode when an
environment had several apps, and otherwise asked for an app and ignored the
answer. They now select an app only when one of those options is given.

operation:run now also reports an operation defined on more than one app when
it is given by name, as it does when choosing one interactively, instead of
running the last one found. The message's markup is fixed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A worker's deployment entry can repeat its app's operations. Without --app or
--worker, such an operation was reported as defined on more than one app.
It now runs on the app, and on the worker with --worker.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@pjcdawkins
pjcdawkins force-pushed the fix-operation-run-ambiguous branch from 269117b to 7dd25c4 Compare October 26, 2026 08:40
@pjcdawkins
pjcdawkins merged commit af567ae into main Oct 26, 2026
11 checks passed
@pjcdawkins
pjcdawkins deleted the fix-operation-run-ambiguous branch October 26, 2026 10:05
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