Skip to content

Add development process orchestration#434

Merged
binaryfire merged 16 commits into
0.4from
feature/dev-command
Jul 14, 2026
Merged

Add development process orchestration#434
binaryfire merged 16 commits into
0.4from
feature/dev-command

Conversation

@binaryfire

@binaryfire binaryfire commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

Summary

This PR adds Laravel-shaped development process orchestration to Hypervel:

  • A worker-lifetime DevCommands registry with framework, dependency, and application priorities.
  • dev and dev:list console commands with filtering, source inspection, JSON output, colors, and process orchestration through concurrently.
  • Bun, npm, pnpm, and Yarn adapters with workspace-aware lockfile detection.
  • Watcher-owned server startup and signal-safe child process cleanup.
  • Static state cleanup, package documentation, and focused regression coverage.

The default process set is php artisan watch, php artisan queue:listen --tries=1 --timeout=0, and the detected package manager's dev script. Hypervel does not add a Pail-equivalent process or change application logging.

For more detailes, see: docs/plans/2026-07-14-development-command-orchestration.md

Motivation

Laravel now has a useful registry and command surface for running an application's development processes from one entry point. The public API maps well to Hypervel, but the implementation cannot be copied without accounting for Swoole process ownership and this repository's Composer and JavaScript workspace layouts.

Hypervel's development server is already owned by Watcher. Starting serve directly from dev would bypass file-change restarts and split process ownership between two commands. The default server process is therefore php artisan watch, which continues to own and restart php artisan serve.

The other important difference is package discovery. Composer path repositories resolve to their real source directories, and nested applications may inherit a package manager from a workspace lockfile above their own directory. Treating every non-vendor source as application code or checking lockfiles only in getcwd() produces incorrect results in both cases.

Design

Registration and priority

DevCommands stores named process definitions and assigns one of three priorities:

  1. Framework defaults.
  2. Composer dependencies.
  3. Application registrations.

A higher-priority registration replaces a lower-priority process with the same name. The full registration backtrace is inspected rather than only the immediate caller, so application code calling through a package helper remains application-owned.

Dependency classification first checks Composer's real install paths for every non-root package. This handles path repositories without treating the root package as a dependency. Both installed package paths and the conventional vendor directory use boundary-safe comparisons, so a sibling such as vendor-tools remains application code. Source paths and the vendor directory are resolved through realpath() when available, which also keeps classification correct for symlinked base paths.

The registry exposes the Laravel-compatible register, artisan, node, nodeExec, only, and except methods. Its public mutators are documented as boot-only because the state persists for the worker lifetime. The PHPUnit after-each subscriber clears the complete registry and the dev prohibition flag between tests.

Process orchestration

dev reads the effective process list once and uses that snapshot for validation, display, and command construction. An empty list returns a clear failure before attempting width or name calculations.

The command checks for Watcher only when the effective server entry is still the framework default. If Watcher is missing, it fails with:

composer require --dev hypervel/watcher

Filtering out server or replacing it with a higher-priority registration bypasses that check because the developer then owns the server process.

Hypervel console commands normally execute inside a coroutine. dev explicitly runs in the native console process because it launches long-running subprocesses and hands the process to pcntl_exec. This ensures the terminal-facing process is also the process that owns the concurrently tree. The fallback path retains passthru behavior and restores the original COLUMNS environment variable.

Package-manager detection

The package-manager API keeps Laravel's public signatures, including no-argument matches(): bool methods on each concrete adapter.

The manager privately walks from getcwd() to the filesystem root. At each directory it checks Bun, pnpm, Yarn, then npm lockfiles. The nearest recognized lockfile wins, and npm remains the fallback when no lockfile exists.

Execution uses project-local tools:

  • bunx for Bun.
  • pnpm exec for pnpm.
  • yarn run for Yarn.
  • npx for npm.

This lets an application nested in a workspace use the package manager and installed binaries selected by that workspace without changing the process-global working directory.

Listing and terminal behavior

dev:list supports interactive output, JSON output, name filtering, dependency-only filtering, source paths, and registration priorities. It uses Hypervel Prompts for terminal width.

When a terminal is too narrow to display a source column, the source is omitted instead of passing a negative width to string truncation. The command remains visible even with COLUMNS=1.

Watcher lifecycle

Targeted termination of the Watcher command can interrupt Watcher::run() before its normal cleanup path completes. WatchCommand now traps SIGINT, SIGTERM, and SIGQUIT before entering the watcher loop.

The trap stops the file-watching driver first and guarantees restart-strategy cleanup with finally. This prevents the Swoole server child from surviving command termination even when driver shutdown throws.

The command keeps its public container constructor and resolves Hypervel\Contracts\Foundation\Application internally. This removes the concrete Foundation application dependency without changing the constructor surface.

Laravel compatibility

The public development command, registry, value object, color, and package-manager APIs remain aligned with Laravel. Hypervel-specific behavior is limited to runtime and repository-layout requirements:

  • Watcher replaces Laravel's default direct server process.
  • Hypervel omits the Pail process because no equivalent command exists.
  • Lockfile detection searches ancestor directories.
  • Composer path repositories are recognized as dependencies through installed package metadata.
  • pnpm and Yarn execute project-local binaries.
  • Watcher explicitly owns termination cleanup for its driver and server child.

Literal php artisan command strings are retained for parity and readable dev:list output.

Verification

composer fix passes in full:

  • CS Fixer: no remaining changes.
  • Both PHPStan configurations: no errors.
  • Framework suite: 22,640 tests, 64,325 assertions, 1,585 skipped.
  • Testbench suite: 327 tests, 955 assertions, 3 skipped.
  • Dogfood package suite: 4 tests, 7 assertions.

Focused coverage includes registry priority traversal, Composer path packages, vendor path boundaries, ancestor lockfile selection, command filtering and replacement, missing Watcher failures, single-snapshot consumption, coroutine opt-out, one-column terminal rendering, provider registration, state cleanup, signal trapping, and guaranteed Watcher child cleanup.

Summary by CodeRabbit

  • New Features

    • Added dev to run server, queue, and frontend development processes together (with watcher integration).
    • Added dev:list to view, filter, and export configured dev processes as JSON or an interactive list.
    • Automatically detects Bun, pnpm, Yarn, or npm from nearest lockfiles and uses the right run/exec commands.
    • Added command coloring and priority-based process registration.
  • Bug Fixes

    • Improved signal handling and shutdown ordering for the watcher to stop managed processes cleanly.
  • Documentation

    • Documented dev/dev:list behavior, package-manager detection, and Hypervel-specific defaults and differences from Laravel.

Port the Laravel package manager contract and the Bun, npm, pnpm, and Yarn adapters. Detect the nearest ancestor lockfile so nested workspace applications use their owning package manager, while preserving the public no-argument matches API. Use project-local execution commands for pnpm and Yarn, and cover detection order, ancestor traversal, fallback behavior, and command generation.
Introduce the typed development command value object and backed color enum used by the process registry. Preserve Laravel's fluent color API, source metadata, priority values, name derivation, and array representation while applying Hypervel's strict typing and enum conventions. Cover construction, serialization, explicit colors, and fluent color selection.
Add the worker-lifetime registry for framework, dependency, and application development processes. Register Watcher, the queue listener, and Vite as defaults; support Laravel-compatible registration, filtering, color assignment, and replacement priorities; and classify Composer path-repository packages through their real install paths with boundary-safe matching. Include complete state reset support and regression coverage for source resolution, priority traversal, path packages, vendor-prefixed siblings, defaults, filters, and colors.
Port Laravel's development process inspection command with text and JSON output, name filtering, vendor filtering, source locations, registration priorities, and interactive formatting. Use Hypervel Prompts for terminal sizing and handle terminals too narrow for a source column without hiding the command text. Declare the Foundation package's direct Prompts dependency and cover all output modes, filters, sources, and narrow-terminal behavior.
Add the Laravel-compatible dev command that renders the effective process list and launches it through the detected package manager's concurrently binary. Run natively so pcntl_exec owns the long-running process tree, consume one registry snapshot, fail cleanly for an empty process list, and require Watcher only while the framework default server remains active. Cover filtering and replacement ownership, actionable Watcher failures, process construction, registry consumption, and coroutine opt-out.
Register the development process registry and dev command prohibition flag with the framework's authoritative PHPUnit cleanup subscriber. This clears worker-lifetime static registrations, filters, colors, package-manager detection, and prohibition state after every test without duplicating teardown logic across test cases.
Register dev and dev:list with Foundation's lazy console command loader and install the default development processes during console boot. Keep registration behind the registry's console guard so non-console workers do not retain development-only static state. Cover command discovery and the expected default process registration through the Foundation service provider.
Resolve the Foundation application through its contract and trap SIGINT, SIGTERM, and SIGQUIT before entering the Watcher loop. Stop the file-watching driver first and guarantee restart-strategy cleanup with finally so targeted command termination cannot leave the Swoole server child running, even when driver cleanup throws. Cover contract resolution, trap registration, normal cleanup, and the exceptional cleanup path.
Document why Hypervel's default development server runs through Watcher instead of invoking serve directly, and why the default process list omits Laravel's Pail process. Describe the resulting server, queue, and Vite orchestration without changing application logging behavior.
Explain Hypervel's ancestor lockfile search for applications nested in JavaScript workspaces and the project-local execution commands used by pnpm and Yarn. Record these deliberate improvements alongside the otherwise Laravel-compatible package manager API.
Document that Watcher owns both its file-watching driver and restarted server process, and stops both when the command receives a termination signal. This makes the development process lifecycle guarantee explicit for maintainers and package users.
Record the motivation, public API, process ownership, registration priority, Composer path-repository handling, ancestor lockfile detection, Watcher shutdown behavior, implementation structure, and verification strategy for the development command feature. Keep the design as a durable reference for future Laravel synchronization and Hypervel maintenance.
Describe DevCommand as a typed command definition rather than an immutable value. Its fluent color methods intentionally update the command definition, so the corrected wording now matches the implemented public API.
@coderabbitai

coderabbitai Bot commented Jul 14, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Adds Laravel-shaped dev and dev:list commands, a development process registry, Node package-manager detection, watcher signal cleanup, service-provider wiring, static-state cleanup, tests, and documentation.

Changes

Development orchestration

Layer / File(s) Summary
Node package-manager detection and execution
src/support/src/NodePackageManager*, src/support/src/NodePackageManagers/*, tests/Support/*, src/support/README.md
Adds lockfile-based package-manager detection, command builders for Bun, pnpm, Yarn, and npm, ancestor-directory lookup, and coverage for precedence and fallback behavior.
Development command registry and command model
src/foundation/src/DevCommand*, src/foundation/src/DevCommands.php, tests/Foundation/FoundationDevCommandTest.php, tests/Foundation/FoundationDevCommandsTest.php, src/testing/src/PHPUnit/AfterEachTestSubscriber.php
Adds typed command metadata, priority resolution, registration helpers, filtering, color assignment, default registrations, and test-state flushing.
Dev commands, listing, and service wiring
src/foundation/src/Console/*Dev*, src/foundation/src/Providers/FoundationServiceProvider.php, src/foundation/composer.json, tests/Foundation/Console/*, tests/Foundation/Providers/*
Adds dev process orchestration through concurrently, dev:list filtering and output modes, console registration, and the prompts dependency.
Watcher shutdown and application resolution
src/watcher/src/Console/WatchCommand.php, tests/Watcher/WatchCommandTest.php, src/watcher/README.md
Uses the application contract for console detection and stops the watcher driver before the restart strategy on termination signals.
Design and test isolation
docs/plans/2026-07-14-development-command-orchestration.md, src/foundation/README.md, tests/Queue/*
Documents orchestration behavior and Laravel differences, while resetting development state and fixing deterministic queue timing tests.

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 8.94% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is concise and accurately summarizes the PR’s main change: adding development process orchestration.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/dev-command

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@greptile-apps

greptile-apps Bot commented Jul 14, 2026

Copy link
Copy Markdown

Greptile Summary

This PR adds a Laravel-compatible development process orchestration surface to Hypervel, adapting it for Swoole worker lifetime and monorepo layout constraints. The implementation is complete and well-tested, covering the dev and dev:list commands, a static DevCommands registry with three-tier priority resolution, Bun/npm/pnpm/Yarn adapters with ancestor-directory lockfile detection, and a targeted WatchCommand signal-handling fix.

  • DevCommands registry: Resolves registration priority from debug_backtrace, correctly distinguishing framework defaults (registerDefaults), Composer path-repository packages (via InstalledVersions), vendor files, and userland callers. Shell escaping for concurrently arguments was verified against a regression covering quotes, env expansion, command separators, and spaced names.
  • dev command: Uses pcntl_exec to hand the process to concurrently, with a passthru fallback; COLUMNS modification is intentionally not restored on the exec path because the process is replaced. The coroutine = false flag keeps native process ownership for subprocess management.
  • WatchCommand: Traps SIGINT/SIGTERM/SIGQUIT before entering the watcher loop, stopping the driver first and the restart strategy in finally. ApplicationContract replaces the concrete Foundation\Application import.

Confidence Score: 5/5

All changes are additive and isolated to new dev-tooling commands, a new package-manager detection layer, and a targeted watcher signal-cleanup fix. No existing runtime paths are altered.

The implementation is thorough and the edge cases are well-covered by focused regression tests. Priority resolution through debug_backtrace, lockfile ancestor walking, shell escaping, terminal-width truncation, single-snapshot color consumption, coroutine opt-out, and signal ordering are all explicitly tested.

No files require special attention.

Important Files Changed

Filename Overview
src/foundation/src/DevCommands.php Core registry for dev commands. Priority resolution using debug_backtrace is well-guarded: frames from DevCommands.php are skipped, registerDefaults is detected via class+function, and path packages are resolved through InstalledVersions. Static color cycling and flushState() are correct.
src/foundation/src/Console/DevCommand.php Dev console command with pcntl_exec path and passthru fallback. Shell-escaping of colors, commands, and names is correct. COLUMNS env var is modified before exec and restored on the passthru path only (intentional — pcntl_exec replaces the process). coroutine=false ensures native console process ownership.
src/foundation/src/Console/DevListCommand.php Lists dev commands with filtering, JSON output, and terminal-width-aware source truncation. The --filter=0 edge case is correctly handled. Source truncation avoids negative widths and gracefully omits the column on very narrow terminals.
src/support/src/NodePackageManager.php Package-manager detector that walks ancestor directories for lockfiles. Bun→pnpm→Yarn→npm priority order, npm fallback when no lockfile found. getcwd() false-return edge case is guarded by the while condition.
src/watcher/src/Console/WatchCommand.php Adds SIGINT/SIGTERM/SIGQUIT trap before watcher loop to guarantee driver and strategy cleanup on signal. ApplicationContract replaces concrete Application import. Signal handler ordering verified by new tests.
src/testing/src/PHPUnit/AfterEachTestSubscriber.php Adds DevCommand (Prohibitable flag) and DevCommands (registry + color counter) to the after-each flush list. Both entries are correctly placed in alphabetical order within their respective blocks.

Reviews (3): Last reviewed commit: "Update development command verification ..." | Re-trigger Greptile

Comment thread src/foundation/src/DevCommands.php
Comment thread src/watcher/src/Console/WatchCommand.php

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (5)
src/foundation/src/Console/DevListCommand.php (1)

119-122: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Align isFiltering logic with filterCommands.

If the --filter option is passed as '0', $this->option('filter') evaluates to false in the boolean context of isFiltering(), but it is successfully processed as an active filter in filterCommands() (since '0' !== ''). Aligning the condition prevents incorrect empty-state output and exit codes for this edge case.

♻️ Proposed refactor
     protected function isFiltering(): bool
     {
-        return $this->option('filter') || $this->option('except-vendor') || $this->option('only-vendor');
+        $filter = $this->option('filter');
+
+        return (is_string($filter) && $filter !== '') || $this->option('except-vendor') || $this->option('only-vendor');
     }
🤖 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 `@src/foundation/src/Console/DevListCommand.php` around lines 119 - 122, Update
DevListCommand::isFiltering() to determine whether --filter is active using the
same non-empty-string semantics as filterCommands(), so a filter value of '0' is
treated as enabled; preserve the existing vendor-option checks.
src/foundation/src/DevCommands.php (3)

310-320: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

flushState() is missing the worker-lifetime mutator warning entirely.

Unlike every other public mutator in this class, flushState() has no "Boot-only."/"Tests only." docblock, even though calling it outside test teardown wipes every registered dev command for the rest of the worker's life (the concrete failure mode: php artisan dev would then report "No development commands are configured to run." for all subsequent requests on that worker).

📝 Suggested docblock addition
     /**
      * Flush all static state.
+     *
+     * Tests only. Calling this outside test teardown erases every registered
+     * dev command for the remaining worker lifetime, breaking `dev`/`dev:list`.
      */
     public static function flushState(): void

As per coding guidelines: "Add Boot-only., Tests only., or Boot or tests only. warnings to public mutators of worker-lifetime state... name the concrete failure mode in the second sentence."

🤖 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 `@src/foundation/src/DevCommands.php` around lines 310 - 320, Add a
worker-lifetime warning docblock to the public static mutator flushState(),
marking it “Boot or tests only.” and stating that calling it during a worker
request clears registered development commands, causing later requests to report
that no development commands are configured.

Source: Coding guidelines


46-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Docblocks say "Boot-only." but these are also directly called from tests.

registerDefaults(), register(), artisan(), only(), except(), etc. are all documented as "Boot-only," yet tests/Foundation/FoundationDevCommandsTest.php calls every one of them directly from test methods (e.g. DevCommands::register(...), DevCommands::registerDefaults(), DevCommands::only(...)), which is a legitimate, intended usage pattern for tests.

📝 Suggested docblock wording
-     * Boot-only. The commands persist in static properties for the worker lifetime
+     * Boot or tests only. The commands persist in static properties for the worker lifetime
      * and affect every subsequent development command invocation.

Apply the same substitution to register(), artisan(), node(), nodeExec(), only(), and except().

As per coding guidelines: "Add Boot-only., Tests only., or Boot or tests only. warnings to public mutators of worker-lifetime state when they are not intended for normal runtime use."

Also applies to: 65-101, 280-300

🤖 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 `@src/foundation/src/DevCommands.php` around lines 46 - 63, Update the
docblocks for the public worker-lifetime mutators registerDefaults(),
register(), artisan(), node(), nodeExec(), only(), and except() to say “Boot or
tests only.” instead of “Boot-only.”, reflecting their supported direct use by
tests while preserving the existing warning that they are not intended for
normal runtime use.

Source: Coding guidelines


246-270: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Consider caching resolved dependency install paths.

isDependencyFile() re-fetches InstalledVersions::getInstalledPackages() and calls realpath() on every installed package's install path for every stack frame of every register()/artisan()/node() call. Since Watcher restarts the worker (and re-runs registerDefaults()/boot) on every file change during local development, this repeated linear scan + filesystem I/O runs on every hot-reload, not just once per process lifetime in the traditional sense.

♻️ Suggested caching
+    /**
+     * Cached realpath()'d install directories of installed Composer dependencies.
+     *
+     * `@var` null|list<string>
+     */
+    protected static ?array $dependencyPaths = null;
+
     protected static function isDependencyFile(string $file): bool
     {
-        $rootPackage = InstalledVersions::getRootPackage()['name'];
-
-        foreach (InstalledVersions::getInstalledPackages() as $package) {
-            if ($package === $rootPackage) {
-                continue;
-            }
-
-            $installPath = InstalledVersions::getInstallPath($package);
-
-            if ($installPath === null || ($installPath = realpath($installPath)) === false) {
-                continue;
-            }
-
-            if (self::isWithinPath($file, $installPath)) {
-                return true;
-            }
-        }
-
-        return false;
+        foreach (self::$dependencyPaths ??= self::resolveDependencyPaths() as $installPath) {
+            if (self::isWithinPath($file, $installPath)) {
+                return true;
+            }
+        }
+
+        return false;
+    }
+
+    /**
+     * `@return` list<string>
+     */
+    protected static function resolveDependencyPaths(): array
+    {
+        $rootPackage = InstalledVersions::getRootPackage()['name'];
+        $paths = [];
+
+        foreach (InstalledVersions::getInstalledPackages() as $package) {
+            if ($package === $rootPackage) {
+                continue;
+            }
+
+            $installPath = InstalledVersions::getInstallPath($package);
+
+            if ($installPath !== null && ($installPath = realpath($installPath)) !== false) {
+                $paths[] = $installPath;
+            }
+        }
+
+        return $paths;
     }

Also reset self::$dependencyPaths = null; in flushState().

🤖 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 `@src/foundation/src/DevCommands.php` around lines 246 - 270, Cache the
resolved non-root dependency install paths used by isDependencyFile() so
InstalledVersions::getInstalledPackages() and realpath() are performed once per
process, then reuse the cached paths for each file check. Add or use a static
dependency-path cache and reset it to null in flushState() so state is rebuilt
after a reset.
src/foundation/src/Console/DevCommand.php (1)

93-98: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Command/color/name segments aren't shell-escaped before being embedded in the concurrently invocation.

$colors, $commands, and $names are interpolated directly into a double-quoted shell string. Since DevCommand::color() accepts any string and registered command strings are developer-supplied, a command or color containing a literal " would break the quoting and produce a broken/unexpected shell invocation (this isn't an externally-exploitable injection here since inputs are locally developer-configured, but it is a real robustness gap).

🛡️ Suggested escaping
         $command = $packageManager->getExecCommand(sprintf(
-            'concurrently -c "%s" "%s" --names=%s --kill-others-on-fail',
-            implode(',', $colors),
-            implode('" "', $commands),
-            implode(',', $names)
+            'concurrently -c %s %s --names=%s --kill-others-on-fail',
+            escapeshellarg(implode(',', $colors)),
+            implode(' ', array_map(escapeshellarg(...), $commands)),
+            escapeshellarg(implode(',', $names))
         ));
🤖 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 `@src/foundation/src/Console/DevCommand.php` around lines 93 - 98, Update the
command construction in DevCommand’s concurrently invocation to shell-escape
each color, command, and name segment before interpolation. Preserve the
existing comma-separated colors/names and individually quoted commands, while
ensuring embedded quotes or other shell-significant characters cannot break the
generated command.
🤖 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.

Nitpick comments:
In `@src/foundation/src/Console/DevCommand.php`:
- Around line 93-98: Update the command construction in DevCommand’s
concurrently invocation to shell-escape each color, command, and name segment
before interpolation. Preserve the existing comma-separated colors/names and
individually quoted commands, while ensuring embedded quotes or other
shell-significant characters cannot break the generated command.

In `@src/foundation/src/Console/DevListCommand.php`:
- Around line 119-122: Update DevListCommand::isFiltering() to determine whether
--filter is active using the same non-empty-string semantics as
filterCommands(), so a filter value of '0' is treated as enabled; preserve the
existing vendor-option checks.

In `@src/foundation/src/DevCommands.php`:
- Around line 310-320: Add a worker-lifetime warning docblock to the public
static mutator flushState(), marking it “Boot or tests only.” and stating that
calling it during a worker request clears registered development commands,
causing later requests to report that no development commands are configured.
- Around line 46-63: Update the docblocks for the public worker-lifetime
mutators registerDefaults(), register(), artisan(), node(), nodeExec(), only(),
and except() to say “Boot or tests only.” instead of “Boot-only.”, reflecting
their supported direct use by tests while preserving the existing warning that
they are not intended for normal runtime use.
- Around line 246-270: Cache the resolved non-root dependency install paths used
by isDependencyFile() so InstalledVersions::getInstalledPackages() and
realpath() are performed once per process, then reuse the cached paths for each
file check. Add or use a static dependency-path cache and reset it to null in
flushState() so state is rebuilt after a reset.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: f8ad20fc-38fe-466f-9684-f5f4ede6480a

📥 Commits

Reviewing files that changed from the base of the PR and between f706013 and c858b24.

📒 Files selected for processing (26)
  • docs/plans/2026-07-14-development-command-orchestration.md
  • src/foundation/README.md
  • src/foundation/composer.json
  • src/foundation/src/Console/DevCommand.php
  • src/foundation/src/Console/DevListCommand.php
  • src/foundation/src/DevCommand.php
  • src/foundation/src/DevCommandColor.php
  • src/foundation/src/DevCommands.php
  • src/foundation/src/Providers/FoundationServiceProvider.php
  • src/support/README.md
  • src/support/src/Contracts/NodePackageManager.php
  • src/support/src/NodePackageManager.php
  • src/support/src/NodePackageManagers/Bun.php
  • src/support/src/NodePackageManagers/Npm.php
  • src/support/src/NodePackageManagers/Pnpm.php
  • src/support/src/NodePackageManagers/Yarn.php
  • src/testing/src/PHPUnit/AfterEachTestSubscriber.php
  • src/watcher/README.md
  • src/watcher/src/Console/WatchCommand.php
  • tests/Foundation/Console/DevCommandTest.php
  • tests/Foundation/FoundationDevCommandTest.php
  • tests/Foundation/FoundationDevCommandsTest.php
  • tests/Foundation/FoundationDevListCommandTest.php
  • tests/Foundation/Providers/FoundationServiceProviderTest.php
  • tests/Support/SupportNodePackageManagerTest.php
  • tests/Watcher/WatchCommandTest.php

Treat a literal zero filter as active so dev:list reports an empty filtered result as a failure, matching the predicate used to apply the filter.

Shell-escape concurrently colors, process commands, and names at the outer shell boundary. This preserves each registered command for concurrently's child shell and prevents quoting, expansion, or command separators from being interpreted too early.

Add focused regressions for both cases, including a command with quotes, environment expansion, shell operators, and a spaced process name.
Freeze Carbon while both mirrored DateInterval tests calculate their exact ten-second timer delay.

InteractsWithTime converts the interval and reads the current timestamp separately. Crossing a whole-second boundary between those reads can turn the expected delay into nine seconds, so the exact timer assertion must run against a fixed clock.

Reset the clock with the same pattern used by the adjacent DateTime tests; the framework after-each subscriber remains the failure-path cleanup guarantee.
Record the final framework test and assertion counts after adding the two bot-review regression tests.

The Testbench and dogfood totals remain unchanged, and the complete composer fix pipeline still passes.
@binaryfire

Copy link
Copy Markdown
Collaborator Author

@coderabbitai @greptile-apps — review follow-up is pushed.

Implemented in 732bb47db:

  • dev:list now treats --filter=0 as an active filter, using the same predicate for filtering and empty-result status. A regression test covers the failure exit code.
  • dev now shell-escapes colors, each process command, and names before building the outer concurrently invocation. This keeps shell syntax intact until concurrently starts the child command. The regression covers quotes, environment expansion, command separators, and a spaced name.

The remaining findings do not require code changes:

  • The referenced foreach variable is local to fillInEmptyColors(), and the method returns immediately after the loop. There is no later access that can reuse the reference. An unset would only guard a future edit and would not change current behavior.
  • WatchCommand resolves the concrete ServerRestartStrategy, whose stop() method catches every Throwable. The proposed double-throw path is not reachable; the existing finally guarantees server cleanup when driver shutdown throws.
  • flushState() keeps the repository-standard title-only docblock. The public registry mutators remain boot-only APIs even though tests exercise them.
  • Dependency-path resolution runs only while development commands are registered. A worker-lifetime cache would add mutable state and reset logic without a practical gain on this cold path.
  • The generic docstring coverage score does not match this repository’s Laravel-style method docblock convention, so adding filler comments would reduce signal.

While running the full checks, the exact DateInterval timer assertions exposed a whole-second clock-boundary race. bac9f16e3 freezes Carbon in both mirrored queue tests, matching their adjacent DateTime tests. ff035c417 updates the recorded verification totals.

Full verification is green:

  • CS Fixer: no changes
  • Both PHPStan configurations: no errors
  • Framework: 22,640 tests, 64,325 assertions, 1,585 skipped
  • Testbench: 327 tests, 955 assertions, 3 skipped
  • Dogfood: 4 tests, 7 assertions

@coderabbitai

coderabbitai Bot commented Jul 14, 2026

Copy link
Copy Markdown

Tip

For best results, initiate chat on the files or code changes.

@binaryfire Thanks for the detailed follow-up — this all looks solid.

The two fixes (treating --filter=0 as an active filter with a shared predicate, and shell-escaping colors/commands/names before the outer concurrently invocation) directly address the concerns raised, and the regression tests covering quotes, environment expansion, command separators, and spaced names give good confidence in the escaping behavior.

On the remaining items, your explanations are convincing and I won't re-raise them:

  • The fillInEmptyColors() foreach variable is indeed local and unused after the loop returns, so no dangling-reference risk.
  • The unreachable double-throw path in WatchCommand is confirmed given ServerRestartStrategy::stop() catches all Throwable.
  • The flushState() docblock and cold-path dependency resolution tradeoffs are reasonable calls given the repo's conventions and practical impact.

Good catch on the Carbon clock-boundary race in the queue tests too — freezing time to match the adjacent DateTime tests is the right fix.

Everything green across CS Fixer, both PHPStan configs, and the full Framework/Testbench/Dogfood suites. Nice work.


✏️ Learnings added
Learnt from: binaryfire
URL: https://github.com/hypervel/components/pull/434

Timestamp: 2026-07-14T13:56:49.755Z
Learning: In Hypervel's `Hypervel\Watcher\Console\WatchCommand`, the resolved `ServerRestartStrategy::stop()` implementation catches every `Throwable` internally, so a double-throw scenario during termination signal handling (driver stop then restart-strategy stop both throwing) is not reachable; the existing `finally` block guarantees server cleanup even if driver shutdown throws.

Learnt from: binaryfire
URL: https://github.com/hypervel/components/pull/434

Timestamp: 2026-07-14T13:56:49.755Z
Learning: In Hypervel's `src/foundation/src/DevCommands.php`, the `fillInEmptyColors()` method's foreach loop variable is local and the method returns immediately after the loop, so there is no risk of the loop variable being reused later; an `unset()` is unnecessary there.

Learnt from: binaryfire
URL: https://github.com/hypervel/components/pull/434

Timestamp: 2026-07-14T13:56:49.755Z
Learning: In Hypervel, `Hypervel\Foundation\DevCommands::flushState()` intentionally keeps a title-only docblock, matching the repository-standard convention; the public registry mutators (`register`, `artisan`, `node`, `nodeExec`, `only`, `except`, `flushState`) are boot-only APIs even though tests exercise them directly.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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 `@tests/Queue/QueueBackgroundQueueTest.php`:
- Around line 164-165: Make Carbon test-clock cleanup exception-safe in the
tests at tests/Queue/QueueBackgroundQueueTest.php lines 164-165 and 185-186 and
tests/Queue/QueueDeferredQueueTest.php lines 164-165 and 185-186: after freezing
Carbon, wrap each test body in try/finally and move Carbon::setTestNow() reset
into the finally block so cleanup runs even when assertions fail.
🪄 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: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: ca4a1ab8-74d1-436a-b2d2-ce803d0f5958

📥 Commits

Reviewing files that changed from the base of the PR and between c858b24 and ff035c4.

📒 Files selected for processing (7)
  • docs/plans/2026-07-14-development-command-orchestration.md
  • src/foundation/src/Console/DevCommand.php
  • src/foundation/src/Console/DevListCommand.php
  • tests/Foundation/Console/DevCommandTest.php
  • tests/Foundation/FoundationDevListCommandTest.php
  • tests/Queue/QueueBackgroundQueueTest.php
  • tests/Queue/QueueDeferredQueueTest.php
🚧 Files skipped from review as they are similar to previous changes (5)
  • src/foundation/src/Console/DevCommand.php
  • tests/Foundation/Console/DevCommandTest.php
  • docs/plans/2026-07-14-development-command-orchestration.md
  • src/foundation/src/Console/DevListCommand.php
  • tests/Foundation/FoundationDevListCommandTest.php

Comment thread tests/Queue/QueueBackgroundQueueTest.php
@binaryfire
binaryfire merged commit 242c5b2 into 0.4 Jul 14, 2026
35 of 36 checks passed
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