chore(legacy): raise PHPStan to level 10

[pjcdawkins]

Raises PHPStan in legacy/ from level 9 to level 10. Level 10 checks mixed values reaching typed parameters, which is where unchecked input and interactive validator bugs show up.

The 644 new errors were reviewed. These real bugs are fixed, each with a test that failed before the fix:

• resources:set: --disk app:default and --disk app:min always failed with "must be an integer in MB".
• resources:set (interactive): when every profile size offered was an integer (e.g. 1, 2), the chosen profile_size was sent as the description text ("CPU 2, memory 768 MB (shared)"). Fixed in QuestionHelper::chooseAssoc().
• auth:api-token-login and auth:verify-phone-number: pressing Enter at the prompt crashed with a TypeError instead of asking again.
• Filesystem::remove($dir, true): the chmod retry for read-only files (used by local:build) never worked, because Symfony had already renamed the directory. Permissions are now fixed before removal.

The remaining errors are baselined (1178 → 1786 entries). Most are mixed values from API responses and decoded JSON. Two edge cases are left for follow-up: the local:web router with passthru: true, and session:switch with all-numeric session IDs.

🤖 Generated with Claude Code

[upsun-dispatch]

Note

Reviewed — No blocking findings · 🔵 2 minor points

🔍 Full review · 10 files reviewed

🔵 Minor points

• legacy/src/Service/Filesystem.php:65 — remove() now calls unprotect($files, true) outside the try block on every $chmod call, not only after a failed delete. unprotect() builds new \FilesystemIterator($file, ...) for each directory, which throws UnexpectedValueException ("failed to open dir") when the directory cannot be opened — e.g. a directory the user owns with mode 0300 (write+exec, not readable: the chmod guard !is_executable($file) || !is_writable($file) is false for it, so it is never relaxed), or a directory removed by another process between is_dir() and the iteration. That exception is not an IOException, so it escapes remove() instead of being turned into a warning and a false return, aborting commands such as local:build with an unhandled exception where the old code returned false.
• legacy/src/Service/QuestionHelper.php:170 — chooseAssoc() maps the answer back to a key with (string) array_search($choice, $items, true) when all keys are integer-like. Two failure modes follow from casting the result: if two entries share the same description string (Symfony only rejects ambiguity when the user types the text, not when they type the index) the first matching key is returned, so the wrong item is chosen; and if the value is not found at all array_search returns false, which (string) turns into "" — the method then reports an empty key as a valid choice instead of failing. The sibling choose() guards the same lookup with if ($choiceKey === false) throw new \RuntimeException(...); this path does not. The condition also re-implements Symfony's private ChoiceQuestion::isAssoc() heuristic, so a change there silently reintroduces the description-instead-of-key bug.

Verification

• validateDiskSize() now handles 'default'/'min'/'minimum' before the integer check, so those keywords no longer hit the 'must be an integer in MB' error.
• Both new validator closures accept ?string and reject empty input with an exception the Symfony question loop catches and re-prompts on, rather than a TypeError.
• remove() materialises a non-string iterable with iterator_to_array() before unprotect(), so a generator of paths is no longer consumed before fs->remove() sees it.
• unprotect() now iterates with FilesystemIterator::CURRENT_AS_PATHNAME, so is_link()/is_dir() get strings as strict_types requires instead of SplFileInfo.
• chooseAssoc()'s new key lookup only triggers when every key is integer-like, leaving string-keyed callers such as EnvironmentDeployCommand unchanged.

Each behaviour fix has a test in the diff: integration-tests/resources_set_values_test.go covers --disk app:default|min and the interactive integer-keyed profile-size choice; integration-tests/empty_input_test.go covers empty input at the two auth prompts; FilesystemServiceTest::testRemoveDirWithChmod covers remove($dir, true) for a string and a generator. CI runs these via the integration-test and legacy-php jobs in .github/workflows/ci.yml, which also runs make lint-phpstan at the new level 10; no PHP unit test covers QuestionHelper::chooseAssoc directly.

Review details

• Commit: 8a735f9
• Model: claude-opus-5

---

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

[upsun-dispatch]

Note

Reviewed — No blocking findings · 🔵 2 minor points

🔁 Incremental · 4 files reviewed

🔵 Minor points

• legacy/src/Service/Filesystem.php:106 — The new catch (\UnexpectedValueException) stops the iterator-construction failure escaping unprotect(), but it only converts it into false, which remove() discards (line 65 ignores the return value) before calling $this->fs->remove($files). Symfony's Filesystem::remove() itself builds new \FilesystemIterator($file, ...) for every directory it descends into, and that throws the same UnexpectedValueException ("failed to open dir"). So for a directory whose mode cannot be relaxed — e.g. a root-owned sub-directory with mode 0300 inside a tree being cleaned by local:build, where @chmod fails because the user is not the owner — Filesystem::remove($dir, true) still terminates with an uncaught UnexpectedValueException (not an IOException) instead of returning false with a warning.
• legacy/src/Service/QuestionHelper.php:166 — The new validator short-circuits on is_string($answer) && array_key_exists($answer, $items), which changes resolution for genuinely associative item sets where one item's value is also another item's key. Given ['a' => 'b', 'b' => 'c'], typing b previously went through ChoiceQuestion's assoc validator, which resolves a typed value to its key and returned 'a'; now it returns 'b'. Neither of the two current call sites (EnvironmentDeployCommand, ResourcesSetCommand) can collide this way, so nothing breaks today, but a future item list whose labels overlap its keys will silently select the wrong entry.

Verification

• Adding !is_readable($file) makes the 0300 directory in the extended testRemoveDirWithChmod case get chmod'd to 0700 before the FilesystemIterator is built.
• chooseAssoc() now returns a key on every path: scalar answers matching a key are cast and returned, others are reverse-looked-up, and an unresolvable answer throws RuntimeException instead of a TypeError.
• The pre-validator handles the all-integer-key case (array_key_exists('2', [1=>…,2=>…]) is true), so typing 2 yields "2" rather than the description text; integration test TestResourcesSet_InteractiveIntegerProfileSizes asserts exactly that.
• Non-interactive mode still works: Symfony's getDefaultAnswer runs the new validator over the default, which returns the default key unchanged for 'stopstart' in EnvironmentDeployCommand.
• The untyped fn($answer) closures introduce no unbaselined PHPStan error — no Anonymous function has parameter ... with no type specified entries exist for the many similar arrow functions already in src/.

FilesystemServiceTest::testRemoveDirWithChmod gains the 0300 directory case (run by the Run PHPUnit tests step in .github/workflows/ci.yml), and the PR's integration-tests/resources_set_values_test.go covers the interactive integer-profile-size path through chooseAssoc; the baseline/level change is exercised by the Lint PHP (phpstan) step. No unit test covers chooseAssoc directly, and nothing covers the new UnexpectedValueException catch (it needs a directory whose chmod fails).

Review details

• Commits: 8a735f9..3ac73e1
• Model: claude-opus-5

---

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

[upsun-dispatch]

Note

Reviewed — No blocking findings · 🔵 1 minor point

🔁 Incremental · 2 files reviewed

🔵 Minor point

• legacy/src/Service/QuestionHelper.php:167 — The new && !in_array($answer, $items, true) guard is meant to give a typed value precedence over an identical key, but for integer-like keys — the very case the surrounding code exists for — the later post-processing undoes it. With $items = ['1' => '3', '2' => '1'] and the user typing 1: the guard sees 1 is also a value, so Symfony's default validator runs; because PHP made the keys ints the choices are not associative, so Symfony returns the choice text '1'; then line 175 (if (is_scalar($choice) && array_key_exists((string) $choice, $items)) return (string) $choice;) maps that text back to the key '1' and returns it, i.e. the key interpretation wins anyway, and the option actually labelled 1 (key '2') is never selectable. For string keys the guard does change behaviour (['a'=>'b','b'=>'c'] + typed b now yields a), so the two key types now resolve the same ambiguity in opposite directions.

Verification

• \UnexpectedValueException extends \RuntimeException and Symfony's IOException does too, so the widened catch in remove() covers both the old and the new case.
• The removed use IOException import is unused elsewhere in Filesystem.php, so dropping it breaks nothing.
• remove() still returns false and raises an E_USER_WARNING when Symfony's recursive FilesystemIterator cannot open a sub-directory, instead of letting the exception escape.
• The two chooseAssoc() call sites (EnvironmentDeployCommand, ResourcesSetCommand) pass items whose keys and values do not overlap, so the validator change does not alter them today.

These two commits add no tests: the widened catch in Filesystem::remove() and the new !in_array() validator branch are uncovered (existing FilesystemServiceTest::testRemoveDirWithChmod only exercises the success path). CI's test job runs make lint-phpstan and ./scripts/test/unit.sh over legacy/.

Review details

• Commits: 3ac73e1..066f5d4
• Model: claude-opus-5

---

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

[upsun-dispatch]

Note

Reviewed — No blocking findings · 🔵 1 minor point

🔁 Incremental · 3 files reviewed

🔵 Minor point

• legacy/src/Service/QuestionHelper.php:167 — The new validator coerces a non-scalar answer (which is what Symfony passes when the user presses Enter and the question has no default: null) to the empty string before matching. If $items ever contains a key '' — or a value '' — then a bare Enter with no default silently resolves to that entry instead of re-prompting, because array_key_exists('', $items) / array_keys($items, '', true) matches. Today's two call sites (ResourcesSetCommand, EnvironmentDeployCommand) never build such an item, so this is latent rather than live, but the helper is generic and the empty-input path is exactly what this change set out to make safe.

Verification

• The validator returns a key for both spellings: typing the key and typing the value each resolve to the key, matching the new tests for string and integer-like keys.
• Non-interactive chooseAssoc still works: Symfony's getDefaultAnswer() runs the validator on the default, so '2' comes back as the key, as testChooseAssocNonInteractive asserts.
• InvalidArgumentException from the validator is caught by Symfony's validateAttempts, so a blank or unmatched answer re-prompts (max 5) instead of crashing.
• Both current callers (ResourcesSetCommand:272, EnvironmentDeployCommand:110) are inside interactive guards, so the new LogicException on a null choice is unreachable from them.
• Removing the chooseAssoc() expects array<string, string> baseline entry is matched by the widened array<array-key, string> docblock on the method.

The diff adds legacy/tests/Service/QuestionHelperTest.php, covering key/value/default answers for both string and integer-like keys plus the non-interactive path; the legacy-php job in .github/workflows/ci.yml runs make lint-phpstan and PHPUnit over legacy/.

Review details

• Commits: 066f5d4..0bdb2c7
• Model: claude-opus-5

---

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

[upsun-dispatch]

Note

Reviewed — No blocking findings · 🔵 1 minor point

🔁 Incremental · 2 files reviewed

🔵 Minor point

• legacy/src/Service/QuestionHelper.php:168 — The validator now rejects every empty answer before the key lookup, so an items array containing an empty-string key can never be chosen: chooseAssoc(['' => 'None', 'a' => 'Apple']) loops for 5 attempts on Enter and then throws InvalidArgumentException('A choice is required') instead of returning '', and in non-interactive mode chooseAssoc($items, $text, '') throws the same from Question::getDefaultAnswer() rather than returning the default. The two current callers (EnvironmentDeployCommand.php:110, ResourcesSetCommand.php:272) use non-empty keys, so nothing fails today, but the new testChooseAssocEmptyAnswer case pins this as the behaviour for any future caller with an empty key. Moving the emptiness check after array_key_exists($answer, $items) would keep rejecting blank input while leaving a genuine '' key selectable.

Verification

• The new empty-answer guard sits before the key/value lookups, so the existing key-, value- and default-resolution cases in chooseAssocProvider are unaffected.
• Both chooseAssoc call sites (EnvironmentDeployCommand.php:110, ResourcesSetCommand.php:272) pass only non-empty keys, so the new throw cannot fire for them.
• I ran the new test file against an installed Symfony 7 console: validateAttempts rethrows the stored InvalidArgumentException when the memory stream hits EOF, so testChooseAssocEmptyAnswer does assert the intended exception and not MissingInputException.
• testChooseAssocNonInteractive still passes because Question::getDefaultAnswer routes the non-null default '2' through the new validator, which returns the key unchanged.

The diff adds legacy/tests/Service/QuestionHelperTest::testChooseAssocEmptyAnswer, covering the new branch; it runs in the ci.yml "Run PHPUnit tests" step (./scripts/test/unit.sh), and I executed that single test file locally against a fresh composer install — all 9 cases pass.

Review details

• Commits: 0bdb2c7..88a7acb
• Model: claude-opus-5

---

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

[upsun-dispatch]

Note

Reviewed — No blocking findings · ⚪ 1 nitpick

🔁 Incremental · 2 files reviewed

⚪ Nitpick

• legacy/src/Service/QuestionHelper.php:171 — Moving the emptiness check to a strict $answer === null test means a whitespace-only or empty typed answer (which trims to '') no longer produces "A choice is required" for item sets without an '' key; it now falls through to sprintf('Value "%s" is invalid', ''), printing Value "" is invalid. The retry still happens, only the message is less helpful.

Verification

• Reproduced the new path with real symfony/console 7: with no default Symfony hands the validator null, so the A choice is required branch is still reached and testChooseAssocEmptyAnswer still throws.
• The added provider case is genuine: with default '' Symfony returns '' from doAsk, ChoiceQuestion's normalizer leaves it, and array_key_exists('', $items) makes chooseAssoc return ''.
• Both existing call sites (EnvironmentDeployCommand, ResourcesSetCommand) pass non-empty keys, so admitting '' as a valid key changes nothing for them.
• chooseAssoc never enables multiselect, so the is_scalar(...) ? ... : '' fallback cannot silently select an '' key from an array answer.

The change is covered by the new empty key as default data-provider row plus the pre-existing testChooseAssocEmptyAnswer/testChooseAssocNonInteractive cases in legacy/tests/Service/QuestionHelperTest.php, run by the legacy-php CI job (./scripts/test/unit.sh, which also runs php-cs-fixer and make lint-phpstan).

Review details

• Commits: 88a7acb..b028bca
• Model: claude-opus-5

---

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