[Feat] Desktop app (Electron + bundled PHP runtime)#1201
Conversation
- Fix desktop auth routes: guest/auth middleware was silently ignored (Spatie Middleware attribute is class-target only) - Generate SSH keys with phpseclib instead of shelling out to openssl/ssh-keygen (required for Windows) - Serve HTTP via supervised php -S with the framework router and PHP_CLI_SERVER_WORKERS instead of artisan serve - Kill process groups on shutdown and reap stale processes from crashed sessions on startup - Add dedicated desktop asset build (public/build-desktop, gitignored) - Add packaging scripts: app dist assembly, portable PHP runtime (static-php-cli on macOS/Linux, php.net build on Windows), smoke test - Add desktop-release workflow (macOS arm64/x64, Windows x64, Linux x64) wired into the release workflow; installers upload to the GitHub release; optional code signing/notarization via secrets - Add desktop-ci workflow: typecheck + full Linux packaging smoke
- Enforce 2FA on desktop unlock (TOTP + recovery codes) and rate limit desktop auth routes - Move desktop mode check into EnsureDesktopMode middleware; extract SetupDesktopAdmin and VerifyDesktopUnlock actions composing CreateUser - Reuse UserResource and UserInfo on the desktop login screen - Fix single-instance race: losing instance exits before booting runtime - Redact APP_KEY/WS_BROADCAST_SECRET in desktop process logs; write runtime .env, sqlite database, and data directories with restrictive permissions - Unescape quoted values when reading the runtime .env back - Add per-attempt timeout to health checks; handle whenReady rejections - Gate VITO_ENV_PATH/VITO_STORAGE_PATH overrides behind VITO_DESKTOP - Verify checksums for spc and php.net downloads; pass workflow secrets explicitly instead of inherit
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis change adds a cross-platform Electron desktop application with a local Laravel backend, desktop authentication, dynamic runtime paths and ports, supervised processes, bundled PHP provisioning, packaging workflows, release automation, and desktop-aware Laravel integration. ChangesDesktop application foundation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Electron as Electron main process
participant Runtime as Runtime context
participant Supervisor as Process supervisor
participant Laravel as Laravel backend
Electron->>Runtime: Create paths, ports, environment, and PHP settings
Electron->>Supervisor: Start Laravel services
Supervisor->>Laravel: Launch HTTP, websocket, queue, and scheduler processes
Electron->>Laravel: Poll /api/health
Laravel-->>Electron: Return healthy response
Electron->>Laravel: Navigate to /desktop/login
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🤖 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 @.github/workflows/desktop-ci.yml:
- Line 20: Pin every referenced workflow action to a full immutable commit SHA
instead of a mutable tag: actions/checkout, actions/setup-node,
shivammathur/setup-php, and actions/cache at .github/workflows/desktop-ci.yml
lines 20, 23, 39, 42, 51, and 65, and .github/workflows/desktop-release.yml
lines 73, 78, 87, and 102.
- Around line 5-13: Expand the path filters in the desktop CI workflow so
changes to all desktop integration surfaces trigger it, including app actions,
controllers, middleware, authentication pages, and frontend types. Prefer
broader app/**, config/**, and resources/** filters, while preserving the
existing desktop-specific workflow and configuration paths.
In @.github/workflows/desktop-release.yml:
- Around line 135-147: Update the “Package desktop app” step and its
matrix-specific environment handling so macOS signing/notarization secrets are
exposed only when the target platform is darwin, while Windows signing secrets
are exposed only for win32. Keep unrelated credentials unset for Linux and other
platforms, using platform-specific jobs or protected environments rather than
exporting all secrets to every matrix job.
In `@app/Actions/Desktop/SetupDesktopAdmin.php`:
- Around line 22-33: Make SetupDesktopAdmin’s transaction the authoritative
first-run guard: serialize the setup operation and re-check that no users exist
within the protected operation before creating the administrator. In
app/Actions/Desktop/SetupDesktopAdmin.php lines 22-33, update the flow around
the transaction and CreateUser call; in
app/Http/Controllers/DesktopAuthController.php lines 44-46, remove the existing
user-existence check or retain it only as a non-authoritative early
optimization.
In `@app/Http/Controllers/DesktopAuthController.php`:
- Around line 42-83: Move the authentication and session orchestration out of
DesktopAuthController methods setup, store, and lock into dedicated Actions,
including setup eligibility, user authentication, session rotation/locking, and
default-project initialization. Keep each controller method limited to invoking
the appropriate Action and constructing the existing redirect response,
preserving route behavior and redirect destinations.
- Around line 59-68: Bind locked-session unlocks to the recorded user: in
app/Http/Controllers/DesktopAuthController.php:59-68, reject requests when the
route-bound user does not match desktop.locked_user_id. In
resources/js/pages/auth/desktop-login.tsx:48-68, prevent selectedUserId changes
while locked, and at :165-185 hide or disable all accounts except
locked_user_id. In tests/Feature/DesktopAuthTest.php:144-170, add a regression
test proving user B cannot unlock a session locked by user A.
In `@app/Support/DesktopRuntime.php`:
- Around line 50-66: Update parse_url handling in DesktopRuntime::websocketUrl
to fall back to an empty array when parsing the configured URL returns false,
ensuring subsequent scheme, host, and port lookups remain safe. Preserve the
existing URL and websocket routing behavior for valid configurations.
- Around line 12-43: Extract the shared configuration lookup, string validation,
and path joining from dataPath, storagePath, and envPath into a private helper
method. Update each public method to delegate to that helper with its respective
configuration key, preserving the existing null behavior for missing or empty
values and the current joined-path results.
In `@app/Support/helpers.php`:
- Around line 17-22: Replace the shell-based key generation and public-key
derivation in generate_key_pair and the adjacent private-key handling flow with
phpseclib3 APIs, matching the native implementation already used by
GenerateKeysCommand. Remove the exec() calls and preserve the existing file
paths and generated Ed25519 key behavior without invoking external commands.
In `@desktop/entitlements.mac.plist`:
- Around line 9-12: Remove the
com.apple.security.cs.allow-dyld-environment-variables and
com.apple.security.cs.disable-library-validation entries from the shared
entitlements configuration. Keep the shared file limited to required
hardened-runtime permissions, and only add these exceptions to separate minimal
executable-specific entitlements if a particular Electron or PHP binary
demonstrably requires them.
In `@desktop/src/env-file.ts`:
- Around line 40-46: Update writeEnvFile to stop swallowing chmodSync failures:
remove the empty catch around the permission-setting call and allow the error to
propagate, ensuring the credentials file is not treated as successfully secured
when chmodSync fails.
In `@desktop/src/logging.ts`:
- Around line 20-27: Update the logging methods line and write to replace
synchronous appendFileSync calls with a buffered write stream, and add bounded
rotation or retention handling so sustained output cannot block the Electron
main thread or grow the log file without limit. Preserve existing timestamps,
source/level prefixes, redaction, and newline behavior.
In `@desktop/src/main.ts`:
- Around line 128-148: Update attachNavigationGuards to register a will-redirect
handler using the same appUrl allowlist as will-navigate. Allow redirects within
appUrl, but prevent external redirects and pass their URLs to openExternally so
they open outside the application window.
In `@desktop/src/ports.ts`:
- Around line 3-18: Update getAvailablePort so dynamically allocated ports are
not released before the supervised services bind: retain the listening socket
through hand-off, or implement distinct-port tracking with EADDRINUSE
reallocation and retry. Preserve the existing rejection behavior for
unrecoverable allocation or address errors, and ensure consecutive
HTTP/WebSocket allocations cannot return the same port.
In `@desktop/src/process-supervisor.ts`:
- Around line 107-113: Update the stale-process entry flow, including
findStalePids and its Windows matching path, to persist options.command as a
separate executable field instead of extracting it by splitting on spaces.
Compare basename(executable) with the tasklist output so paths containing spaces
still match the correct process, and apply the same change to the corresponding
logic around the additional referenced block.
- Around line 195-217: Update the process lifecycle handling around the child
error and exit listeners so a failed spawn clears this.child and enters the same
restart/backoff flow even when no exit event occurs. Prefer consolidating
recovery in a close handler, or add a once-only guard shared by error/exit to
prevent duplicate restart scheduling while preserving the existing persistent,
stopping, and canRestart checks.
In `@scripts/desktop/smoke-test.sh`:
- Around line 47-50: Update REQUIRED_EXTENSIONS in the smoke-test script to
include all modules listed by COMMON_EXTENSIONS in fetch-php.sh, specifically
adding bcmath, gmp, and sodium, and add an appropriate Zend OPcache validation.
Preserve the existing platform-specific pcntl and posix additions.
In `@tests/Feature/DesktopAuthTest.php`:
- Around line 144-170: Add a regression test alongside
test_desktop_unlock_requires_password_after_locking that creates users A and B,
stores A’s ID in desktop.locked_user_id, and submits B’s valid password to B’s
desktop login route. Assert the request is rejected with the appropriate
password/session error and remains unauthenticated.
In `@tests/Unit/Commands/GenerateKeysCommandTest.php`:
- Around line 8-11: Add the RefreshDatabase trait to GenerateKeysCommandTest and
import it from the framework testing namespace, so the inherited TestCase setUp
database interactions run against a refreshed database for each test.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 278146f4-44cb-4db8-a8ec-880c2fd3423a
⛔ Files ignored due to path filters (2)
desktop/build/icon.pngis excluded by!**/*.pngdesktop/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (54)
.env.desktop.example.github/workflows/desktop-ci.yml.github/workflows/desktop-release.yml.github/workflows/releases.yml.gitignoreapp/Actions/Desktop/SetupDesktopAdmin.phpapp/Actions/Desktop/VerifyDesktopUnlock.phpapp/Console/Commands/GenerateKeysCommand.phpapp/Http/Controllers/ConsoleController.phpapp/Http/Controllers/DesktopAuthController.phpapp/Http/Controllers/EventsController.phpapp/Http/Controllers/HomeController.phpapp/Http/Kernel.phpapp/Http/Middleware/Authenticate.phpapp/Http/Middleware/EnsureDesktopMode.phpapp/Http/Middleware/HandleInertiaRequests.phpapp/Providers/AppServiceProvider.phpapp/Providers/AuthServiceProvider.phpapp/Support/DesktopRuntime.phpapp/Support/helpers.phpbootstrap/app.phpconfig/desktop.phpconfig/queue.phpdesktop/README.mddesktop/electron-builder.ymldesktop/entitlements.mac.plistdesktop/package.jsondesktop/src/env-file.tsdesktop/src/health-check.tsdesktop/src/laravel-runtime.tsdesktop/src/loading-page.tsdesktop/src/logging.tsdesktop/src/main.tsdesktop/src/ports.tsdesktop/src/preload.tsdesktop/src/process-supervisor.tsdesktop/src/runtime-paths.tsdesktop/tsconfig.jsondocs/desktop-app-plan.mdpackage.jsonresources/js/components/user-menu-content.tsxresources/js/pages/auth/desktop-login.tsxresources/js/types/index.d.tsscripts/desktop/build-app.shscripts/desktop/fetch-php.shscripts/desktop/smoke-test.shtests/Feature/DesktopAuthTest.phptests/Feature/NotificationChannelsTest.phptests/Feature/ServerProvidersTest.phptests/Feature/SourceControlsTest.phptests/Feature/StorageProvidersTest.phptests/Unit/Commands/GenerateKeysCommandTest.phptests/Unit/DesktopRuntimeTest.phpvite.config.ts
| paths: | ||
| - 'desktop/**' | ||
| - 'scripts/desktop/**' | ||
| - '.github/workflows/desktop-ci.yml' | ||
| - '.github/workflows/desktop-release.yml' | ||
| - 'vite.config.ts' | ||
| - 'bootstrap/app.php' | ||
| - 'config/desktop.php' | ||
| - 'app/Support/DesktopRuntime.php' |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Run desktop CI for every desktop integration surface.
These filters omit supplied stack dependencies such as app/Actions/Desktop/**, desktop controllers and middleware, desktop authentication pages, and frontend types. Changes there can break the packaged application without running this workflow. Add the complete desktop integration paths, or use broader app/**, config/**, and resources/** filters.
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 3-13: insufficient job-level concurrency limits (concurrency-limits): workflow is missing concurrency setting
(concurrency-limits)
🤖 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 @.github/workflows/desktop-ci.yml around lines 5 - 13, Expand the path
filters in the desktop CI workflow so changes to all desktop integration
surfaces trigger it, including app actions, controllers, middleware,
authentication pages, and frontend types. Prefer broader app/**, config/**, and
resources/** filters, while preserving the existing desktop-specific workflow
and configuration paths.
| runs-on: ubuntu-24.04 | ||
|
|
||
| steps: | ||
| - uses: actions/checkout@v4 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Pin workflow actions to immutable commit SHAs.
The new packaging workflows depend on mutable action tags, allowing an upstream tag change to execute different code in privileged build and release jobs.
.github/workflows/desktop-ci.yml#L20-L20: pinactions/checkout..github/workflows/desktop-ci.yml#L23-L23: pinactions/setup-node..github/workflows/desktop-ci.yml#L39-L39: pinactions/checkout..github/workflows/desktop-ci.yml#L42-L42: pinshivammathur/setup-php..github/workflows/desktop-ci.yml#L51-L51: pinactions/setup-node..github/workflows/desktop-ci.yml#L65-L65: pinactions/cache..github/workflows/desktop-release.yml#L73-L73: pinactions/checkout..github/workflows/desktop-release.yml#L78-L78: pinshivammathur/setup-php..github/workflows/desktop-release.yml#L87-L87: pinactions/setup-node..github/workflows/desktop-release.yml#L102-L102: pinactions/cache.
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 20-20: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 20-20: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
📍 Affects 2 files
.github/workflows/desktop-ci.yml#L20-L20(this comment).github/workflows/desktop-ci.yml#L23-L23.github/workflows/desktop-ci.yml#L39-L39.github/workflows/desktop-ci.yml#L42-L42.github/workflows/desktop-ci.yml#L51-L51.github/workflows/desktop-ci.yml#L65-L65.github/workflows/desktop-release.yml#L73-L73.github/workflows/desktop-release.yml#L78-L78.github/workflows/desktop-release.yml#L87-L87.github/workflows/desktop-release.yml#L102-L102
🤖 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 @.github/workflows/desktop-ci.yml at line 20, Pin every referenced workflow
action to a full immutable commit SHA instead of a mutable tag:
actions/checkout, actions/setup-node, shivammathur/setup-php, and actions/cache
at .github/workflows/desktop-ci.yml lines 20, 23, 39, 42, 51, and 65, and
.github/workflows/desktop-release.yml lines 73, 78, 87, and 102.
Source: Linters/SAST tools
| - name: Package desktop app | ||
| shell: bash | ||
| run: npx electron-builder --config electron-builder.yml ${{ matrix.build-args }} --publish never | ||
| working-directory: desktop | ||
| env: | ||
| CSC_LINK: ${{ secrets.DESKTOP_MAC_CSC_LINK }} | ||
| CSC_KEY_PASSWORD: ${{ secrets.DESKTOP_MAC_CSC_KEY_PASSWORD }} | ||
| WIN_CSC_LINK: ${{ secrets.DESKTOP_WIN_CSC_LINK }} | ||
| WIN_CSC_KEY_PASSWORD: ${{ secrets.DESKTOP_WIN_CSC_KEY_PASSWORD }} | ||
| APPLE_ID: ${{ secrets.DESKTOP_APPLE_ID }} | ||
| APPLE_APP_SPECIFIC_PASSWORD: ${{ secrets.DESKTOP_APPLE_APP_SPECIFIC_PASSWORD }} | ||
| APPLE_TEAM_ID: ${{ secrets.DESKTOP_APPLE_TEAM_ID }} | ||
| CSC_IDENTITY_AUTO_DISCOVERY: ${{ secrets.DESKTOP_MAC_CSC_LINK != '' }} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Expose signing credentials only to their target platform.
Every matrix job currently receives the macOS, Windows, and Apple notarisation credentials. A compromised Linux or Windows packaging dependency could therefore exfiltrate unrelated signing identities. Export macOS secrets only for darwin and Windows secrets only for win32, preferably through separately protected jobs or environments.
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 137-137: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🤖 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 @.github/workflows/desktop-release.yml around lines 135 - 147, Update the
“Package desktop app” step and its matrix-specific environment handling so macOS
signing/notarization secrets are exposed only when the target platform is
darwin, while Windows signing secrets are exposed only for win32. Keep unrelated
credentials unset for Linux and other platforms, using platform-specific jobs or
protected environments rather than exporting all secrets to every matrix job.
| return DB::transaction(function () use ($input): User { | ||
| $user = app(CreateUser::class)->create([ | ||
| 'name' => $input['name'] ?? null, | ||
| 'email' => $input['email'] ?? null, | ||
| 'password' => $input['password'] ?? null, | ||
| 'role' => UserRole::ADMIN->value, | ||
| ]); | ||
|
|
||
| $user->ensureHasDefaultProject(); | ||
|
|
||
| return $user; | ||
| }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
First-run administrator eligibility is not enforced atomically. Concurrent unauthenticated requests can both observe an empty user table before creating separate administrators.
app/Actions/Desktop/SetupDesktopAdmin.php#L22-L33: serialise setup and re-check that no user exists inside the protected operation.app/Http/Controllers/DesktopAuthController.php#L44-L46: remove this check as the authoritative guard, or retain it only as an early optimisation.
🧰 Tools
🪛 PHPMD (2.15.0)
[error] 22-33: Avoid using static access to class '\Illuminate\Support\Facades\DB' in method 'create'. (undefined)
(StaticAccess)
📍 Affects 2 files
app/Actions/Desktop/SetupDesktopAdmin.php#L22-L33(this comment)app/Http/Controllers/DesktopAuthController.php#L44-L46
🤖 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/Actions/Desktop/SetupDesktopAdmin.php` around lines 22 - 33, Make
SetupDesktopAdmin’s transaction the authoritative first-run guard: serialize the
setup operation and re-check that no users exist within the protected operation
before creating the administrator. In app/Actions/Desktop/SetupDesktopAdmin.php
lines 22-33, update the flow around the transaction and CreateUser call; in
app/Http/Controllers/DesktopAuthController.php lines 44-46, remove the existing
user-existence check or retain it only as a non-authoritative early
optimization.
| public function setup(Request $request, SetupDesktopAdmin $setupDesktopAdmin): RedirectResponse | ||
| { | ||
| abort_unless(User::query()->doesntExist(), 404); | ||
|
|
||
| $user = $setupDesktopAdmin->create($request->all()); | ||
|
|
||
| Auth::login($user); | ||
|
|
||
| $request->session()->forget(self::LOCKED_USER_ID); | ||
| $request->session()->regenerate(); | ||
|
|
||
| return redirect()->intended(RouteServiceProvider::HOME); | ||
| } | ||
|
|
||
| #[Post('/login/{user}', name: 'desktop.login.store', middleware: ['guest', 'throttle:desktop-auth'])] | ||
| public function store(Request $request, User $user, VerifyDesktopUnlock $verifyDesktopUnlock): RedirectResponse | ||
| { | ||
| if ($this->lockedUserId($request) !== null) { | ||
| $verifyDesktopUnlock->verify($user, $request->all()); | ||
| } | ||
|
|
||
| Auth::login($user); | ||
|
|
||
| $request->session()->forget(self::LOCKED_USER_ID); | ||
| $request->session()->regenerate(); | ||
|
|
||
| $user->ensureHasDefaultProject(); | ||
|
|
||
| return redirect()->intended(RouteServiceProvider::HOME); | ||
| } | ||
|
|
||
| #[Post('/lock', name: 'desktop.lock', middleware: 'auth')] | ||
| public function lock(Request $request): RedirectResponse | ||
| { | ||
| /** @var User $user */ | ||
| $user = $request->user(); | ||
|
|
||
| Auth::guard('web')->logout(); | ||
|
|
||
| $request->session()->migrate(true); | ||
| $request->session()->put(self::LOCKED_USER_ID, $user->id); | ||
| $request->session()->regenerateToken(); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Move authentication and session orchestration into Actions.
These methods perform setup eligibility, authentication, locking, session rotation, and project initialisation. Keep the controller to Action invocation and response construction.
As per coding guidelines, “Controllers are thin — they call an Action and return the response.”
🧰 Tools
🪛 PHPMD (2.15.0)
[error] 48-48: Avoid using static access to class '\Illuminate\Support\Facades\Auth' in method 'setup'. (undefined)
(StaticAccess)
[error] 63-63: Avoid using static access to class '\Illuminate\Support\Facades\Auth' in method 'store'. (undefined)
(StaticAccess)
🤖 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/Http/Controllers/DesktopAuthController.php` around lines 42 - 83, Move
the authentication and session orchestration out of DesktopAuthController
methods setup, store, and lock into dedicated Actions, including setup
eligibility, user authentication, session rotation/locking, and default-project
initialization. Keep each controller method limited to invoking the appropriate
Action and constructing the existing redirect response, preserving route
behavior and redirect destinations.
Source: Coding guidelines
| function findStalePids(entry: StaleProcessEntry): number[] { | ||
| try { | ||
| if (process.platform === 'win32') { | ||
| const output = execFileSync('tasklist', ['/FI', `PID eq ${entry.pid}`, '/FO', 'CSV', '/NH'], { encoding: 'utf8' }); | ||
|
|
||
| return output.toLowerCase().includes(basename(entry.command.split(' ')[0]).toLowerCase()) ? [entry.pid] : []; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Store the executable separately when matching Windows processes.
Line 112 truncates paths at the first space. An installation under a path such as C:\Users\Jane Doe\... therefore fails to match php.exe, leaving stale queue and scheduler processes alive across launches. Persist options.command as a separate executable field and compare basename(executable) with tasklist.
Also applies to: 165-174
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { ChildProcessWithoutNullStreams, execFileSync, spawn } from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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 `@desktop/src/process-supervisor.ts` around lines 107 - 113, Update the
stale-process entry flow, including findStalePids and its Windows matching path,
to persist options.command as a separate executable field instead of extracting
it by splitting on spaces. Compare basename(executable) with the tasklist output
so paths containing spaces still match the correct process, and apply the same
change to the corresponding logic around the additional referenced block.
| this.child.on('error', (error) => { | ||
| this.log.error(`${this.options.name} failed to start: ${error.message}`); | ||
| }); | ||
|
|
||
| this.child.on('exit', (code, signal) => { | ||
| this.log.info(`${this.options.name} exited with code=${code ?? 'null'} signal=${signal ?? 'null'}`); | ||
| this.child = null; | ||
| this.onChange?.(); | ||
|
|
||
| if (Date.now() - this.startedAt >= 60000) { | ||
| this.consecutiveRestarts = 0; | ||
| } | ||
|
|
||
| if (!this.stopping && this.options.persistent !== false && this.canRestart()) { | ||
| const delay = Math.min(1000 * 2 ** this.consecutiveRestarts, this.options.restartBackoffMaxMs ?? 30000); | ||
| this.consecutiveRestarts += 1; | ||
| this.log.info(`Restarting ${this.options.name} in ${delay}ms`); | ||
| this.restartTimer = setTimeout(() => { | ||
| this.restartTimer = null; | ||
| this.start(); | ||
| }, delay); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
In Node.js 22 child_process.spawn, is the "exit" event guaranteed after an "error" event when spawning fails, or should lifecycle handling use "close"?
💡 Result:
In Node.js 22, the 'exit' event is not guaranteed to fire after an 'error' event when spawning a child process [1][2]. Specifically, if a process fails to spawn (e.g., the command does not exist), the 'error' event is emitted, but the 'exit' event will not necessarily follow [1][3]. Regarding lifecycle handling, using the 'close' event is generally more robust for detecting when a process has fully completed its lifecycle [1][2]. The 'close' event is emitted only after both the process has ended and all stdio streams have been closed [1][4]. This ensures that all data from stdout/stderr has been processed, whereas the 'exit' event may be emitted while stdio streams are still open [1][2]. For reliable error handling and completion monitoring, you should: - Listen for the 'error' event to catch spawn-time failures [1][3]. - Listen for the 'close' event to detect the final termination of the process and its associated streams [1][4]. - Guard against handling both 'exit' and 'error' events in a way that might execute your callback or logic multiple times, as both may fire in some scenarios, but not others [1][5]. The 'close' event will always fire after either 'exit' (if the process started) or 'error' (if the spawn failed), making it a single point of truth for process cleanup [1][2].
Citations:
- 1: https://nodejs.org/api/child_process.html
- 2: https://r2.nodejs.org/docs/v22.17.1/api/child_process.html
- 3: https://nodejs.org/api/child_process.md
- 4: https://stackoverflow.com/questions/37522010/difference-between-childprocess-close-exit-events
- 5: https://github.com/nodejs/node/blob/main/doc/api/child_process.md
🏁 Script executed:
set -e
# Map the file structure and inspect the relevant file in context.
git ls-files desktop/src/process-supervisor.ts
wc -l desktop/src/process-supervisor.ts
sed -n '1,280p' desktop/src/process-supervisor.tsRepository: vitodeploy/vito
Length of output: 8742
🏁 Script executed:
set -e
git ls-files desktop/src/process-supervisor.ts
wc -l desktop/src/process-supervisor.ts
sed -n '1,280p' desktop/src/process-supervisor.tsRepository: vitodeploy/vito
Length of output: 8742
🏁 Script executed:
sed -n '280,380p' desktop/src/process-supervisor.tsRepository: vitodeploy/vito
Length of output: 1993
Handle spawn failures in the restart path. error only logs here, and exit is not guaranteed after a failed spawn. That leaves child uncleared and never triggers the restart/backoff logic, so the supervised process can stay down indefinitely. Move the restart bookkeeping to close, or schedule recovery from error with a once-only guard.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { ChildProcessWithoutNullStreams, execFileSync, spawn } from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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 `@desktop/src/process-supervisor.ts` around lines 195 - 217, Update the process
lifecycle handling around the child error and exit listeners so a failed spawn
clears this.child and enters the same restart/backoff flow even when no exit
event occurs. Prefer consolidating recovery in a close handler, or add a
once-only guard shared by error/exit to prevent duplicate restart scheduling
while preserving the existing persistent, stopping, and canRestart checks.
| REQUIRED_EXTENSIONS=(ctype curl dom fileinfo filter ftp iconv intl mbstring openssl pdo_sqlite phar session simplexml sockets sqlite3 tokenizer xml xmlreader xmlwriter zip zlib) | ||
| if [ "$IS_WINDOWS" != "true" ]; then | ||
| REQUIRED_EXTENSIONS+=(pcntl posix) | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate every required bundled PHP capability.
This list omits at least bcmath, gmp, and sodium from COMMON_EXTENSIONS in scripts/desktop/fetch-php.sh Lines 20-21. A deficient packaged runtime can therefore pass CI and fail only when those features are used. Add the omitted modules and an appropriate check for Zend OPcache.
🤖 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 `@scripts/desktop/smoke-test.sh` around lines 47 - 50, Update
REQUIRED_EXTENSIONS in the smoke-test script to include all modules listed by
COMMON_EXTENSIONS in fetch-php.sh, specifically adding bcmath, gmp, and sodium,
and add an appropriate Zend OPcache validation. Preserve the existing
platform-specific pcntl and posix additions.
| public function test_desktop_unlock_requires_password_after_locking(): void | ||
| { | ||
| config()->set('desktop.enabled', true); | ||
|
|
||
| /** @var User $user */ | ||
| $user = User::factory()->create([ | ||
| 'password' => Hash::make('secret-password'), | ||
| ]); | ||
|
|
||
| $this->withSession(['desktop.locked_user_id' => $user->id]) | ||
| ->post(route('desktop.login.store', $user), [ | ||
| 'password' => 'wrong-password', | ||
| ]) | ||
| ->assertSessionHasErrors('password'); | ||
|
|
||
| $this->assertGuest(); | ||
|
|
||
| $response = $this->withSession(['desktop.locked_user_id' => $user->id]) | ||
| ->post(route('desktop.login.store', $user), [ | ||
| 'password' => 'secret-password', | ||
| ]); | ||
|
|
||
| $this->assertAuthenticatedAs($user); | ||
| $response | ||
| ->assertRedirect(RouteServiceProvider::HOME) | ||
| ->assertSessionMissing('desktop.locked_user_id'); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Cover attempts to unlock as a different user.
Add a regression test that stores user A in desktop.locked_user_id, posts valid credentials for user B to user B’s route, and confirms the request is rejected and remains unauthenticated.
🧰 Tools
🪛 PHPMD (2.15.0)
[error] 144-170: The method test_desktop_unlock_requires_password_after_locking is not named in camelCase. (undefined)
(CamelCaseMethodName)
[error] 150-150: Avoid using static access to class '\Illuminate\Support\Facades\Hash' in method 'test_desktop_unlock_requires_password_after_locking'. (undefined)
(StaticAccess)
🤖 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/DesktopAuthTest.php` around lines 144 - 170, Add a regression
test alongside test_desktop_unlock_requires_password_after_locking that creates
users A and B, stores A’s ID in desktop.locked_user_id, and submits B’s valid
password to B’s desktop login route. Assert the request is rejected with the
appropriate password/session error and remains unauthenticated.
| class GenerateKeysCommandTest extends TestCase | ||
| { | ||
| public function test_it_generates_keys_when_storage_path_contains_spaces(): void | ||
| { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add the RefreshDatabase trait.
The base TestCase's setUp() method creates models such as $this->user and $this->server, which requires database interaction. As per the path instructions for tests/**, tests must use the RefreshDatabase trait to ensure the database is properly initialized and cleaned up.
🛠️ Proposed fix to add the trait
namespace Tests\Unit\Commands;
use Illuminate\Support\Facades\File;
+use Illuminate\Foundation\Testing\RefreshDatabase;
use Tests\TestCase;
class GenerateKeysCommandTest extends TestCase
{
+ use RefreshDatabase;
+
public function test_it_generates_keys_when_storage_path_contains_spaces(): void📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class GenerateKeysCommandTest extends TestCase | |
| { | |
| public function test_it_generates_keys_when_storage_path_contains_spaces(): void | |
| { | |
| namespace Tests\Unit\Commands; | |
| use Illuminate\Foundation\Testing\RefreshDatabase; | |
| use Illuminate\Support\Facades\File; | |
| use Tests\TestCase; | |
| class GenerateKeysCommandTest extends TestCase | |
| { | |
| use RefreshDatabase; | |
| public function test_it_generates_keys_when_storage_path_contains_spaces(): void |
🤖 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/Unit/Commands/GenerateKeysCommandTest.php` around lines 8 - 11, Add the
RefreshDatabase trait to GenerateKeysCommandTest and import it from the
framework testing namespace, so the inherited TestCase setUp database
interactions run against a refreshed database for each test.
Source: Path instructions
# Conflicts: # app/Http/Controllers/ConsoleController.php # app/Http/Controllers/EventsController.php
…S_URL in DesktopRuntime::websocketUrl
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@desktop/src/process-supervisor.ts`:
- Around line 94-98: Update the stale-process termination logic to inspect the
error caught from process.kill in the cleanup loop. Continue treating ESRCH as
already cleaned, but preserve the PID file and stop removing cleanup state for
other errors such as EPERM so the next launch can retry termination.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9328df00-c6b8-49a6-a756-30ad089dc42f
📒 Files selected for processing (3)
desktop/src/env-file.tsdesktop/src/process-supervisor.tseslint.config.js
| try { | ||
| process.kill(pid, 'SIGKILL'); | ||
| } catch { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Retain cleanup state when terminating a stale process fails.
The PID file is removed after this catch, even when process.kill() fails with a real error such as EPERM. That prevents the next launch from retrying cleanup, leaving stale PHP, queue, or scheduler processes running and potentially causing port conflicts. Treat ESRCH as already-cleaned, but preserve the PID file for other failures.
🛠️ Proposed fix
export function cleanupStaleProcesses(pidFilePath: string, log: DesktopLog): void {
+ let cleanupFailed = false;
+
// ...
try {
process.kill(pid, 'SIGKILL');
- } catch {
+ } catch (error) {
+ const code = error instanceof Error ? (error as NodeJS.ErrnoException).code : undefined;
+ if (code !== 'ESRCH') {
+ cleanupFailed = true;
+ }
continue;
}
@@
- rmSync(pidFilePath, { force: true });
+ if (!cleanupFailed) {
+ rmSync(pidFilePath, { force: true });
+ }
}Also applies to: 106-106
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { ChildProcessWithoutNullStreams, execFileSync, spawn } from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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 `@desktop/src/process-supervisor.ts` around lines 94 - 98, Update the
stale-process termination logic to inspect the error caught from process.kill in
the cleanup loop. Continue treating ESRCH as already cleaned, but preserve the
PID file and stop removing cleanup state for other errors such as EPERM so the
next launch can retry termination.
The packaged desktop app ships compiled assets only; resources/js is excluded from the distribution, so the runtime page lookup must be off.
1 participant
What
Ships Vito as a single-binary desktop app for macOS (arm64/x64), Windows (x64), and Linux (x64). One installer contains the Electron shell, the Laravel app, the built frontend assets, and a portable PHP 8.4 runtime — users need nothing else installed, and the app works exactly like the web version.
How it works
The Electron main process supervises four backend processes on
127.0.0.1with dynamic ports:php -S(HTTP, framework router, multi-worker),queue:work database,schedule:work, andws:serve. Desktop mode uses SQLite + database queues + file cache/sessions (no Redis/Horizon). All mutable data lives in the OS app-data dir (~/Library/Application Support/Vito,%APPDATA%/Vito,~/.config/Vito), never inside the installed bundle.Desktop auth: first launch shows a first-run admin setup; afterwards a local user picker. "Lock app" replaces logout and requires the user's password — and their 2FA code if enabled — to unlock.
Frontend assets for desktop build to
public/build-desktop(gitignored) vianpm run build:desktop, leaving the committedpublic/builduntouched.PHP runtime per platform
php.ini; checksums verified againstreleases.json.curl.cainfo/openssl.cafile/SSL_CERT_FILE.Process reliability
php -Sworkers and scheduler children die too).Release flow
releasesworkflow now also builds and attaches desktop installers to every release:Vito-<version>-mac-{arm64,x64}.{dmg,zip},Vito-<version>-win-x64.exe,Vito-<version>-linux-x86_64.AppImage.desktop-releaseworkflow can be dispatched standalone with an existing tag to (re)build desktop assets for it.desktop-ciworkflow runs on PRs touching desktop files: typecheck + a full Linux packaging smoke test (assemble dist, fetch PHP, boot, migrate,optimize, HTTP health check, electron-builder).Optional signing secrets (unsigned builds without them)
DESKTOP_MAC_CSC_LINK/DESKTOP_MAC_CSC_KEY_PASSWORDDESKTOP_APPLE_ID/DESKTOP_APPLE_APP_SPECIFIC_PASSWORD/DESKTOP_APPLE_TEAM_IDDESKTOP_WIN_CSC_LINK/DESKTOP_WIN_CSC_KEY_PASSWORDBug fixes found along the way
guest/authmiddleware — method-level#[Middleware]attributes are silently ignored (class-target only); moved to route params.ssh-key:generateshelled out toopenssl/ssh-keygen(absent on stock Windows) — now uses phpseclib.artisan serveorphaned its innerphp -Son shutdown and served single-threaded — replaced with a directly-supervisedphp -S+ workers..env/DB file permissions, secret redaction in process logs, health-check timeouts, checksum-verified runtime downloads.Verified
Follow-ups (not in scope)
Summary by CodeRabbit