[4.x] Worker QoL#1147
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds encrypted environment-variable storage and APIs for Sites and Workers, validation/merge logic, supervisor formatting and site-scoped restarts, a WorkerEnvDialog UI, migration/backfill, worker status sync and restart orchestration, and comprehensive tests across models, controllers, services, and frontend. ChangesWorker Environment Management
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (4)
tests/Feature/WorkerEnvironmentMigrationTest.php (1)
73-77: ⚡ Quick winMake migration resolution deterministic in
runMigration().Line 73 + Line 76 currently rely on the first
glob()match. If more than one file matches, this test can run the wrong migration and produce misleading results. Prefer asserting a single match (or selecting by exact basename) beforerequire.Proposed fix
private function runMigration(): void { $paths = glob(database_path('migrations/*_add_worker_environment_support.php')) ?: []; - $this->assertNotEmpty($paths, 'Worker environment migration not found.'); + sort($paths); + $this->assertCount(1, $paths, 'Expected exactly one worker environment migration file.'); $migration = require $paths[0]; $migration->up(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Feature/WorkerEnvironmentMigrationTest.php` around lines 73 - 77, The test currently grabs migrations with $paths = glob(database_path('migrations/*_add_worker_environment_support.php')) and requires $paths[0], which is non-deterministic if multiple files match; update the test in WorkerEnvironmentMigrationTest (the runMigration() flow) to deterministically resolve the migration by either asserting exactly one match with $this->assertCount(1, $paths) before requiring, or locate the exact basename (e.g. array_filter by basename === 'YYYY_add_worker_environment_support.php') and then require that chosen path instead of blindly using $paths[0], then call up() on the resolved migration.Source: Linters/SAST tools
app/Models/Worker.php (1)
57-57: ⚡ Quick winUse the model secret cast format required by the repo standard.
Line 57 uses
'encrypted:array', but this field stores sensitive environment values and should follow the project’s'encrypted:json'convention for secret-bearing model attributes.Suggested change
- 'environment' => 'encrypted:array', + 'environment' => 'encrypted:json',As per coding guidelines, “Use
'encrypted:json'cast on model properties that store credentials or secrets.” Based on learnings, the same model-secret cast rule is explicitly reinforced forapp/Models/**/*.php.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Models/Worker.php` at line 57, The model cast for the sensitive "environment" attribute in the Worker model is using 'encrypted:array' but must follow the repo secret convention; update the cast for the 'environment' property in app/Models/Worker.php (class Worker) from 'encrypted:array' to 'encrypted:json' so secrets/credentials are stored with the JSON secret cast.Sources: Coding guidelines, Learnings
app/Models/Site.php (1)
126-126: ⚡ Quick winAlign
worker_environmentcast with the repository’s secret-storage standard.Line 126 should use the secret-field cast convention used by this codebase for credentials/secrets.
Suggested change
- 'worker_environment' => 'encrypted:array', + 'worker_environment' => 'encrypted:json',As per coding guidelines, “Use
'encrypted:json'cast on model properties that store credentials or secrets.” Based on learnings, this same rule is explicitly tracked for model files.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Models/Site.php` at line 126, The Site model's cast for the worker_environment attribute uses 'encrypted:array' but the repository standard requires secret fields to use the 'encrypted:json' cast; update the Site model's $casts entry for 'worker_environment' to 'encrypted:json' (locate the $casts array in the Site class where 'worker_environment' is defined) so the field follows the secret-storage convention used across the codebase.Sources: Coding guidelines, Learnings
app/Jobs/Worker/RestartAllJob.php (1)
43-43: ⚡ Quick winAlign
failed()signature with the jobs convention.Use
Exceptioninfailed()to match the project’s job contract and keep failure handlers consistent.As per coding guidelines,
app/Jobs/**/*.phpshould implementfailed(Exception $e)for error handling/status updates.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Jobs/Worker/RestartAllJob.php` at line 43, Change the RestartAllJob::failed method signature from failed(Throwable $e): void to failed(Exception $e): void to match the project's job contract; update the type hint and any usages inside failed() accordingly (ensure imports/use statements reference Exception if necessary) so the method implements the expected failed(Exception $e) handler for app/Jobs.Sources: Coding guidelines, Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Actions/Worker/RestartAllWorkers.php`:
- Around line 15-25: The worker status updates in RestartAllWorkers (the block
calling $server->workers()->when(...)->whereNotIn(...)->get()->each(...)) must
be wrapped in a database transaction to avoid partial state if a save fails;
change the logic to perform the fetch and the per-Worker status/error mutation
inside a DB::transaction (or use an atomic bulk update on the query) so all
matching rows are set to WorkerStatus::RESTARTING with error=null together, then
after the transaction dispatch the RestartAllJob to the 'ssh' queue; ensure you
reference WorkerStatus::RESTARTING, the same query used
($server->workers()->when(...)->whereNotIn(...)) and dispatch(new
RestartAllJob($server, $site)) when making the change.
In `@app/Actions/Worker/UpdateWorkerEnvironment.php`:
- Around line 13-18: The PHPDoc for the update(Worker $worker, array $input):
WorkerEnvironmentUpdateResult method currently only documents `@throws` SSHError
but Validator::make(...)->validate() can throw ValidationException; update the
PHPDoc block for the update method to include `@throws` ValidationException (or
the fully-qualified Illuminate\Validation\ValidationException) alongside
SSHError so the annotations accurately reflect possible exceptions thrown by
update().
In `@app/Jobs/Worker/RestartAllJob.php`:
- Line 32: The unique lock key passed to run(...) only uses
"server-{$this->server->id}", which can collide for different site-scoped
restart jobs; update the key to include the site scope (e.g., append the site
identifier such as $this->site->id or $this->site->uuid) so each site/server
combo is unique. Locate the run(...) call in RestartAllJob.php (the line with
$this->run("server-{$this->server->id}", ...)) and modify the string to include
the site identifier (using $this->site or the appropriate site property) so the
lock distinguishes site-scoped jobs from each other.
In `@app/Policies/WorkerPolicy.php`:
- Around line 39-44: manage() accepts a $site but never verifies it belongs to
the given $server, allowing scoped operations against mismatched Site objects;
update the method to also check that when $site is provided it is tied to the
same server (e.g. add a condition like $site === null || $site->server_id ===
$server->id or $site->server->id === $server->id) in the return expression
alongside the existing hasWriteAccess($user, $server->project),
$server->isReady() and $server->processManager() checks so the Site-to-Server
boundary is enforced.
In `@app/Services/ProcessManager/Supervisor.php`:
- Around line 228-248: The parsing loop in Supervisor.php silently ignores lines
it cannot parse (using $output and building $statuses) which leads to empty maps
for non-empty, unparsable output; update the method to detect when $output is
non-empty but no valid status entries were produced (e.g., $output !== '' and
empty($statuses) after the loop) and fail fast by throwing an exception
(RuntimeException or the existing ProcessManagerException) with a clear message
including the raw $output so callers can handle command/parsing failures instead
of treating them as “Process not found”; implement this check immediately after
the loop that iterates explode("\n", $output) and before returning $statuses.
In `@database/migrations/2026_06_06_120237_add_worker_environment_support.php`:
- Around line 50-52: The down() migration always drops the
sites.worker_environment column which can delete preexisting data; update down()
to mirror up() by checking existence before dropping (e.g., use
Schema::hasColumn('sites', 'worker_environment') or the same conditional used in
up()) and only call $table->dropColumn('worker_environment') when that check
passes so rollback doesn't remove unrelated columns.
In `@public/api-docs/openapi/workers.yaml`:
- Around line 145-160: The OpenAPI schema for environment.value is declared as a
non-null string but the backend permits null; update the environment.*.value
property in public/api-docs/openapi/workers.yaml (the "value" property inside
the environment variable object) to allow null by adding nullable: true (or
change the type to allow "null", e.g., type: [ "string", "null" ]) while keeping
the property listed in required if the field must be present; ensure the
description reflects that null is allowed.
In `@resources/js/pages/workers/components/env-dialog.tsx`:
- Around line 157-167: The Save button is still enabled when fetching
environment variables fails (query.isError), allowing submission of an empty
`variables` payload and accidentally clearing existing variables; update the
Save button (the Button that triggers the save handler — e.g.,
handleSave/onSave) to be disabled when query.isError or when the fetch hasn't
succeeded (use !query.isSuccess || query.isFetching || query.isError), and add a
guard in the save handler (handleSave/onSave) to no-op or show an error if
`variables` is empty or the query is not successful to prevent sending an empty
payload.
In `@tests/Feature/API/WorkersTest.php`:
- Around line 472-474: The test currently hardcodes the foreign key when
creating the Server fixture (Server::factory()->create(['user_id' => 1])), which
makes the test order-dependent; change it to reference an explicit user instance
instead—either use $this->user->id if the test sets up $this->user, or create a
User via User::factory()->create() and pass that user's id into
Server::factory()->create(['user_id' => $user->id']) so the fixture is
deterministic.
---
Nitpick comments:
In `@app/Jobs/Worker/RestartAllJob.php`:
- Line 43: Change the RestartAllJob::failed method signature from
failed(Throwable $e): void to failed(Exception $e): void to match the project's
job contract; update the type hint and any usages inside failed() accordingly
(ensure imports/use statements reference Exception if necessary) so the method
implements the expected failed(Exception $e) handler for app/Jobs.
In `@app/Models/Site.php`:
- Line 126: The Site model's cast for the worker_environment attribute uses
'encrypted:array' but the repository standard requires secret fields to use the
'encrypted:json' cast; update the Site model's $casts entry for
'worker_environment' to 'encrypted:json' (locate the $casts array in the Site
class where 'worker_environment' is defined) so the field follows the
secret-storage convention used across the codebase.
In `@app/Models/Worker.php`:
- Line 57: The model cast for the sensitive "environment" attribute in the
Worker model is using 'encrypted:array' but must follow the repo secret
convention; update the cast for the 'environment' property in
app/Models/Worker.php (class Worker) from 'encrypted:array' to 'encrypted:json'
so secrets/credentials are stored with the JSON secret cast.
In `@tests/Feature/WorkerEnvironmentMigrationTest.php`:
- Around line 73-77: The test currently grabs migrations with $paths =
glob(database_path('migrations/*_add_worker_environment_support.php')) and
requires $paths[0], which is non-deterministic if multiple files match; update
the test in WorkerEnvironmentMigrationTest (the runMigration() flow) to
deterministically resolve the migration by either asserting exactly one match
with $this->assertCount(1, $paths) before requiring, or locate the exact
basename (e.g. array_filter by basename ===
'YYYY_add_worker_environment_support.php') and then require that chosen path
instead of blindly using $paths[0], then call up() on the resolved migration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cafe3fa8-8bd9-4c12-87f2-df1cde91da14
📒 Files selected for processing (41)
app/Actions/Site/UpdateSiteWorkerEnvironment.phpapp/Actions/Worker/CreateWorker.phpapp/Actions/Worker/RestartAllWorkers.phpapp/Actions/Worker/SyncWorkerStatuses.phpapp/Actions/Worker/UpdateWorkerEnvironment.phpapp/Http/Controllers/API/WorkerController.phpapp/Http/Controllers/SiteSettingController.phpapp/Http/Controllers/WorkerController.phpapp/Jobs/Worker/RestartAllJob.phpapp/Models/Site.phpapp/Models/Worker.phpapp/Policies/WorkerPolicy.phpapp/Services/ProcessManager/ProcessManager.phpapp/Services/ProcessManager/Supervisor.phpapp/SiteTypes/AbstractProxiedSiteType.phpapp/Support/Testing/SSHFake.phpdatabase/factories/WorkerFactory.phpdatabase/migrations/2026_06_06_120237_add_worker_environment_support.phppublic/api-docs/openapi/workers.yamlresources/js/components/dialogs/registry.tsresources/js/components/radio-card.tsxresources/js/lib/editor.tsresources/js/lib/env.tsresources/js/pages/application/components/env.tsxresources/js/pages/application/components/proxied-app-card.tsxresources/js/pages/site-settings/components/start-command.tsxresources/js/pages/workers/components/columns.tsxresources/js/pages/workers/components/env-dialog.tsxresources/js/pages/workers/components/worker-row-actions.tsxresources/js/pages/workers/index.tsxresources/views/ssh/services/process-manager/supervisor/restart-all-workers.blade.phpresources/views/ssh/services/process-manager/supervisor/worker-statuses.blade.phpresources/views/ssh/services/process-manager/supervisor/worker.blade.phptests/Feature/API/WorkersTest.phptests/Feature/AfterDeployHookTest.phptests/Feature/SiteSettingsProxiedSiteTest.phptests/Feature/WorkerEnvironmentMigrationTest.phptests/Feature/WorkerEnvironmentTest.phptests/Feature/WorkersTest.phptests/Unit/AbstractProxiedSiteTypeTest.phpvite.config.ts
saeedvaziry
approved these changes
Jun 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Tests