fix/php version regex and installer output

data/ci/sudo-helper.sh

@@ -4,7 +4,7 @@
 # This script performs all operations that require root privileges:
 # - Adding ondrej PPA
 # - Running apt-get update
-# - Installing PHP versions (8.2, 8.3, 8.4, 8.5)
+# - Installing PHP versions (whichever versions ondrej/php currently ships)
 # - Installing PHP extensions
 #
 # This script is designed to be called by horde-components CI setup
@@ -30,8 +30,17 @@ case "$OPERATION" in
     install-php)
         # Install PHP version
         # Usage: install-php 8.4
+        #
+        # The regex only enforces the shape MAJOR.MINOR so command
+        # injection via shell metacharacters is impossible. Deciding
+        # which versions are actually installable is not this script's
+        # job: PhpInstaller picks the matrix, ondrej/php decides what
+        # apt can resolve, and apt's own "Unable to locate package ..."
+        # is the authoritative signal. Hard-coding a version range here
+        # (previously ^8\.[2-5]$) silently breaks the day PHP 8.6 or 9.0
+        # ships.
         PHP_VERSION="$1"
-        if [[ ! "$PHP_VERSION" =~ ^8\.[2-5]$ ]]; then
+        if [[ ! "$PHP_VERSION" =~ ^[0-9]+\.[0-9]+$ ]]; then
             echo "ERROR: Invalid PHP version: $PHP_VERSION" >&2
             exit 1
         fi
@@ -43,7 +52,7 @@ case "$OPERATION" in
         # Usage: install-extension 8.4 curl
         PHP_VERSION="$1"
         EXTENSION="$2"
-        if [[ ! "$PHP_VERSION" =~ ^8\.[2-5]$ ]]; then
+        if [[ ! "$PHP_VERSION" =~ ^[0-9]+\.[0-9]+$ ]]; then
             echo "ERROR: Invalid PHP version: $PHP_VERSION" >&2
             exit 1
         fi

src/Ci/Run/PrCommentReporter.php

@@ -356,6 +356,39 @@ private function allPhpStanLanesAdvisory(array $results): bool
         return $seen;
     }
 
+    /**
+     * Render the watermark+1 advisory suffix for the PHPStan PR-comment
+     * line. The maintainer sees "— N advisor{y,ies} at level X" appended
+     * to the success line when the PHPStan task captured advisory data
+     * for the first failing level above the post-discovery watermark.
+     *
+     * Returns an empty string when:
+     * - No lane reported advisory data (the field is absent or null
+     *   across the board).
+     * - The advisory error count is zero (would be misleading - if
+     *   level+1 had zero errors, the discovery loop would have raised
+     *   the watermark to that level and there would be no advisory).
+     *
+     * @param array<string, mixed> $phpstanStats Aggregated stats from
+     *                                            {@see self::aggregateToolStats()}.
+     */
+    private function formatAdvisoryNextLevelSuffix(array $phpstanStats): string
+    {
+        $level = $phpstanStats['advisory_level'] ?? null;
+        $errors = $phpstanStats['advisory_errors_max'] ?? 0;
+
+        if ($level === null || $errors <= 0) {
+            return '';
+        }
+
+        return sprintf(
+            ' — %d %s at level %d',
+            $errors,
+            $errors === 1 ? 'advisory' : 'advisories',
+            $level,
+        );
+    }
+
     /**
      * True when every lane that produced a PHPUnit result reported
      * `mode: no_test_suite` (no test/ directory found by Unit::run).
@@ -578,8 +611,15 @@ private function generateQualityMetrics(array $results, array $findingsByTool =
                 );
             } else {
                 $errors = $phpstanStats['errors'] ?? 0;
+                // Watermark+1 advisory: the maintainer's "promotion
+                // budget." Shown only when the lane is passing (no
+                // hard errors) and the PHPStan task captured advisory
+                // data for the next level above watermark. When the
+                // watermark fails, advisory data is meaningless because
+                // there's no level+1 peek.
+                $advisorySuffix2 = $this->formatAdvisoryNextLevelSuffix($phpstanStats);
                 if ($errors === 0) {
-                    $md .= "- **PHPStan**{$advisorySuffix}: No errors found ✅\n";
+                    $md .= "- **PHPStan**{$advisorySuffix}: No errors found ✅{$advisorySuffix2}\n";
                 } else {
                     $md .= sprintf(
                         "- **PHPStan**%s: %d error%s found ⚠️\n",
@@ -907,6 +947,39 @@ private function aggregateToolStats(array $results, string $tool): array
                     $value
                 );
             }
+
+            // PHPStan watermark+1 advisory: capture per-lane level and
+            // error count, then surface the max-across-lanes counts so
+            // the PR comment can show a single "N advisories at level
+            // M" suffix. Different lanes might end up with slightly
+            // different post-discovery watermarks (a fix that's
+            // version-gated could pass level X on PHP 8.3 but not 8.2),
+            // so taking the max errors at the highest advisory level
+            // is the safe upper bound for the maintainer's promotion
+            // budget.
+            $advisory = $result['advisory_next_level'] ?? null;
+            if (is_array($advisory)
+                && isset($advisory['level'], $advisory['errors'])
+                && is_int($advisory['level'])
+                && is_int($advisory['errors'])
+            ) {
+                // Track the highest advisory level seen and its
+                // associated error count. If multiple lanes report
+                // different levels (e.g. lane A says level 4, lane B
+                // says level 5 because B's actual watermark is higher),
+                // pick the level seen and report its max-across-lanes
+                // error count.
+                $currentLevel = $aggregated['advisory_level'] ?? null;
+                if ($currentLevel === null || $advisory['level'] > $currentLevel) {
+                    $aggregated['advisory_level'] = $advisory['level'];
+                    $aggregated['advisory_errors_max'] = $advisory['errors'];
+                } elseif ($advisory['level'] === $currentLevel) {
+                    $aggregated['advisory_errors_max'] = max(
+                        $aggregated['advisory_errors_max'] ?? 0,
+                        $advisory['errors'],
+                    );
+                }
+            }
         }
 
         return $aggregated;

src/Ci/Run/ResultCollector.php

@@ -99,6 +99,15 @@ public function addResultFromFile(string $laneName, string $tool, string $jsonFi
             // "which tests failed" view across lanes.
             'failures' => $data['failures'] ?? [],
             'errors_detail' => $data['errors'] ?? [],
+            // PHPStan watermark+1 advisory: the smallest failing level
+            // above watermark, captured by the PHPStan task during its
+            // auto-raise discovery loop. The PR-comment renderer reads
+            // this through aggregateToolStats() and appends a "N
+            // advisories at level X" suffix to the PHPStan success
+            // line. Null when watermark fails, watermark is already
+            // at the maximum, or the maintainer reaches the maximum
+            // via auto-raise (no further level to peek at).
+            'advisory_next_level' => $data['advisory_next_level'] ?? null,
         ];
     }
 

src/Ci/Run/RunCommand.php

@@ -364,20 +364,46 @@ private function generateSummaryMarkdown(): string
 
         // Lane results table
         $md .= "### Lane Results\n\n";
-        $md .= "| Lane | PHPUnit | PHPStan | PHP-CS-Fixer | Status |\n";
-        $md .= "|------|---------|---------|--------------|--------|\n";
+
+        // Watermark+1 advisory: add an optional `PHPStan Advisory`
+        // column when at least one lane has captured advisory data.
+        // The column is suppressed entirely when every lane is silent
+        // (watermark already at max, or watermark failing across the
+        // board) so non-modernised components don't get a useless
+        // empty column.
+        $showAdvisoryColumn = $this->anyLaneHasPhpStanAdvisory($results);
+
+        if ($showAdvisoryColumn) {
+            $md .= "| Lane | PHPUnit | PHPStan | PHPStan Advisory | PHP-CS-Fixer | Status |\n";
+            $md .= "|------|---------|---------|------------------|--------------|--------|\n";
+        } else {
+            $md .= "| Lane | PHPUnit | PHPStan | PHP-CS-Fixer | Status |\n";
+            $md .= "|------|---------|---------|--------------|--------|\n";
+        }
 
         foreach ($results as $laneName => $tools) {
             $laneStatus = $this->isLanePassed($tools) ? '✅' : '❌';
 
-            $md .= sprintf(
-                "| %s | %s | %s | %s | %s |\n",
-                $laneName,
-                $this->formatToolForTable($tools['phpunit'] ?? null, 'phpunit'),
-                $this->formatToolForTable($tools['phpstan'] ?? null, 'phpstan'),
-                $this->formatToolForTable($tools['phpcsfixer'] ?? null, 'phpcsfixer'),
-                $laneStatus
-            );
+            if ($showAdvisoryColumn) {
+                $md .= sprintf(
+                    "| %s | %s | %s | %s | %s | %s |\n",
+                    $laneName,
+                    $this->formatToolForTable($tools['phpunit'] ?? null, 'phpunit'),
+                    $this->formatToolForTable($tools['phpstan'] ?? null, 'phpstan'),
+                    $this->formatPhpStanAdvisoryForTable($tools['phpstan'] ?? null),
+                    $this->formatToolForTable($tools['phpcsfixer'] ?? null, 'phpcsfixer'),
+                    $laneStatus
+                );
+            } else {
+                $md .= sprintf(
+                    "| %s | %s | %s | %s | %s |\n",
+                    $laneName,
+                    $this->formatToolForTable($tools['phpunit'] ?? null, 'phpunit'),
+                    $this->formatToolForTable($tools['phpstan'] ?? null, 'phpstan'),
+                    $this->formatToolForTable($tools['phpcsfixer'] ?? null, 'phpcsfixer'),
+                    $laneStatus
+                );
+            }
         }
 
         // Detailed metrics
@@ -541,6 +567,67 @@ private function formatPhpCsFixerCell(array $stats, bool $success): string
         return sprintf('❌ %d files need formatting', $withIssues);
     }
 
+    /**
+     * True when at least one lane's PHPStan result carries an
+     * `advisory_next_level` block with a positive error count. Drives
+     * whether the lane table renders the `PHPStan Advisory` column. A
+     * lane reporting `passing: true` at the next level should never
+     * happen (the discovery loop would have auto-raised the watermark
+     * instead) but guard against it anyway so we don't emit a useless
+     * empty column.
+     *
+     * @param array<string,array<string,array<string,mixed>>> $results
+     */
+    private function anyLaneHasPhpStanAdvisory(array $results): bool
+    {
+        foreach ($results as $tools) {
+            $phpstan = $tools['phpstan'] ?? null;
+            if (!is_array($phpstan)) {
+                continue;
+            }
+            $advisory = $phpstan['advisory_next_level'] ?? null;
+            if (!is_array($advisory)) {
+                continue;
+            }
+            $errors = (int) ($advisory['errors'] ?? 0);
+            if ($errors > 0) {
+                return true;
+            }
+        }
+        return false;
+    }
+
+    /**
+     * Render the `PHPStan Advisory` table cell for a single lane.
+     *
+     * Returns `+N @ L` when the PHPStan task captured `N` advisory
+     * findings at level `L` above this lane's post-discovery watermark,
+     * or `-` when the lane has no advisory data (watermark failed,
+     * watermark is already at the maximum, or auto-raise reached the
+     * maximum). The cell deliberately omits a ✅ icon: the maintainer
+     * already sees the green status in the adjacent `PHPStan` cell;
+     * this column is the climbing budget, not a verdict.
+     *
+     * @param array<string,mixed>|null $phpstanResult The lane's
+     *                                                  PHPStan tool block.
+     */
+    private function formatPhpStanAdvisoryForTable(?array $phpstanResult): string
+    {
+        if (!is_array($phpstanResult)) {
+            return '-';
+        }
+        $advisory = $phpstanResult['advisory_next_level'] ?? null;
+        if (!is_array($advisory)) {
+            return '-';
+        }
+        $level = $advisory['level'] ?? null;
+        $errors = (int) ($advisory['errors'] ?? 0);
+        if (!is_int($level) || $errors <= 0) {
+            return '-';
+        }
+        return sprintf('+%d @ %d', $errors, $level);
+    }
+
     /**
      * Generate detailed metrics section.
      *

src/Ci/Setup/ExtensionInstaller.php

@@ -482,9 +482,24 @@ private function installExtension(string $extension, string $phpVersion): ?bool
             return null; // Skip
         }
 
-        // Try to install using sudo helper
-        if (!SudoHelper::installExtension($phpVersion, $extension)) {
-            $this->output->warn("Failed to install {$package}");
+        // Try to install using sudo helper.
+        //
+        // Surface apt's captured output on failure — this is the layer
+        // where "Unable to locate package php8.4-imagick" turns into a
+        // useful clue for the reader of the CI log. A bare warn() with
+        // just the package name used to leave people guessing whether
+        // the package name was wrong, the PPA was missing, or apt was
+        // in some transient bad state.
+        $result = SudoHelper::installExtension($phpVersion, $extension);
+        if (!$result['success']) {
+            if ($result['output'] !== '') {
+                $this->output->plain($result['output']);
+            }
+            $this->output->warn(sprintf(
+                'Failed to install %s (sudo helper exited %d)',
+                $package,
+                $result['exitCode']
+            ));
             return false;
         }
 

src/Ci/Setup/PhpInstaller.php

@@ -150,11 +150,26 @@ private function isPpaAdded(): bool
     /**
      * Add ondrej PPA.
      *
+     * On failure, prints whatever the sudo helper (and any apt commands
+     * it invoked) wrote to stdout/stderr before returning false, so
+     * callers don't have to guess whether add-apt-repository, apt-get
+     * update, or PPA GPG import was the thing that broke.
+     *
      * @return bool
      */
     private function addPpa(): bool
     {
-        return SudoHelper::addPpa();
+        $result = SudoHelper::addPpa();
+        if (!$result['success']) {
+            if ($result['output'] !== '') {
+                $this->output->plain($result['output']);
+            }
+            $this->output->error(sprintf(
+                'sudo helper exited %d while adding ondrej/php PPA',
+                $result['exitCode']
+            ));
+        }
+        return $result['success'];
     }
 
     /**
@@ -273,9 +288,23 @@ private function aptHasPackage(string $package): bool
      */
     private function installPhpVersion(string $version): bool
     {
-        // Use sudo helper to install PHP
-        if (!SudoHelper::installPhp($version)) {
-            $this->output->error("Failed to install PHP {$version}");
+        // Use sudo helper to install PHP.
+        //
+        // On failure, print the helper's captured output (which is
+        // apt-get's own message plus, if the version was rejected by
+        // the shape guard in sudo-helper.sh, that rejection line) so
+        // the CI log shows the real reason — e.g. "E: Unable to locate
+        // package php9.0-cli" — instead of a bare "Failed to install
+        // PHP 9.0".
+        $result = SudoHelper::installPhp($version);
+        if (!$result['success']) {
+            if ($result['output'] !== '') {
+                $this->output->plain($result['output']);
+            }
+            $this->output->error(sprintf(
+                "Failed to install PHP {$version} (sudo helper exited %d)",
+                $result['exitCode']
+            ));
             return false;
         }