fix(legacy): handle a null exit code in Shell::runProcess()

[pjcdawkins]

If a process can't start, Symfony throws ProcessStartFailedException, which is a subclass of ProcessFailedException, and getExitCode() returns null. When $mustRun was false, Shell::runProcess() returned that null from a method declared to return int. The result was a TypeError.

In CI this came from Config::getVersion(). When application.version is still the placeholder, it runs git describe --tags with CLI_ROOT as the working directory. Inside a phar, CLI_ROOT is a phar:// path. That path passes Symfony's is_dir() check, but proc_open() can't use it.

Changes:

• runProcess() returns 1 when the exit code is null.
• getVersion() skips the git lookup when CLI_ROOT is a phar:// path.
• Removes the PHPStan baseline entry that was hiding this bug.
• Adds a test that uses a phar:// tar directory as the working directory. It reproduced the TypeError before the fix.

🤖 Generated with Claude Code

[upsun-dispatch]

Warning

Changes suggested — 🟡 1 warning · 🔵 1 minor point

🔍 Full review · 4 files reviewed

Verification

• $process->getExitCode() ?? 1 keeps the declared int return type, and execute()'s $exitCode > 0 check turns it into the documented false result.
• The removed phpstan-baseline entry matches exactly the runProcess() should return int but returns int|null error the change fixes; no other baseline entry for that method is orphaned.
• HasTempDirTrait::tearDown() uses !empty($this->tempDir), which is isset-safe, so the other two tests in the class that never call tempDirSetUp() do not hit an uninitialized typed property.
• The new guard in getVersion() is evaluated in the same branch that previously ran the git lookup, so non-phar installs keep the git describe --tags fallback unchanged.

The diff adds ShellServiceTest::testExecuteStartFailure(), which builds a phar:// tar directory and asserts execute() returns false; it runs in CI's legacy-php job via ./scripts/test/unit.sh, and that job also runs make lint-phpstan, which is what the removed baseline entry affects. No test covers the $mustRun = true start-failure path or the new phar:// branch in Config::getVersion().

Review details

• Commit: 722028e
• Model: claude-opus-5

---

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

[upsun-dispatch]

Note

Reviewed — No new blocking findings · 🔵 1 minor point · 1 still open

🔁 Incremental · 2 files reviewed

🔵 Minor point

• legacy/src/Service/Shell.php:223 — Rethrowing the raw ProcessStartFailedException avoids the LogicException at the throw site, but hands a never-started Process to the handlers that catch Symfony's ProcessFailedException and then read the process. Shell::exceptionMeansCommandDoesNotExist() (Shell.php ~283) does $process->getExitCode() === 127 (null for a start failure, so false) and then, on Windows, $process->getErrorOutput(), which calls requireProcessIsStarted() and throws LogicException: Process must be started before calling getErrorOutput(). On Windows Symfony starts processes with bypass_shell, so a missing executable makes proc_open() itself fail: Git::ensureInstalled() (Git.php:61-62) on a Windows machine without git now reaches that catch and dies with the opaque LogicException instead of the intended DependencyMissingException('Git must be installed'). Same for Shell::findWhere()'s fallback attempt (Shell.php:259-261). Guarding exceptionMeansCommandDoesNotExist() with !$process->isStarted() ? true/false (or checking $e instanceof ProcessStartFailedException) would complete the fix.

Outstanding from earlier reviews:

• 🔵 #4099111704 — legacy/src/Service/Config.php:550: Same latent failure remains for every other caller passing a phar path. — Declined by the author; Shell::setupProcess() still accepts any phar:// dir passing is_dir(), so the guard remains per-call-site in Config::getVersion().

Verification

• Process::start() leaves status READY when proc_open() fails, so !$process->isStarted() correctly selects the start-failure case for the rethrow.
• The !$mustRun early return still precedes the new guard, so execute() keeps returning false (exit code 1) for a start failure.
• ProcessStartFailedException exists in the locked symfony/process v7.4.19, so the new test import resolves.
• findWhere()'s catch (ProcessFailedException) uses Symfony's base class, so the rethrown start-failure exception is still caught there.

The diff adds mustExecute() coverage to testExecuteStartFailure in legacy/tests/Service/ShellServiceTest.php, asserting ProcessStartFailedException; CI's php job runs ./scripts/test/unit.sh plus make lint-phpstan (.github/workflows/ci.yml). No test covers the Windows exceptionMeansCommandDoesNotExist() path described in the finding.

Review details

• Commits: 722028e..745b6f4
• Model: claude-opus-5

---

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

[pjcdawkins]

Re the minor point in the latest review: fixed in 65bbefe. exceptionMeansCommandDoesNotExist() now returns false for a process that never started, and the test covers this.

One correction to the scenario: on Windows, Symfony wraps every command in cmd /C (Process::prepareWindowsCommandLine()). So a missing git doesn't make proc_open() fail, and Git::ensureInstalled() was not affected. The guard matters for other start failures, such as an unusable working directory.

🤖 Addressed by Claude Code

[upsun-dispatch]

Warning

Changes suggested — 🟡 1 warning

🔁 Incremental (head + base moved) · 2 files reviewed

Verification

• The !$process->isStarted() guard is placed before getExitCode()/getErrorOutput(), so the LogicException from requireProcessIsStarted() can no longer be reached from this helper.
• ProcessStartFailedException exists in the pinned symfony/process v7.4.19 (composer.lock) and extends ProcessFailedException, so the test's catch and runProcess()'s catch both match.
• The rewritten test asserts a real return value rather than relying on expectException, so the assertion after the throw is actually executed.
• Base-branch movement (#107) touched only Go files under internal/ and commands/; nothing it renamed is referenced by this PHP change.

Covered by ShellServiceTest::testExecuteStartFailure(), which is run by the legacy-php job's "Run PHPUnit tests" step in .github/workflows/ci.yml; that job runs on Linux only, so the Windows bypass_shell start-failure path the new guard mainly affects is not exercised by any test.

Review details

• Commits: 745b6f4..65bbefe
• Base moved since: dbc11df
• Model: claude-opus-5

---

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